This is an archive of the discontinued LLVM Phabricator instance.

[lldb/test] Catch invalid calls to expect()

Authored by kastiglione on Oct 3 2020, 11:03 PM.



Add preconditions to TestBase.expect() that catch semantically invalid calls
that happen to succeed anyway. This also fixes the broken callsites caught by
these checks.

This prevents the following incorrect calls:

  1. self.expect("lldb command", "some substr")
  2. self.expect("lldb command", "assert message", "some substr")
  3. self.expect("lldb command", substrs="some substr")
  4. self.expect("lldb command", patterns="some pattern")

Diff Detail

Event Timeline

kastiglione created this revision.Oct 3 2020, 11:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2020, 11:03 PM
kastiglione requested review of this revision.Oct 3 2020, 11:03 PM
labath added a subscriber: labath.Oct 5 2020, 12:09 AM
labath added inline comments.

assert isinstance(substrs, six.string_types), "substrs must be a collection of strings" ?

teemperor accepted this revision.Oct 5 2020, 2:39 AM

Thanks for fixing all of this! Beside the assertion this LGTM.


Or we could do assertNotIsInstance for a better error message that should show the actual type.

This revision is now accepted and ready to land.Oct 5 2020, 2:39 AM

Does it make sense to add a few tests for this to lldb/test/API/assert_messages_test/ (this is only checking expect() messages, despite the generic name)

Oh wow — that paid off!


I'm not not sure I understand this change?

Resyntax the isinstance asserts; Add expect() tests


It turns out that in practice assertNotIsInstance isn't a good match for this case because six.string_types is a tuple, and the formatted string becomes "… is an instance of (<class 'str'>,)".

kastiglione edited the summary of this revision. (Show Details)Oct 5 2020, 10:05 AM
kastiglione added inline comments.

The second parameter, msg, is only valid if there are other "matchers", like substrs or startstr. If not, then the msg isn't used and so shouldn't need to be passed. By preventing a msg here, it allows the precondition to distinguish between cases where the msg is valid, and where it's not.

"missing a matcher" -> "missing a matcher argument"

This revision was automatically updated to reflect the committed changes.
shafik added a subscriber: shafik.Oct 6 2020, 11:06 AM

I am happy this was not hiding any regressions.