This is an archive of the discontinued LLVM Phabricator instance.

Allow use of self.filecheck in LLDB tests (c.f self.expect)
ClosedPublic

Authored by vsk on Aug 14 2018, 5:28 PM.

Details

Summary

Add a "filecheck" method to the LLDB test base. This allows test authors
to pattern match command output using FileCheck, making it possible to
write stricter tests than what self.expect allows.

For context (motivation, examples of stricter checking, etc), see the
lldb-dev thread: "Using FileCheck in lldb inline tests".

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Aug 14 2018, 5:28 PM
vsk updated this revision to Diff 162491.Aug 24 2018, 3:43 PM
vsk retitled this revision from WIP: Expose FileCheck-style testing within lldb inline tests to Allow use of self.filecheck in LLDB tests (c.f self.expect).
vsk edited the summary of this revision. (Show Details)
vsk added reviewers: teemperor, aprantl, zturner.
vsk added a project: Restricted Project.
vsk removed a subscriber: zturner.
teemperor requested changes to this revision.Aug 28 2018, 3:11 PM
teemperor added inline comments.
lldb/packages/Python/lldbsuite/test/configuration.py
194 ↗(On Diff #162491)

This is true if we build LLDB as part of LLVM, but this doesn't seem correct if LLDB is built against an existing LLVM (which is in general supported in LLDB's CMake if IIUC). I think the best way is to pass the binary directory for this to dotest and configure it in the lit.cfg.

204 ↗(On Diff #162491)

Windows itself can probably handle the fact that the .exe suffix is missing for FileCheck, but this assert will fail in this situation.

lldb/packages/Python/lldbsuite/test/lldbtest.py
51 ↗(On Diff #162491)

I think that list is supposed to be sorted alphabetically

2166 ↗(On Diff #162491)

Making a temporary file and opening it later by name is not really supported (see the NamedTemporaryFile docs). Could we maybe pass the output to FileCheck via stdin? That would also save us from the detour over the fs.

2181 ↗(On Diff #162491)

We should quote the paths in here. Or at least, the temporary file path should be quoted as it will contain spaces on some setups.

This revision now requires changes to proceed.Aug 28 2018, 3:11 PM
lldb/packages/Python/lldbsuite/test/configuration.py
194 ↗(On Diff #162491)

Rather than making the change here, as @teemperor pointed out, you should use the existing mechanisms we have in the CMake files. Look for LLDB_DEFAULT_TEST_DSYMUTIL to see how we handle this (and the extension too!) for dsymutil.

vsk updated this revision to Diff 165613.Sep 14 2018, 5:01 PM
vsk marked 6 inline comments as done.

Sorry for the delay, I was busy with other work. I think I've addressed the review feedback. PTAL.

stella.stamenova accepted this revision.Sep 17 2018, 9:57 AM

LGTM, but I'd wait till @teemperor approves as well.

JDevlieghere accepted this revision.Sep 18 2018, 12:27 AM

Thanks Vedant, this looks very useful!

teemperor accepted this revision.Sep 18 2018, 1:38 AM

LGTM minus one small detail (see inline comment). Thanks Vedant!

lldb/packages/Python/lldbsuite/test/lldbtest.py
51 ↗(On Diff #165613)

I think we don't need that import anymore.

This revision is now accepted and ready to land.Sep 18 2018, 1:38 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.