[lldb] Correct wording of EXP_MSG

[DavidSpickett]

Summary

EXP_MSG generates a message to show on assert
failure. Currently it looks like:
AssertionError: False is not True : '<cmd>'
returns expected result, got '<actual output>'

Which seems to say that the test failed but
also got the expected result.

It should say:
AssertionError: False is not True : '<cmd>'
returned unexpected result, got '<actual output>'

[teemperor]

If you look at the other "assert messages" above and below your patch you'll see that all (most?) of them use this very confusing "Say the opposite of what actually happened" style. Weirdly enough only around 70% of the "assertion messages" in the test suite are using this style, the others are using an "unexpected thing has happened" error message.

IMHO we should just completely remove all these assertion messages. Not once have they been useful to me when debugging a test. I would argue that 99% of them don't add any additional information that isn't obvious when looking at the assert itself. They also are usually just making the assertions longer and harder to read. Even worse, when I started hacking on LLDB they were often even suppressing the *actual error* which made it impossible to debug a test on a build bot (but that is thankfully fixed these days). And finally the fact that they can be worded in these two different ways just ends up causing confusion.

[teemperor]

FWIW, I actually think this patch itself is perfectly fine. As these messages are only displayed when an assert failed they should be worded with the assumption that the assert did indeed fail. It would be nice to reword all the other messages around this change to also follow this style, but I'm not a fan of signing up others for tedious work and this is a step in the right direction, so LGTM.

[labath]

Though these extra messages are often superfluous in general, I don't think that's the case for this particular message. This macro is only used in the expect and match checks. These are the most common way of asserting something right now, so I think it's reasonable to spend some effort to make the errors it produces clear and actionable. The current state is pretty far from ideal.

What I would do here is:

• remove the "False is not True" part from the message. This can be done by using the fail method to well.. fail the test instead of assertTrue/False.
• provide a good error message to the fail method. It should include the command being run, the actual output, and also list any expectations that failed. Maybe something like this (slightly inspired by the gtest assertion format):

Command: frame variable foo
Actual output:
(int) foo = 47
Failed because:
- output does not start with "(float)"
- output does not contain "47.0"

The functions already track most of this output, but they only print it out in the "trace" mode, so this would only be a matter of reorganizing the code so that it makes it into the assertion message too.

[DavidSpickett]

Ok to land this as is just to remove some confusion?

I agree it could be much improved, I'll look into that.

[teemperor]

That's true, EXP_MSG is not used like the other messages so the only problem with it is the confusing error message.

IMHO this is still good to land as it improves the error message. If you want to implement Pavel's much nicer error message in a follow-up patch, that would be very much appreciated!

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

 
 def COMPLETION_MSG(str_before, str_after, completions):
     '''A generic message generator for the completion mechanism.'''
     return ("'%s' successfully completes to '%s', but completions were:\n%s"
            % (str_before, str_after, "\n".join(completions)))
 
 
 def EXP_MSG(str, actual, exe):
-    '''A generic "'%s' returns expected result" message generator if exe.
-    Otherwise, it generates "'%s' matches expected result" message.'''
+    '''A generic "'%s' returned unexpected result" message generator if exe.
+    Otherwise, it generates "'%s' does not match expected result" message.'''
 
-    return "'%s' %s expected result, got '%s'" % (
-        str, 'returns' if exe else 'matches', actual.strip())
+    return "'%s' %s result, got '%s'" % (
+        str, 'returned unexpected' if exe else 'does not match expected', actual.strip())
 
 
 def SETTING_MSG(setting):
     '''A generic "Value of setting '%s' is correct" message generator.'''
     return "Value of setting '%s' is correct" % setting
 
 
 def line_number(filename, string_to_match):