[lldb/test] Catch invalid calls to expect()

[kastiglione]

Summary

Add preconditions to TestBase.expect() that catch semantically invalid calls
that happen to succeed anyway. This also fixes the broken callsites caught by
these checks.

This prevents the following incorrect calls:

1. self.expect("lldb command", "some substr")
2. self.expect("lldb command", "assert message", "some substr")
3. self.expect("lldb command", substrs="some substr")
4. self.expect("lldb command", patterns="some pattern")

[labath]

lldb/packages/Python/lldbsuite/test/lldbtest.py

2440–2441

assert isinstance(substrs, six.string_types), "substrs must be a collection of strings" ?

[teemperor]

Thanks for fixing all of this! Beside the assertion this LGTM.

lldb/packages/Python/lldbsuite/test/lldbtest.py

2440–2441

Or we could do assertNotIsInstance for a better error message that should show the actual type.

[DavidSpickett]

Does it make sense to add a few tests for this to lldb/test/API/assert_messages_test/TestAssertMessages.py? (this is only checking expect() messages, despite the generic name)

[aprantl]

Oh wow — that paid off!

lldb/test/API/types/TestRecursiveTypes.py

54

I'm not not sure I understand this change?

[kastiglione]

Resyntax the isinstance asserts; Add expect() tests

lldb/packages/Python/lldbsuite/test/lldbtest.py

2440–2441

It turns out that in practice assertNotIsInstance isn't a good match for this case because six.string_types is a tuple, and the formatted string becomes "… is an instance of (<class 'str'>,)".

[kastiglione]

lldb/test/API/types/TestRecursiveTypes.py

54

The second parameter, msg, is only valid if there are other "matchers", like substrs or startstr. If not, then the msg isn't used and so shouldn't need to be passed. By preventing a msg here, it allows the precondition to distinguish between cases where the msg is valid, and where it's not.

[kastiglione]

"missing a matcher" -> "missing a matcher argument"

[shafik]

I am happy this was not hiding any regressions.

lldb/packages/Python/lldbsuite/test/lldbtest.py

def expect(
         client is expecting the output of the command not to match the golden
         input.
 
         Finally, the required argument 'str' represents the lldb command to be
         sent to the command interpreter.  In case the keyword argument 'exe' is
         set to False, the 'str' is treated as a string to be matched/not-matched
         against the golden input.
         """
+        # Catch cases where `expect` has been miscalled. Specifically, prevent
+        # this easy to make mistake:
+        #     self.expect("lldb command", "some substr")
+        # The `msg` parameter is used only when a failed match occurs. A failed
+        # match can only occur when one of `patterns`, `startstr`, `endstr`, or
+        # `substrs` has been given. Thus, if a `msg` is given, it's an error to
+        # not also provide one of the matcher parameters.
+        if msg and not (patterns or startstr or endstr or substrs or error):
+            assert False, "expect() missing a matcher argument"
+
+        # Check `patterns` and `substrs` are not accidentally given as strings.
+        assert not isinstance(patterns, six.string_types), \
+            "patterns must be a collection of strings"
+        assert not isinstance(substrs, six.string_types), \
+            "substrs must be a collection of strings"
+
         trace = (True if traceAlways else trace)
 
         if exe:
             # First run the command.  If we are expecting error, set check=False.
             # Pass the assert message along since it provides more semantic
             # info.
             self.runCmd(
                 str,

lldb/test/API/assert_messages_test/TestAssertMessages.py

def test_expect(self):
         self.assert_expect_fails_with("mouse",
             dict(startstr="cat", exe=False, msg="Reason for check goes here!"),
             """\
                Checking string:
                "mouse"
 
                Expecting start string: "cat" (was not found)
                Reason for check goes here!""")
+
+        # Verify expect() preconditions.
+        # Both `patterns` and `substrs` cannot be of type string.
+        self.assert_expect_fails_with("any command",
+            dict(patterns="some substring"),
+            "patterns must be a collection of strings")
+        self.assert_expect_fails_with("any command",
+            dict(substrs="some substring"),
+            "substrs must be a collection of strings")
+        # Prevent `self.expect("cmd", "substr")`
+        self.assert_expect_fails_with("any command",
+            dict(msg="some substring"),
+            "expect() missing a matcher argument")
+        # Prevent `self.expect("cmd", "msg", "substr")`
+        self.assert_expect_fails_with("any command",
+            dict(msg="a message", patterns="some substring"),
+            "must be a collection of strings")

lldb/test/API/commands/frame/diagnose/bad-reference/TestBadReference.py

class TestBadReference(TestBase):
     def test_bad_reference(self):
         TestBase.setUp(self)
         self.build()
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
         self.runCmd("run", RUN_SUCCEEDED)
         self.expect("thread list", "Thread should be stopped",
                     substrs=['stopped'])
-        self.expect("frame diagnose", "Crash diagnosis was accurate", "f->b")
+        self.expect("frame diagnose", "Crash diagnosis was accurate",
+                    substrs=["f->b"])

lldb/test/API/commands/frame/diagnose/complicated-expression/TestComplicatedExpression.py

def test_diagnose_dereference_argument(self):
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
         self.runCmd("run", RUN_SUCCEEDED)
         self.expect("thread list", "Thread should be stopped",
                     substrs=['stopped'])
         self.expect(
             "frame diagnose",
             "Crash diagnosis was accurate",
-            "f->b->d")
+            substrs=["f->b->d"])

lldb/test/API/commands/frame/diagnose/dereference-argument/TestDiagnoseDereferenceArgument.py

def test_diagnose_dereference_argument(self):
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
         self.runCmd("run", RUN_SUCCEEDED)
         self.expect("thread list", "Thread should be stopped",
                     substrs=['stopped'])
         self.expect(
             "frame diagnose",
             "Crash diagnosis was accurate",
-            "f->b->d")
+            substrs=["f->b->d"])

lldb/test/API/commands/frame/diagnose/dereference-this/TestDiagnoseDereferenceThis.py

def test_diagnose_dereference_this(self):
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
         self.runCmd("run", RUN_SUCCEEDED)
         self.expect("thread list", "Thread should be stopped",
                     substrs=['stopped'])
         self.expect(
             "frame diagnose",
             "Crash diagnosis was accurate",
-            "this->a")
+            substrs=["this->a"])

lldb/test/API/commands/frame/diagnose/inheritance/TestDiagnoseInheritance.py

class TestDiagnoseInheritance(TestBase):
     def test_diagnose_inheritance(self):
         TestBase.setUp(self)
         self.build()
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
         self.runCmd("run", RUN_SUCCEEDED)
         self.expect("thread list", "Thread should be stopped",
                     substrs=['stopped'])
-        self.expect("frame diagnose", "Crash diagnosis was accurate", "d")
+        self.expect("frame diagnose", "Crash diagnosis was accurate",
+                    substrs=["d"])

lldb/test/API/commands/frame/diagnose/local-variable/TestLocalVariable.py

class TestLocalVariable(TestBase):
     def test_local_variable(self):
         TestBase.setUp(self)
         self.build()
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
         self.runCmd("run", RUN_SUCCEEDED)
         self.expect("thread list", "Thread should be stopped",
                     substrs=['stopped'])
-        self.expect("frame diagnose", "Crash diagnosis was accurate", "myInt")
+        self.expect("frame diagnose", "Crash diagnosis was accurate",
+                    substrs=["myInt"])

lldb/test/API/commands/frame/diagnose/virtual-method-call/TestDiagnoseDereferenceVirtualMethodCall.py

class TestDiagnoseVirtualMethodCall(TestBase):
     def test_diagnose_virtual_method_call(self):
         TestBase.setUp(self)
         self.build()
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
         self.runCmd("run", RUN_SUCCEEDED)
         self.expect("thread list", "Thread should be stopped",
                     substrs=['stopped'])
-        self.expect("frame diagnose", "Crash diagnosis was accurate", "foo")
+        self.expect("frame diagnose", "Crash diagnosis was accurate",
+                    substrs=["foo"])

lldb/test/API/commands/settings/TestSettings.py

def test_settings_with_quotes(self):
             'settings set target.env-vars ["MY_FILE"]="this is a file name with spaces.txt"')
         self.expect("settings show target.env-vars",
                     substrs=['MY_FILE=this is a file name with spaces.txt'])
         self.runCmd("settings clear target.env-vars")
         # Test and make sure that setting "format-string" settings obeys quotes
         # if they are provided
         self.runCmd("settings set thread-format    'abc def'   ")
         self.expect("settings show thread-format",
-                    'thread-format (format-string) = "abc def"')
+                    startstr='thread-format (format-string) = "abc def"')
         self.runCmd('settings set thread-format    "abc def"   ')
         self.expect("settings show thread-format",
-                    'thread-format (format-string) = "abc def"')
+                    startstr='thread-format (format-string) = "abc def"')
         # Make sure when no quotes are provided that we maintain any trailing
         # spaces
         self.runCmd('settings set thread-format abc def   ')
         self.expect("settings show thread-format",
-                    'thread-format (format-string) = "abc def   "')
+                    startstr='thread-format (format-string) = "abc def   "')
         self.runCmd('settings clear thread-format')
 
     def test_settings_with_trailing_whitespace(self):
 
         # boolean
         # Set to known value
         self.runCmd("settings set target.skip-prologue true")
         # Set to new value with trailing whitespace

lldb/test/API/driver/batch_mode/TestBatchMode.py

def test_batch_mode_run_crash(self):
         # We should have hit the breakpoint & continued:
         child.expect_exact("continue")
         # The App should have crashed:
         child.expect_exact("About to crash")
         # The -k option should have printed the frame variable once:
         child.expect_exact('(char *) touch_me_not')
         # Then we should have a live prompt:
         self.expect_prompt()
-        self.expect("frame variable touch_me_not", substrs='(char *) touch_me_not')
+        self.expect("frame variable touch_me_not", substrs=['(char *) touch_me_not'])
 
     @expectedFlakeyFreeBSD("llvm.org/pr25172 fails rarely on the buildbot")
     def test_batch_mode_run_exit(self):
         """Test that the lldb driver's batch mode works correctly."""
         self.build()
 
         exe = self.getBuildArtifact("a.out")
 

lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py

class BreakpointByLineAndColumnTestCase(TestBase):
 
     ## Skip gcc version less 7.1 since it doesn't support -gcolumn-info
     @skipIf(compiler="gcc", compiler_version=['<', '7.1'])
     def testBreakpointByLineAndColumn(self):
         self.build()
         main_c = lldb.SBFileSpec("main.c")
         _, _, _, breakpoint = lldbutil.run_to_line_breakpoint(self,
                                                               main_c, 11, 50)
-        self.expect("fr v did_call", substrs='1')
+        self.expect("fr v did_call", substrs=['1'])
         in_then = False
         for i in range(breakpoint.GetNumLocations()):
             b_loc = breakpoint.GetLocationAtIndex(i).GetAddress().GetLineEntry()
             self.assertEqual(b_loc.GetLine(), 11)
             in_then |= b_loc.GetColumn() == 50
         self.assertTrue(in_then)
 
     ## Skip gcc version less 7.1 since it doesn't support -gcolumn-info
     @skipIf(compiler="gcc", compiler_version=['<', '7.1'])
     def testBreakpointByLine(self):
         self.build()
         main_c = lldb.SBFileSpec("main.c")
         _, _, _, breakpoint = lldbutil.run_to_line_breakpoint(self, main_c, 11)
-        self.expect("fr v did_call", substrs='0')
+        self.expect("fr v did_call", substrs=['0'])
         in_condition = False
         for i in range(breakpoint.GetNumLocations()):
             b_loc = breakpoint.GetLocationAtIndex(i).GetAddress().GetLineEntry()
             self.assertEqual(b_loc.GetLine(), 11)
             in_condition |= b_loc.GetColumn() < 30
         self.assertTrue(in_condition)

lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py

def test_nsindexpath_with_run_command(self):
             'frame variable t3',
             substrs=[
                 '1 10th of a second', 'value = 1', 'timescale = 10',
                 'epoch = 0'
             ])
         self.expect(
             'frame variable t4',
             substrs=['10 seconds', 'value = 10', 'timescale = 1', 'epoch = 0'])
-        self.expect('frame variable t5', '-oo')
-        self.expect('frame variable t6', '+oo')
-        self.expect('frame variable t7', 'indefinite')
+        self.expect('frame variable t5', substrs=['+oo'])
+        self.expect('frame variable t6', substrs=['-oo'])
+        self.expect('frame variable t7', substrs=['indefinite'])

lldb/test/API/lang/cpp/constructors/TestCppConstructors.py

def test_constructors(self):
         self.expect_expr("ClassWithOneCtor(2).value", result_type="int", result_value="2")
         self.expect_expr("ClassWithMultipleCtor(3).value", result_type="int", result_value="3")
         self.expect_expr("ClassWithMultipleCtor(3, 1).value", result_type="int", result_value="4")
 
         self.expect_expr("ClassWithDeletedCtor().value", result_type="int", result_value="6")
         self.expect_expr("ClassWithDeletedDefaultCtor(7).value", result_type="int", result_value="7")
 
         # FIXME: It seems we try to call the non-existent default constructor here which is wrong.
-        self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs="Couldn't lookup symbols:")
+        self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs=["Couldn't lookup symbols:"])
 
         # FIXME: Calling deleted constructors should fail before linking.
         self.expect("expr ClassWithDeletedCtor(1).value", error=True, substrs=["Couldn't lookup symbols:"])
         self.expect("expr ClassWithDeletedDefaultCtor().value", error=True, substrs=["Couldn't lookup symbols:"])
 
     @skipIfWindows # Can't find operator new.
     @skipIfLinux # Fails on some Linux systems with SIGABRT.
     def test_constructors_new(self):
         self.build()
         lldbutil.run_to_source_breakpoint(self,"// break here", lldb.SBFileSpec("main.cpp"))
 
         self.expect_expr("(new ClassWithOneCtor(1))->value; 1", result_type="int", result_value="1")

lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py

def test_macabi(self):
         self.build()
         log = self.getBuildArtifact('packets.log')
         self.expect("log enable gdb-remote packets -f "+log)
         lldbutil.run_to_source_breakpoint(self, "break here",
                                           lldb.SBFileSpec('main.c'))
         self.expect("image list -t -b",
                     patterns=[self.getArchitecture() +
                               r'.*-apple-ios.*-macabi a\.out'])
-        self.expect("fr v s", "Hello macCatalyst")
-        self.expect("p s", "Hello macCatalyst")
+        self.expect("fr v s", substrs=["Hello macCatalyst"])
+        self.expect("p s", substrs=["Hello macCatalyst"])
         self.check_debugserver(log)
 
     def check_debugserver(self, log):
         """scan the debugserver packet log"""
         process_info = lldbutil.packetlog_get_process_info(log)
         self.assertTrue('ostype' in process_info)
         self.assertEquals(process_info['ostype'], 'maccatalyst')
 
         aout_info = None
         dylib_info = lldbutil.packetlog_get_dylib_info(log)
         for image in dylib_info['images']:
             if image['pathname'].endswith('a.out'):
                 aout_info = image
         self.assertTrue(aout_info)
         self.assertEquals(aout_info['min_version_os_name'], 'maccatalyst')

lldb/test/API/types/TestRecursiveTypes.py

def print_struct(self):
             self,
             "recursive_type_main.cpp",
             self.line,
             num_expected_locations=-1,
             loc_exact=True)
 
         self.runCmd("run", RUN_SUCCEEDED)
 
-        self.expect("print tpi", RUN_SUCCEEDED)
-        self.expect("print *tpi", RUN_SUCCEEDED)
+        self.expect("print tpi")
+        self.expect("print *tpi")