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

[aprantl]

Summary

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.

[xiaobai]

packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py

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()?

packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_on_vectors/TestValueOfVectorVariable.py

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?

packages/Python/lldbsuite/test/lldbinline.py

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.

packages/Python/lldbsuite/test/lldbtest.py

533–539

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

stem is unused, you could make this a _.

711

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

I recommend if variant is None instead of if not variant

packages/Python/lldbsuite/test/plugins/builder_base.py

63 ↗

(On Diff #132267)

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

[xiaobai]

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

[labath]

I am not sure this actually creates enough separation. Plenty of tests have more then one test method per file:

TestFoo.py:
class FooTestCase(TestBase):
  NO_DEBUG_INFO_TESTCASE = True
  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).

packages/Python/lldbsuite/test/lldbinline.py

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]

Address review feedback from Alex.

[aprantl]

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.

packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py

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.

packages/Python/lldbsuite/test/lldbinline.py

96–98 ↗

(On Diff #132267)

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

[labath]

packages/Python/lldbsuite/test/lldbinline.py

96–98 ↗

(On Diff #132267)

:D

packages/Python/lldbsuite/test/python_api/hello_world/TestHelloWorld.py

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.)

[aprantl]

This comment was removed by aprantl.

[aprantl]

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.

[aprantl]

@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]

Fix broken testcases.

[labath]

packages/Python/lldbsuite/test/api/listeners/TestListener.py

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]

packages/Python/lldbsuite/test/api/listeners/TestListener.py

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 self.build() in self.setUp() in NO_DEBUG_INFO_TESTCASEs.

[labath]

packages/Python/lldbsuite/test/api/listeners/TestListener.py

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 (lldbtest.py:776). 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/case.py:379 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.

packages/Python/lldbsuite/test/lldbtest.py

721–728

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

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]

packages/Python/lldbsuite/test/lldbinline.py

96–98 ↗

(On Diff #132267)

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

packages/Python/lldbsuite/test/lldbtest.py

728

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

[aprantl]

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

[aprantl]

Cleanup and address outstanding review feedback.

[labath]

This looks much better. Thanks.

packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py

 @skipIfWindows  # Windows doesn't have dlopen and friends, dynamic libraries work differently
 class LoadUnloadTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
+        self.setup_test()
+        # Invoke the default build rule.
+        self.build()
         # Find the line number to break for main.cpp.
         self.line = line_number(
             'main.cpp',
             '// Set break point at this line for test_lldb_process_load_and_unload_commands().')
         self.line_d_function = line_number(
             'd.cpp', '// Find this line number within d_dunction().')
 
     def setup_test(self):
class LoadUnloadTestCase(TestBase):
     # libloadunload_d.so does not appear in the image list because executable
     # dependencies are resolved relative to the debuggers PWD. Bug?
     @expectedFailureAll(oslist=["linux"])
     @skipIfFreeBSD  # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support
     @not_remote_testsuite_ready
     @skipIfWindows  # Windows doesn't have dlopen and friends, dynamic libraries work differently
     def test_modules_search_paths(self):
         """Test target modules list after loading a different copy of the library libd.dylib, and verifies that it works with 'target modules search-paths add'."""
-        self.setup_test()
-        # Invoke the default build rule.
-        self.build()
-
         if self.platformIsDarwin():
             dylibName = 'libloadunload_d.dylib'
         else:
             dylibName = 'libloadunload_d.so'
 
         # The directory with the dynamic library we did not link to.
         new_dir = os.path.join(self.getBuildDir(), "hidden")
 
class LoadUnloadTestCase(TestBase):
     # libloadunload_d.so does not appear in the image list because executable
     # dependencies are resolved relative to the debuggers PWD. Bug?
     @expectedFailureAll(oslist=["linux"])
     @skipIfFreeBSD  # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support
     @expectedFailureAndroid  # wrong source file shows up for hidden library
     @skipIfWindows  # Windows doesn't have dlopen and friends, dynamic libraries work differently
     def test_dyld_library_path(self):
         """Test (DY)LD_LIBRARY_PATH after moving libd.dylib, which defines d_function, somewhere else."""
-        self.setup_test()
-        # Invoke the default build rule.
-        self.build()
         self.copy_shlibs_to_remote(hidden_dir=True)
 
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Shut off ANSI color usage so we don't get ANSI escape sequences
         # mixed in with stop locations.
         self.dbg.SetUseColor(False)
class LoadUnloadTestCase(TestBase):
     @expectedFailureAll(
         bugnumber="llvm.org/pr25805",
         hostoslist=["windows"],
         triple='.*-android')
     @skipIfFreeBSD  # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support
     @skipIfWindows  # Windows doesn't have dlopen and friends, dynamic libraries work differently
     def test_lldb_process_load_and_unload_commands(self):
         """Test that lldb process load/unload command work correctly."""
-        self.setup_test()
-        # Invoke the default build rule.
-        self.build()
         self.copy_shlibs_to_remote()
 
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Break at main.cpp before the call to dlopen().
         # Use lldb's process load command to load the dylib, instead.
 
def test_lldb_process_load_and_unload_commands(self):
                 "Unloading .* with index %s.*ok" %
                 index])
 
         self.runCmd("process continue")
 
     @skipIfFreeBSD  # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support
     def test_load_unload(self):
         """Test breakpoint by name works correctly with dlopen'ing."""
-        self.setup_test()
-        # Invoke the default build rule.
-        self.build()
         self.copy_shlibs_to_remote()
 
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Break by function name a_function (not yet loaded).
         lldbutil.run_break_set_by_symbol(
             self, "a_function", num_expected_locations=0)
def test_load_unload(self):
         # The breakpoint should have a hit count of 2.
         self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
                     substrs=[' resolved, hit count = 2'])
 
     @skipIfFreeBSD  # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support
     @skipIfWindows  # Windows doesn't have dlopen and friends, dynamic libraries work differently
     def test_step_over_load(self):
         """Test stepping over code that loads a shared library works correctly."""
-        self.setup_test()
-        # Invoke the default build rule.
-        self.build()
         self.copy_shlibs_to_remote()
 
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Break by function name a_function (not yet loaded).
         lldbutil.run_break_set_by_file_and_line(
             self, "main.cpp", self.line, num_expected_locations=1, loc_exact=True)
class LoadUnloadTestCase(TestBase):
 
     # We can't find a breakpoint location for d_init before launching because
     # executable dependencies are resolved relative to the debuggers PWD. Bug?
     @expectedFailureAll(oslist=["linux"])
     @skipIfFreeBSD  # llvm.org/pr14424 - missing FreeBSD Makefiles/testcase support
     @skipIfWindows  # Windows doesn't have dlopen and friends, dynamic libraries work differently
     def test_static_init_during_load(self):
         """Test that we can set breakpoints correctly in static initializers"""
-        self.setup_test()
-        self.build()
         self.copy_shlibs_to_remote()
 
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         a_init_bp_num = lldbutil.run_break_set_by_symbol(
             self, "a_init", num_expected_locations=0)
         b_init_bp_num = lldbutil.run_break_set_by_symbol(

packages/Python/lldbsuite/test/lldbtest.py

class Base(unittest2.TestCase):
     def compute_mydir(test_file):
         '''Subclasses should call this function to correctly calculate the
            required "mydir" attribute as follows:
 
             mydir = TestBase.compute_mydir(__file__)
         '''
         # /abs/path/to/packages/group/subdir/mytest.py -> group/subdir
         rel_prefix = test_file[len(os.environ["LLDB_TEST"]) + 1:]
         return os.path.dirname(rel_prefix)
 
     def TraceOn(self):
         """Returns True if we are in trace mode (tracing detailed test execution)."""
         return traceAlways
 
     @classmethod
     def setUpClass(cls):
         """
         Python unittest framework class setup fixture.
         Do current directory manipulation.
         """
         # Fail fast if 'mydir' attribute is not overridden.
         if not cls.mydir or len(cls.mydir) == 0:
             raise Exception("Subclasses must override the 'mydir' attribute.")
def setPlatformWorkingDir(self):
             # list of files we have to remove during test runs.
             shell_cmd = lldb.SBPlatformShellCommand(
                 "rm %s/*" % remote_test_dir)
             lldb.remote_platform.Run(shell_cmd)
         self.addTearDownHook(clean_working_directory)
 
     def getSourceDir(self):
         """Return the full path to the current test."""
         return os.path.join(os.environ["LLDB_TEST"], self.mydir)
 
     def getBuildDir(self):
         """Return the full path to the current test."""
         variant = self.getDebugInfo()
         if variant is None:
             variant = 'default'
         return os.path.join(os.environ["LLDB_BUILD"], self.mydir,
                             self.testMethodName)
     
      
     def makeBuildDir(self):
         """Create the test-specific working directory."""
         # See also dotest.py which sets up ${LLDB_BUILD}.
         lldbutil.mkdir_p(self.getBuildDir())
  
     def getBuildArtifact(self, name="a.out"):
         """Return absolute path to an artifact in the test's build directory."""
         return os.path.join(self.getBuildDir(), name)
  
     def getSourcePath(self, name):
         """Return absolute path to a file in the test's source directory."""
         return os.path.join(self.getSourceDir(), name)
 
     def setUp(self):
         """Fixture for unittest test case setup.
 
         It works with the test driver to conditionally skip tests and does other
         initializations."""
         #import traceback
         # traceback.print_stack()
def buildProgram(self, sources, exe_name):
              'EXE': exe_name}
         self.buildDefault(dictionary=d)
 
     def buildDefault(
             self,
             architecture=None,
             compiler=None,
             dictionary=None,
-            clean=True,
-            testdir=None,
-            testname=None):
+            clean=True):
         """Platform specific way to build the default binaries."""
-        if not testdir:
-            testdir = self.mydir
-        if not testname:
-            testname = self.testMethodName
         if self.getDebugInfo():
             raise Exception("buildDefault tests must set NO_DEBUG_INFO_TESTCASE")
         module = builder_module()
         self.makeBuildDir()
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildDefault(self, architecture, compiler,
                                    dictionary, clean, testdir, testname):
             raise Exception("Don't know how to build default binary")
 
     def buildDsym(
             self,
             architecture=None,
             compiler=None,
             dictionary=None,
-            clean=True,
-            testdir=None,
-            testname=None):
+            clean=True):
         """Platform specific way to build binaries with dsym info."""
-        if not testdir:
-            testdir = self.mydir
-        if not testname:
-            testname = self.testMethodName
         if self.getDebugInfo() != "dsym":
             raise Exception("NO_DEBUG_INFO_TESTCASE must build with buildDefault")
 
         module = builder_module()
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildDsym(self, architecture, compiler,
                                 dictionary, clean, testdir, testname):
             raise Exception("Don't know how to build binary with dsym")
 
     def buildDwarf(
             self,
             architecture=None,
             compiler=None,
             dictionary=None,
-            clean=True,
-            testdir=None,
-            testname=None):
+            clean=True):
         """Platform specific way to build binaries with dwarf maps."""
-        if not testdir:
-            testdir = self.mydir
-        if not testname:
-            testname = self.testMethodName
         if self.getDebugInfo() != "dwarf":
             raise Exception("NO_DEBUG_INFO_TESTCASE must build with buildDefault")
 
         module = builder_module()
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildDwarf(self, architecture, compiler,
                                    dictionary, clean, testdir, testname):
             raise Exception("Don't know how to build binary with dwarf")
 
     def buildDwo(
             self,
             architecture=None,
             compiler=None,
             dictionary=None,
-            clean=True,
-            testdir=None,
-            testname=None):
+            clean=True):
         """Platform specific way to build binaries with dwarf maps."""
-        if not testdir:
-            testdir = self.mydir
-        if not testname:
-            testname = self.testMethodName
         if self.getDebugInfo() != "dwo":
             raise Exception("NO_DEBUG_INFO_TESTCASE must build with buildDefault")
 
         module = builder_module()
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildDwo(self, architecture, compiler,
                                    dictionary, clean, testdir, testname):
             raise Exception("Don't know how to build binary with dwo")
 
     def buildGModules(
             self,
             architecture=None,
             compiler=None,
             dictionary=None,
-            clean=True,
-            testdir=None,
-            testname=None):
+            clean=True):
         """Platform specific way to build binaries with gmodules info."""
-        if not testdir:
-            testdir = self.mydir
-        if not testname:
-            testname = self.testMethodName
         if self.getDebugInfo() != "gmodules":
             raise Exception("NO_DEBUG_INFO_TESTCASE must build with buildDefault")
 
         module = builder_module()
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if not module.buildGModules(self, architecture, compiler,
                                     dictionary, clean, testdir, testname):
             raise Exception("Don't know how to build binary with gmodules")
def build(
             clean=True):
         """Platform specific way to build the default binaries."""
         module = builder_module()
         self.makeBuildDir()
 
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if self.getDebugInfo() is None:
             return self.buildDefault(architecture, compiler, dictionary,
-                                     clean, self.mydir)
+                                     clean)
         elif self.getDebugInfo() == "dsym":
-            return self.buildDsym(architecture, compiler, dictionary,
-                                  clean, self.mydir)
+            return self.buildDsym(architecture, compiler, dictionary, clean)
         elif self.getDebugInfo() == "dwarf":
-            return self.buildDwarf(architecture, compiler, dictionary,
-                                   clean, self.mydir)
+            return self.buildDwarf(architecture, compiler, dictionary, clean)
         elif self.getDebugInfo() == "dwo":
             return self.buildDwo(architecture, compiler, dictionary,
-                                 clean, self.mydir)
+                                 clean)
         elif self.getDebugInfo() == "gmodules":
             return self.buildGModules(architecture, compiler, dictionary,
-                                      clean, self.mydir)
+                                      clean)
         else:
             self.fail("Can't build for debug info: %s" % self.getDebugInfo())
 
     def run_platform_command(self, cmd):
         platform = self.dbg.GetSelectedPlatform()
         shell_command = lldb.SBPlatformShellCommand(cmd)
         err = platform.Run(shell_command)
         return (err, shell_command.GetStatus(), shell_command.GetOutput())