Page MenuHomePhabricator

Build each testcase variant in its own subdirectory and remove the srcdir lock file

Authored by aprantl on Jan 31 2018, 1:51 PM.



This patch creates a <test>.dwarf, <test>.dwo, etc., build directory for each testcase variant.
Most importantly, this eliminates the need for the per-test lock file in the source directory.

Tests that are marked as NO_DEBUG_INFO_TESTCASE and build with buildDefault() are built in a <test>.default build directory.

The implementation changes self.mydir to a tuple (test_subdir, test_name). I chose not to rename the variable as to not break anyone's out-of-tree testcases any more than necessary.

It would be awesome if someone could test this on Windows / Linux / Android again.

Event Timeline

aprantl created this revision.Jan 31 2018, 1:51 PM
xiaobai added inline comments.
39–40 ↗(On Diff #132267)

Instead of making a temporary variable src_path that gets used once in each instance, why not just insert self.getBuildArtifact(_SRC_FILE) directly into lldbutil.run_break_set_by_file_and_line()?

25–26 ↗(On Diff #132267)

You should be able to insert the call to getBuildArtifact() into the construction of d, since exe is used nowhere else.

40 ↗(On Diff #132267)

Is this d used?

96–98 ↗(On Diff #132267)

Good opportunity to move away from the old style of formatting strings to a newer style? It doesn't make a huge difference, I just think it'd be nice to do. :)
return "-N dwarf {}".format(testdir)
return "-N dwarf {}".format(testdir)

This is supported in Python 2.7, but I'm not sure if we can assume that version or greater to be present.

533–539 ↗(On Diff #132888)

Might I suggest this?

(rel_test_path, extension) = os.path.splitext(rel_prefix)
if extension not in [".py", ".pyc"]:
    raise Exception("test_file is not a python file")
return rel_test_path

Feels cleaner to me, at least. :)

706 ↗(On Diff #132888)

stem is unused, you could make this a _.

711 ↗(On Diff #132888)

subdir and stem are kind of confusing to me. It looks like you used test_subdir below, so it might be good to use that here too, I think. Also, I think the name test_name is less confusing than test_stem. What do you think?

712 ↗(On Diff #132888)

I recommend if variant is None instead of if not variant

63 ↗(On Diff #132267)

I find test_stem kind of confusing. How do you feel about test_name?

Oh, FWIW I tested this on Linux (specifically CentOS) and the test result summary showed no difference in Success/Failure rate.

labath added a comment.Feb 1 2018, 2:20 AM

I am not sure this actually creates enough separation. Plenty of tests have more then one test method per file:
class FooTestCase(TestBase):
  def test_bar(self):
  def test_baz(self):

If I understand correctly, you patch will run both test_bar and test_baz in the same build folder (TestFoo.default). Shouldn't we separate those as well ? (e.g. TestFoo.test_bar, TestFoo.test_baz) ?

The good thing about this is that you don't need to worry about debug info variants in this code at all. You just take the test name, and the debug info will be encoded in there (if I drop the NO_DEBUG_INFO_TESTCASE, the metaclass will "magically" create test methods "test_bar_dwarf", "test_bar_dwo", ...)

(Technically that still leaves the possibility that a single file will have two classes, and each of them will have the same method name, but no test currently does that, so we can just disallow that).

96–98 ↗(On Diff #132267)

2.7 is definitely fine, and we use {} format in plently of places already. (But I don't have any beef with the %s format either).

aprantl updated this revision to Diff 132417.Feb 1 2018, 9:46 AM

Address review feedback from Alex.

I am not sure this actually creates enough separation.

That's a good point. If I manage to extract the testname somehow via Python reflection magic, I could also get rid of the unintuitive self.mydir tuple. I'll give that a try.

39–40 ↗(On Diff #132267)

While most tests have their sources in the source directory, some use generated source file, which are located in the build directory, so that is unfortunately not an option.

96–98 ↗(On Diff #132267)

In this case I think this should just be return "-N dsym " + testdir

labath added inline comments.Feb 1 2018, 9:54 AM
96–98 ↗(On Diff #132267)


85 ↗(On Diff #132417)

I think self.testMethodName is what you're looking for. (There also seems to be a self._testMethodName but I'm not sure what's the difference between the two.)

This comment was removed by aprantl.
aprantl updated this revision to Diff 132433.Feb 1 2018, 11:13 AM

Update the diff with Pavel's awesome suggestion of using self.testMethodName.

Note that that I haven't fixed all the resulting test error yet, I'm working through those now.

@jingham wrote:

Now that we aren't mixing variants, would it be possible to have a test class claim that all the tests use the same binary file? At present to get self-contained tests you often need to do roughly the same thing many times, on the same binary. You only need to build it once per variant in that case. Of course, not all tests would behave this way, some play tricks with the binary products mid-test. But I bet the majority of test classes with more than one test method just build exactly the same thing as all the other tests, and don't modify it.

Yes, but I would like to keep this for a separate patch, since it is nontrivial:

  • For NO_DEBUG_INFO_TESTCASEs this is as easy as invoking buildDefault() in self.setUp().
  • For tests with variants, we first would need to define a method setUpVariant() that behaves like setUp() in that is invoked for each variant but before any tests are executed.
aprantl updated this revision to Diff 132456.Feb 1 2018, 12:46 PM

Fix broken testcases.

labath added inline comments.Feb 1 2018, 12:54 PM
26 ↗(On Diff #132456)

I'm confused by these changes. I was under the impression that setUp() runs before each test method (and hence this should be NFC). Can you explain the reasoning behind this?

aprantl added inline comments.Feb 1 2018, 1:00 PM
26 ↗(On Diff #132456)

(see also my previous comment)
setUp runs before everything else, but it runs once per testcase and self.testMethodName is not initialized yet. Therefore we can only run in self.setUp() in NO_DEBUG_INFO_TESTCASEs.

labath added inline comments.Feb 2 2018, 2:37 AM
26 ↗(On Diff #132456)

I'm not sure where you got the idea that testMethodName is not initialized. It's initialized as one of the first things in Base.setUp ( This also demonstrates that the setUp method is run once *per test function* (as otherwise testMethodName would make no sense; also check out third_party/Python/module/unittest2/unittest2/ to see how tests are run).

The problem I think you are having is that self.debug_info is not initialized. I was curious at hard it would be to make this work, so I played around with it a bit and the result was D42836.

It's not that I think this change is that bad (although I would prefer if we could keep doing these things in setUp), but I think this could also help make happen the ideas about building binaries once per test case (class) -- the way I'd do that is that I would enumerate all variants that are needed to build in setUpClass (and build them); then the actual test could fetch the corresponding binary according to the variant.

721–728 ↗(On Diff #132888)

Can we use the (new) lldbutil.mkdir_p here? If there's some circular dependency, maybe we could move the function somewhere else?

1506–1507 ↗(On Diff #132888)

Why was this necessary? Could we either always pass the name by argument, or always fetch it from self? Maybe it will be possible after D42836?

JDevlieghere added inline comments.Feb 2 2018, 3:00 AM
96–98 ↗(On Diff #132267)

+1. I agree with Pavel that we should adopt the new formatters. Let's be idiomatic :-)

728 ↗(On Diff #132888)

"{} is not a directory".format(path)

aprantl updated this revision to Diff 132875.Feb 5 2018, 11:57 AM

Updated to query self.getDebugInfo() in getBuildDir().

aprantl updated this revision to Diff 132888.Feb 5 2018, 1:39 PM
aprantl marked 17 inline comments as done.

Cleanup and address outstanding review feedback.

labath accepted this revision.Feb 6 2018, 2:20 AM

This looks much better. Thanks.

This revision is now accepted and ready to land.Feb 6 2018, 2:20 AM
This revision was automatically updated to reflect the committed changes.