[lldb] Add support for checking children in expect_expr

[teemperor]

Summary

expect_expr currently can't verify the children of the result SBValue.

This patch adds the ability to check them. The idea is to have a CheckValue
class where one can specify what attributes of a SBValue should be checked.
Beside the properties we already check for (summary, type, etc.) this also
has a list of children which is again just a list of CheckValue object (which
can also have children of their own).

The main motivation is to make checking the children no longer based
on error-prone substring checks that allow tests to pass just because
for example the error message contains the expected substrings by accident.

I also expect that we can just have a variant of expect_expr for LLDB's
expression paths (aka 'frame var') feature.

[labath]

Do you expect that the order of the children will be important to check?

I'm wondering if the children descriptor shouldn't be a name=>matcher map instead of a list...

Somewhat tying into that is the question whether the list of children should be exhaustive. With a list it makes sense for it to be exhaustive, with a map -- not so much.

But overall, I like the idea.

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

2492

If we make this a standalone class, could we get rid of the self. in the self.ValueCheck invocations?

2493–2494

the expect_s in the argument names seem unnecessary, and they make the invocations longer. Having the suffix in the property names is probably good.

[teemperor]

In D83792#2158243, @labath wrote:

I'm wondering if the children descriptor shouldn't be a name=>matcher map instead of a list...

Actually the 'name => matcher' map was my first implementation, but that turned out to be not working very well. The problem is that if we design it like that, then one always has to specify a name for this scheme to work. However for many unordered formatters, the names are the indizes of the respective element ([0], [1]), so it would make adding support for not enforcing the child order impossible (without a second API at least). Also having 'name' just being one of many properties one can check (or not) with a ValueCheck seems like a more consistent design.

Do you expect that the order of the children will be important to check?

Somewhat tying into that is the question whether the list of children should be exhaustive. With a list it makes sense for it to be exhaustive, with a map -- not so much.

But overall, I like the idea.

I think ignoring order and making it non-exhaustive should be added, but I just didn't add it in this patch (mostly because the code for that is not as straightforward as this patch). I would say that matching order and exhaustive list should be the default, so if we add it later it shouldn't cause any additional work down the line.

[teemperor]

• Removed expect_ prefix and make ValueCheck standalone (Thanks Pavel!)

[labath]

In D83792#2184207, @teemperor wrote:

In D83792#2158243, @labath wrote:

I'm wondering if the children descriptor shouldn't be a name=>matcher map instead of a list...

Actually the 'name => matcher' map was my first implementation, but that turned out to be not working very well. The problem is that if we design it like that, then one always has to specify a name for this scheme to work. However for many unordered formatters, the names are the indizes of the respective element ([0], [1]), so it would make adding support for not enforcing the child order impossible (without a second API at least). Also having 'name' just being one of many properties one can check (or not) with a ValueCheck seems like a more consistent design.

Do you expect that the order of the children will be important to check?

Somewhat tying into that is the question whether the list of children should be exhaustive. With a list it makes sense for it to be exhaustive, with a map -- not so much.

But overall, I like the idea.

I think ignoring order and making it non-exhaustive should be added, but I just didn't add it in this patch (mostly because the code for that is not as straightforward as this patch). I would say that matching order and exhaustive list should be the default, so if we add it later it shouldn't cause any additional work down the line.

ok, that makes sense to me.

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

if fpath:
             return program
     else:
         for path in os.environ["PATH"].split(os.pathsep):
             exe_file = os.path.join(path, program)
             if is_exe(exe_file):
                 return exe_file
     return None
 
+class ValueCheck:
+    def __init__(self, name=None, value=None, type=None, summary=None,
+                 children=None):
+        """
+        :param name: The name that the SBValue should have. None if the summary
+                     should not be checked.
+        :param summary: The summary that the SBValue should have. None if the
+                        summary should not be checked.
+        :param value: The value that the SBValue should have. None if the value
+                      should not be checked.
+        :param type: The type that the SBValue result should have. None if the
+                     type should not be checked.
+        :param children: A list of ValueChecks that need to match the children
+                         of this SBValue. None if children shouldn't be checked.
+                         The order of checks is the order of the checks in the
+                         list. The number of checks has to match the number of
+                         children.
+        """
+        self.expect_name = name
+        self.expect_value = value
+        self.expect_type = type
+        self.expect_summary = summary
+        self.children = children
+
+    def check_value(self, test_base, val, error_msg=None):
+        """
+        Checks that the given value matches the currently set properties
+        of this ValueCheck. If a match failed, the given TestBase will
+        be used to emit an error. A custom error message can be specified
+        that will be used to describe failed check for this SBValue (but
+        not errors in the child values).
+        """
+
+        this_error_msg = error_msg if error_msg else ""
+        this_error_msg += "\nChecking SBValue: " + str(val)
+
+        test_base.assertSuccess(val.GetError())
+
+        if self.expect_name:
+            test_base.assertEqual(self.expect_name, val.GetName(),
+                                  this_error_msg)
+        if self.expect_value:
+            test_base.assertEqual(self.expect_value, val.GetValue(),
+                                  this_error_msg)
+        if self.expect_type:
+            test_base.assertEqual(self.expect_type, val.GetDisplayTypeName(),
+                                  this_error_msg)
+        if self.expect_summary:
+            test_base.assertEqual(self.expect_summary, val.GetSummary(),
+                                  this_error_msg)
+        if self.children is not None:
+            self.check_value_children(test_base, val, error_msg)
+
+    def check_value_children(self, test_base, val, error_msg=None):
+        """
+        Checks that the children of a SBValue match a certain structure and
+        have certain properties.
+
+        :param test_base: The current test's TestBase object.
+        :param val: The SBValue to check.
+        """
+
+        this_error_msg = error_msg if error_msg else ""
+        this_error_msg += "\nChecking SBValue: " + str(val)
+
+        test_base.assertEqual(len(self.children), val.GetNumChildren(), this_error_msg)
+
+        for i in range(0, val.GetNumChildren()):
+            expected_child = self.children[i]
+            actual_child = val.GetChildAtIndex(i)
+            expected_child.check_value(test_base, actual_child, error_msg)
 
 class recording(SixStringIO):
     """
     A nice little context manager for recording the debugger interactions into
     our session object.  If trace flag is ON, it also emits the interactions
     into the stderr.
     """
 
def expect(
                 keepgoing = matched if matching else not matched
                 if not keepgoing:
                     break
 
         self.assertTrue(matched if matching else not matched,
                         msg + "\nCommand output:\n" + EXP_MSG(str, output, exe)
                         if msg else EXP_MSG(str, output, exe))
 
     def expect_expr(
             self,
             expr,
             result_summary=None,
             result_value=None,
             result_type=None,
+            result_children=None
             ):
         """
         Evaluates the given expression and verifies the result.
         :param expr: The expression as a string.
         :param result_summary: The summary that the expression should have. None if the summary should not be checked.
         :param result_value: The value that the expression should have. None if the value should not be checked.
         :param result_type: The type that the expression result should have. None if the type should not be checked.
+        :param result_children: The expected children of the expression result
+                                as a list of ValueChecks. None if the children shouldn't be checked.
         """
         self.assertTrue(expr.strip() == expr, "Expression contains trailing/leading whitespace: '" + expr + "'")
 
         frame = self.frame()
         options = lldb.SBExpressionOptions()
 
         # Disable fix-its that tests don't pass by accident.
         options.SetAutoApplyFixIts(False)
 
         # Set the usual default options for normal expressions.
         options.SetIgnoreBreakpoints(True)
 
         if self.frame().IsValid():
             options.SetLanguage(frame.GuessLanguage())
             eval_result = self.frame().EvaluateExpression(expr, options)
         else:
             target = self.target()
             # If there is no selected target, run the expression in the dummy
             # target.
             if not target.IsValid():
                 target = self.dbg.GetDummyTarget()
             eval_result = target.EvaluateExpression(expr, options)
 
-        self.assertSuccess(eval_result.GetError())
-
-        if result_type:
-            self.assertEqual(result_type, eval_result.GetDisplayTypeName())
-
-        if result_value:
-            self.assertEqual(result_value, eval_result.GetValue())
-
-        if result_summary:
-            self.assertEqual(result_summary, eval_result.GetSummary())
+        value_check = ValueCheck(type=result_type, value=result_value,
+                                 summary=result_summary, children=result_children)
+        value_check.check_value(self, eval_result, str(eval_result))
 
     def invoke(self, obj, name, trace=False):
         """Use reflection to call a method dynamically with no argument."""
         trace = (True if traceAlways else trace)
 
         method = getattr(obj, name)
         import inspect
         self.assertTrue(inspect.ismethod(method),

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py

class LibcxxMapDataFormatterTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def setUp(self):
         TestBase.setUp(self)
         ns = 'ndk' if lldbplatformutil.target_is_android() else ''
         self.namespace = 'std'
 
+    def check_pair(self, first_value, second_value):
+        pair_children = [ValueCheck(name="first", value=first_value),
+                         ValueCheck(name="second", value=second_value)]
+        return ValueCheck(children=pair_children)
+
     @add_test_categories(["libc++"])
     def test_with_run_command(self):
         """Test that that file and class static variables display correctly."""
         self.build()
         self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
 
         bkpt = self.target().FindBreakpointByID(
             lldbutil.run_break_set_by_source_regexp(
def test_with_run_command(self):
             self.runCmd(
                 "settings set target.max-children-count 256",
                 check=False)
 
         # Execute the cleanup function during test case tear down.
         self.addTearDownHook(cleanup)
 
         ns = self.namespace
-        self.expect('p ii',
-                    substrs=['%s::map' % ns,
-                             'size=0',
-                             '{}'])
+        self.expect_expr("ii", result_summary="size=0", result_children=[])
+
         self.expect('frame var ii',
                     substrs=['%s::map' % ns,
                              'size=0',
                              '{}'])
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
 
-        self.expect('p ii',
-                    substrs=['%s::map' % ns, 'size=2',
-                             '[0] = ',
-                             'first = 0',
-                             'second = 0',
-                             '[1] = ',
-                             'first = 1',
-                             'second = 1'])
+        self.expect_expr("ii", result_summary="size=2", result_children=[
+            self.check_pair("0", "0"),
+            self.check_pair("1", "1")
+        ])
 
         self.expect('frame variable ii',
                     substrs=['%s::map' % ns, 'size=2',
                              '[0] = ',
                              'first = 0',
                              'second = 0',
                              '[1] = ',
                              'first = 1',

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py

def check_numbers(self, var_name):
                              '[1] = 12',
                              '[2] = 123',
                              '[3] = 1234',
                              '[4] = 12345',
                              '[5] = 123456',
                              '[6] = 1234567',
                              '}'])
 
-        self.expect("p " + var_name,
-                    substrs=['$', 'size=7',
-                             '[0] = 1',
-                             '[1] = 12',
-                             '[2] = 123',
-                             '[3] = 1234',
-                             '[4] = 12345',
-                             '[5] = 123456',
-                             '[6] = 1234567',
-                             '}'])
+        self.expect_expr(var_name, result_summary="size=7", result_children=[
+            ValueCheck(value="1"),
+            ValueCheck(value="12"),
+            ValueCheck(value="123"),
+            ValueCheck(value="1234"),
+            ValueCheck(value="12345"),
+            ValueCheck(value="123456"),
+            ValueCheck(value="1234567"),
+        ])
 
         # check access-by-index
         self.expect("frame variable " + var_name + "[0]",
                     substrs=['1'])
         self.expect("frame variable " + var_name + "[1]",
                     substrs=['12'])
         self.expect("frame variable " + var_name + "[2]",
                     substrs=['123'])