[RFC] Make substrs argument to self.expect ordered by default.

[JDevlieghere]

Summary

This patch is more of an RFC to change the default behavior of the substrs argument to self.expect. Currently, the elements of substrs are unordered and as long as the string appears in the output, the assertion passes. I think we can be more precise by requiring that the substrings be ordered in the way they appear. My hope is that this will make it harder to accidentally pass a check because a string appears out of order.

A possible alternative is to keep the option but have it off by default. Currently there's about 50 tests failing because of this. If we like to move forward with making this the default I plan to incrementally update tests before flipping the switch.

[JDevlieghere]

Remove debug print

[teemperor]

I'm very much in favor of making this more strict by default. My only problem is that the current error message is really cryptic:

AssertionError: False is not True : 'expr --show-types -- *(StgClosure*)$r14' returns expected result, got '(StgClosure) $3 = {
  (StgClosure *) &$3 = 0x0000000000215eb0
  (int) addr = 2186928
  (int) load_address = 2186928
}'

I originally did something like this for expect_expr and having something like self.assertIn(subsets, output[:start], "Untruncated output: " + output) in there helped a lot with debugging.

[labath]

I think this is great, but do we really need the extra ordered argument for this? If you just fix the order of everything beforehand, then the switch to an ordered mode can be a no-op.

[teemperor]

FWIW I'm currently going over the failing tests and fixing them, so unless I find a test that actually needs unordered subsets I don't think having the parameter makes sense.

[JDevlieghere]

In D73766#1851224, @teemperor wrote:

FWIW I'm currently going over the failing tests and fixing them, so unless I find a test that actually needs unordered subsets I don't think having the parameter makes sense.

Yep, that was my reasoning as well, if everything can be converted without falling back to the old behavior there's no need to keep the parameter.

[jingham]

The current behavior does make tests less dependent on the exact details of lldb's command output. If there were two independent pieces of data in a command output that you wanted to compare against, you would have to fix some tests if you ever changed the ordering in the result. It was the case that the gdb test suite hampered the evolution of the command output because it was just too much of a pain to go change the tests. For instance, you really couldn't muck with breakpoint reporting output or stop notifications; the pain of fixing up the testsuite was way too high. But in this case, the fix would just be reordering the substrings in the Python array. So though this sort of thing makes me a little uneasy, in this case I think it's fine.

Note, if there is a test that depends on the substring output not coming out in a fixed order, then I think giving that test the stink-eye is more appropriate than keeping an "ordered" flag around.

lldb/packages/Python/lldbsuite/test/functionalities/mtc/simple/TestMTCSimple.py

def mtc_tests(self):
         view = "NSView" if lldbplatformutil.getPlatform() == "macosx" else "UIView"
 
         self.expect("thread info",
                     substrs=['stop reason = -[' + view +
                              ' superview] must be used from main thread only'])
 
         self.expect(
             "thread info -s",
-            substrs=["instrumentation_class", "api_name", "class_name", "selector", "description"])
+            substrs=["api_name", "class_name", "description", "instrumentation_class", "selector"])
         self.assertEqual(thread.GetStopReason(), lldb.eStopReasonInstrumentation)
         output_lines = self.res.GetOutput().split('\n')
         json_line = '\n'.join(output_lines[2:])
         data = json.loads(json_line)
         self.assertEqual(data["instrumentation_class"], "MainThreadChecker")
         self.assertEqual(data["api_name"], "-[" + view + " superview]")
         self.assertEqual(data["class_name"], view)
         self.assertEqual(data["selector"], "superview")
         self.assertEqual(data["description"], "-[" + view + " superview] must be used from main thread only")

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

def expect(
             str,
             msg=None,
             patterns=None,
             startstr=None,
             endstr=None,
             substrs=None,
             trace=False,
             error=False,
+            ordered=True,
             matching=True,
             exe=True,
             inHistory=False):
         """
         Similar to runCmd; with additional expect style output matching ability.
 
         Ask the command interpreter to handle the command and then check its
         return status.  The 'msg' parameter specifies an informational assert
def expect(
             matched = output.endswith(endstr)
             with recording(self, trace) as sbuf:
                 print("%s end string: %s" % (heading, endstr), file=sbuf)
                 print("Matched" if matched else "Not matched", file=sbuf)
 
         # Look for sub strings, if specified.
         keepgoing = matched if matching else not matched
         if substrs and keepgoing:
+            start = 0
             for substr in substrs:
-                matched = output.find(substr) != -1
+                index = output[start:].find(substr)
+                print("Looking for %s in %s" % (substr, output[start:]))
+                start = start + index if ordered and matching else 0
+                matched = index != -1
                 with recording(self, trace) as sbuf:
                     print("%s sub string: %s" % (heading, substr), file=sbuf)
                     print("Matched" if matched else "Not matched", file=sbuf)
                 keepgoing = matched if matching else not matched
                 if not keepgoing:
                     break
 
         # Search for regular expression patterns, if specified.