Page MenuHomePhabricator

Wrap all references to build artifacts in the LLDB testsuite in TestBase::getBuildArtifact()
ClosedPublic

Authored by aprantl on Jan 18 2018, 7:14 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Jan 18 2018, 7:14 PM
labath added a subscriber: labath.Jan 19 2018, 2:19 AM

os.path.join(os.getcwd(), os.environ["LLDB_BUILD"], self.mydir, "a.out") is getting quite long. I think we should make a utility function out of that.
Maybe self.getBuildArtifact("a.out") ? We could even make "a.out" the default value of the argument...

packages/Python/lldbsuite/test/android/platform/TestDefaultCacheLineSize.py
22 ↗(On Diff #130544)

Do we want os.environ["LLDB_BUILD"] to be interpreted relative to os.getcwd(), what's the value of os.getcwd() at the point when this is executed?

aprantl updated this revision to Diff 130664.Jan 19 2018, 11:14 AM
aprantl retitled this revision from WIP: mechanical LLDB testsuite changes to Wrap all references to build artifacts in the LLDB testsuite in TestBase::getBuildArtifact().
aprantl edited the summary of this revision. (Show Details)
aprantl added a subscriber: lldb-commits.

I have reworked this into an NFC commit based on feedback from Pavel.

We have a lot of ugly boilerplate in the testsuite. I added:

(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
                           "Set a breakpoint here", self.main_source_file)

Because that's what a lot of tests did. I converted a handful of tests and then ran out of time.

If we're going to touch how all the tests get started with their executable it would be nice to see how many of them can be easily converted to this interface. I can also add a file & line version of this, though in fact most tests that use file & line breakpoints actually use a source match to get a line number and then feed the line to lldb, so they could easily be converted to this.

Then all the business of locating the binary could be centralized.

I guess it's a little late for this comment, however...

vsk added a subscriber: vsk.Jan 19 2018, 11:38 AM

We have a lot of ugly boilerplate in the testsuite. I added:

(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
                           "Set a breakpoint here", self.main_source_file)

Because that's what a lot of tests did. I converted a handful of tests and then ran out of time.

If we're going to touch how all the tests get started with their executable it would be nice to see how many of them can be easily converted to this interface. I can also add a file & line version of this, though in fact most tests that use file & line breakpoints actually use a source match to get a line number and then feed the line to lldb, so they could easily be converted to this.

Then all the business of locating the binary could be centralized.

I guess it's a little late for this comment, however...

I think lldbutil.run_to_source_breakpoint is definitely worth standardizing on, but as it stands, this patch lgtm as an initial step.

labath accepted this revision.Jan 19 2018, 11:40 AM

Awesome. This will make flipping the out-of-tree switch much easier.

Making more tests use lldbutil.run_to_source_breakpoint would be nice, but I suspect that cannot be done with a shellscript, so I wouldn't tie that to this.

This revision is now accepted and ready to land.Jan 19 2018, 11:40 AM
davide accepted this revision.Jan 19 2018, 11:45 AM
davide added a subscriber: davide.

lgtm

jingham accepted this revision.Jan 19 2018, 11:47 AM

I didn't intend to block this patch, just to point out that this was really work you shouldn't have had to do, and that as we touch these files we should clean them up so next time we don't have to. It will also make the test files easier to read.

I didn't intend to block this patch, just to point out that this was really work you shouldn't have had to do, and that as we touch these files we should clean them up so next time we don't have to. It will also make the test files easier to read.

I skimmed through every test*.py file while preparing this patch and noticed that there seem to be 3-4 templates that many tests are derived from. Refactoring all the common code would be very good thing to do. I will try to get some of that done in subsequent patches.

aprantl updated this revision to Diff 130673.Jan 19 2018, 11:57 AM

Couple more bugfixes.
ninja check-lldb comes out clean for me now on Darwin.

This revision was automatically updated to reflect the committed changes.

I didn't intend to block this patch, just to point out that this was really work you shouldn't have had to do, and that as we touch these files we should clean them up so next time we don't have to. It will also make the test files easier to read.

While we're on that topic, I would actually propose going one step further than lldbutil.run_to_source_breakpoint. I would propose creating a new test class (StandardLaunchTestCase ?) . This class would assume a certain executable name (a.out), a certain source file name (main.cpp, but this can be taken from a class property, so it can be overriden to main.mm for objc tests) and a certain breakpoint string.

That way, it can do the "build, set breakpoint, launch, validate stop reason" dance without any user input in the setUp() method, and the test can start with the process primed and ready and just focus on the thing it's testing