Adjust error_msg handling for expect_expr in lldbtest.py

[shafik]

Summary

I was trying to use the error_msg argument for expect_expr (looks like I am the first one) and the assumption that eval_result.IsValid() is false does not look correct, so I removed it.

I also changed the way we check of the error messages matches from an exact match to just looking for the string withing the error message. Matching the whole error message does not feel necessary.

[shafik]

I am open to suggestions on alternative approaches, for some context I ran into this trying to add a failing test to D75761 as was suggested.

[jingham]

The result from EvaluateExpression is pretty much always going to be valid, since the result holds the error. The correct way to do the first check is:

self.assertTrue(eval_result.GetError().Fail(), "Unexpected success...")

though you probably also want to make sure "eval_result.IsValid()" too, just to be on the safe side. It's good to check that first, especially if you are going to do a find rather than a strict compare. I don't think there's any guarantee that you have to leave the string in an Error empty if you return False from SBError().Fail().

I'm on the fence about using a "find" not a strict string compare. The only reason you'd be passing in an error_msg is that you want to test that you got the error string you were expecting. I worry that using substrings will lead to weakening this test by having too general a match. OTOH, it would be super annoying to match all of some of the compiler's error messages...

[shafik]

In D76080#1919862, @jingham wrote:

I'm on the fence about using a "find" not a strict string compare. The only reason you'd be passing in an error_msg is that you want to test that you got the error string you were expecting. I worry that using substrings will lead to weakening this test by having too general a match. OTOH, it would be super annoying to match all of some of the compiler's error messages...

I really would prefer not to have to match string like this warning: <user expression 0>:1:4: '<=>' is a single token in C++20; add a space to avoid a change in behavior.*" != "warning: <user expression 0>:1:4: '<=>' is a single token in C++20; add a space to avoid a change in behavior\nb1 <=> b2\n ^\nerror: <user expression 0>:1:6: expected expression\nb1 <=> b2\n ^\n"

it also feels fragile to changes in formatting etc...

[shafik]

Incorporate feedback on how to verify the results.

[teemperor]

• All of the asserts should print a useful error when failing (i.e., one that allows us to directly write a fix). You could do assertIn which is clearer than find(...) and automatically gives an error message that is useful. For the assertTrue just print the unexpected result in the assert message as the old code did.
• Just doing a plain find on a single string is a step backwards from the substr list in plain expect. The whole idea of the expect_expr is to have an API that is less prone to unintentionally passing tests and encouraging tests that are as strict as possible.

IMHO something similar to Clang's diagnostic verifier could be nice. So passing a list of expected errors and warnings that we expect and unexpected errors/warnings cause a test failure. The warnings themselves could be an ordered list of substrs (that's what Clang is doing IIRC). We don't have proper "error:" or "warning:" labels in all errors yet IIRC, but otherwise I guess that could work?

Maybe it makes sense to check the current error cases we have and see what we are usually testing for (I assume it's mostly Clang diagnostics).

[shafik]

Moving to using assertIn as suggest in comment.

[shafik]

In D76080#1920483, @teemperor wrote:

• All of the asserts should print a useful error when failing (i.e., one that allows us to directly write a fix). You could do assertIn which is clearer than find(...) and automatically gives an error message that is useful. For the assertTrue just print the unexpected result in the assert message as the old code did.

Using assertIn makes sense.

We are using assertIn in a few places and what we are looking for in those is not very helpful for guidance.

[shafik]

After discussing this @teemperor in some detail it looks like getting the error_msg case to work well is not a trivial task. So for these cases we should revert to just using expect. I think the plan is that he will remove the feature for now.

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

def expect_expr(
 
         # Set the usual default options for normal expressions.
         options.SetIgnoreBreakpoints(True)
         options.SetLanguage(frame.GuessLanguage())
 
         eval_result = frame.EvaluateExpression(expr, options)
 
         if error_msg:
-            self.assertFalse(eval_result.IsValid(), "Unexpected success with result: '" + str(eval_result) + "'")
-            self.assertEqual(error_msg, eval_result.GetError().GetCString())
+            self.assertTrue(eval_result.IsValid(), "Result is not valid")
+            self.assertTrue(eval_result.GetError().Fail(), "Unexpected success...")
+            self.assertIn(error_msg,eval_result.GetError().GetCString())
             return
 
         if not eval_result.GetError().Success():
             self.assertTrue(eval_result.GetError().Success(),
                 "Unexpected failure with msg: " + eval_result.GetError().GetCString())
 
         if result_type:
             self.assertEqual(result_type, eval_result.GetDisplayTypeName())