Page MenuHomePhabricator

[lldb] Correct wording of EXP_MSG
ClosedPublic

Authored by DavidSpickett on Aug 26 2020, 2:03 AM.

Details

Reviewers
teemperor
Group Reviewers
Restricted Project
Commits
rG9ad5d37fd917: [lldb] Correct wording of EXP_MSG
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>'

Diff Detail

Event Timeline

DavidSpickett created this revision.Aug 26 2020, 2:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 2:03 AM
DavidSpickett requested review of this revision.Aug 26 2020, 2:03 AM
teemperor added a reviewer: Restricted Project.Aug 26 2020, 2:16 AM

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 accepted this revision.Aug 26 2020, 2:27 AM

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.

This revision is now accepted and ready to land.Aug 26 2020, 2:27 AM
labath added a subscriber: labath.Aug 26 2020, 2:33 AM

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.

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

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

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!

This revision was automatically updated to reflect the committed changes.