Page MenuHomePhabricator

[lldb] Improve test failure reporting for expect()
ClosedPublic

Authored by DavidSpickett on Fri, Aug 28, 9:08 AM.

Details

Summary

This updates the errors reported by expect()
to something like:

Ran command:
"help"

Got output:
Debugger commands:
<...>

Expecting start string: "Debugger commands:" (was found)
Expecting end string: "foo" (was not found)

(see added tests for more examples)

This shows the user exactly what was run,
what checks passed and which failed. Along with
whether that check was supposed to pass.
(including what regex patterns matched)

These lines are also output to the test
trace file, whether the test passes or not.

Note that expect() will still fail at the first failed
check, in line with previous behaviour.

Also I have flipped the wording of the assert
message functions (.*_MSG) to describe failures
not successes. This makes more sense as they are
only shown on assert failures.

Diff Detail

Event Timeline

DavidSpickett created this revision.Fri, Aug 28, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Aug 28, 9:08 AM
DavidSpickett requested review of this revision.Fri, Aug 28, 9:08 AM
DavidSpickett added reviewers: teemperor, labath.

I realise the test is a bit meta so if there might be a better place to put it. It does at least run successfully from the API test dir.

DavidSpickett added inline comments.Fri, Aug 28, 9:13 AM
lldb/packages/Python/lldbsuite/test/lldbtest.py
2459

I've added speech marks around things that are probably on the shorter side. I've found it helps narrow down issues with empty strings or leading/trailing whitespace.

DavidSpickett edited the summary of this revision. (Show Details)Fri, Aug 28, 9:20 AM
labath added a comment.Wed, Sep 2, 7:17 AM

I like this. And a big thank-you for writing the tests. We don't usually bother to write self-tests for the test infrastructure, but we definitely should be doing that. The place is slightly weird, but I think it will do for now -- it's easy to move this around if we find a better place for it.

lldb/test/API/assert_messages_test/TestAssertMessages.py
38–39

These two calls always come in pairs, right? Might be nice to make this a single function which gets both the command to run, and the error message it is expected to fail with.

Combined two functions into one single run command
and check message function.

With some odd kwargs, but I think it helps to be able
to put the expected lines on the end.

DavidSpickett marked an inline comment as done.Wed, Sep 2, 8:13 AM
labath added a comment.Wed, Sep 2, 8:24 AM

Awesome.

Combined two functions into one single run command
and check message function.

With some odd kwargs, but I think it helps to be able
to put the expected lines on the end.

I think that's ok. Another option might be to take the expect argument as a dictionary instead of **kwargs. Something like:

    def assert_expect_fails_with(self, cmd, expect_args, failure_substrs):
        try:
            # This expect should fail
            self.expect(cmd, **expect_args)
        except AssertionError as e:
            # Then check message from previous expect
            self.expect(str(e), exe=False, substrs=failure_substrs)

...
assert_expect_fails_with("foo", dict(endstr="foo"), "...")
lldb/test/API/assert_messages_test/TestAssertMessages.py
83

I'd consider moving the dedent and the listification ([]) operations into the assert_expect_fails_with function.

  • Use settings command so we don't have to substr help's output
  • Move list and dedent calls into assert function
DavidSpickett marked an inline comment as done.Thu, Sep 3, 2:16 AM
labath accepted this revision.Thu, Sep 3, 4:42 AM
This revision is now accepted and ready to land.Thu, Sep 3, 4:42 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!