Page MenuHomePhabricator

[lldb/Test] Introduce "assertSuccess"
ClosedPublic

Authored by labath on Jun 29 2020, 5:23 AM.

Details

Summary

A lot of our tests do 'self.assertTrue(error.Success()'. The problem
with that is that when this fails, it produces a completely useless
error message (False is not True) and the most important piece of
information -- the actual error message -- is completely hidden.

Sometimes we mitigate that by including the error message in the "msg"
argument, but this has two additional problems:

  • as the msg argument is evaluated unconditionally, one needs to be careful to not trigger an exception when the operation was actually successful.
  • it requires more typing, which means we often don't do it

assertSuccess solves these problems by taking the entire SBError object
as an argument. If the operation was unsuccessful, it can format a
reasonable error message itself. The function still accepts a "msg"
argument, which can include any additional context, but this context now
does not need to include the error message.

To demonstrate usage, I replace a number of existing assertTrue
assertions with the new function. As this process is not easily
automatable, I have just manually updated a representative sample. In
some cases, I did not update the code to use assertSuccess, but I went
for even higher-level assertion apis (runCmd, expect_expr), as these are
even shorter, and can produce even better failure messages.

Event Timeline

labath created this revision.Jun 29 2020, 5:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 5:23 AM
Herald added a subscriber: arphaman. · View Herald Transcript
JDevlieghere accepted this revision.Jun 29 2020, 9:43 AM

Looks like a great improvement!

At some point we should document our custom methods in https://lldb.llvm.org/resources/test.html to make them more discoverable, but that's orthogonal to this patch.

lldb/packages/Python/lldbsuite/test/lldbtest.py
2537 ↗(On Diff #274070)

I'm not actually sure this works for custom types, but should we add an assert isinstance(obj, lldb.SBError) to avoid misuse?

This revision is now accepted and ready to land.Jun 29 2020, 9:43 AM
teemperor accepted this revision.Jun 30 2020, 3:54 AM

LGTM, thanks!

lldb/packages/Python/lldbsuite/test/lldbtest.py
2537 ↗(On Diff #274070)

I actually also wondered if obj can be anything else in LLDB's SB API. I guess it's Pythonic to allow anything that fits that signature but some one-line documentaiton that this intended is for SBError would be nice.

labath marked 3 inline comments as done.Jun 30 2020, 4:35 AM
labath added inline comments.
lldb/packages/Python/lldbsuite/test/lldbtest.py
2537 ↗(On Diff #274070)

Yeah, duck-typing is very pythonic :P. At one point I actually had this special-case SBCommandReturnObject (It has Succeeded (:/) and GetError methods, but then I realized those are better handled by runCmd and friends.

I'll add some documentation about SBError.

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.