[lldb/Test] Ensure inline tests have a unique build directory

[JDevlieghere]

Summary

Inline tests have one method named 'test' which means that multiple inline tests in the same file end up sharing the same build directory per variant.

The output bellow illustrates that even though BasicEntryValues_GNU and BasicEntryValues_V5 have unique names, the test method for the dwarf variant is called test_dwarf in both cases.

PASS: LLDB (/Users/jonas/llvm/build-ra/bin/clang-11-x86_64) :: test_dwarf (lldbsuite.test.lldbtest.BasicEntryValues_GNU)
PASS: LLDB (/Users/jonas/llvm/build-ra/bin/clang-11-x86_64) :: test_gmodules (lldbsuite.test.lldbtest.BasicEntryValues_GNU)
PASS: LLDB (/Users/jonas/llvm/build-ra/bin/clang-11-x86_64) :: test_dsym (lldbsuite.test.lldbtest.BasicEntryValues_V5)
PASS: LLDB (/Users/jonas/llvm/build-ra/bin/clang-11-x86_64) :: test_dwarf (lldbsuite.test.lldbtest.BasicEntryValues_V5)
PASS: LLDB (/Users/jonas/llvm/build-ra/bin/clang-11-x86_64) :: test_gmodules (lldbsuite.test.lldbtest.BasicEntryValues_V5)

The 'inline_name' attribute ensures that the (unique) test name is used instead.

[JDevlieghere]

I discovered this because TestBasicEntryValues.py was failing with the reproducers. Because the paths to the binaries were the same, we'd end up with only the GNU variant of the binary in the VFS.

[vsk]

lldb/packages/Python/lldbsuite/test/lldbtest.py

1750 ↗

(On Diff #269695)

Is inline_suffix = attrs.get('__inline_name__') no good?

1778 ↗

(On Diff #269695)

Would it be all right to unconditionally set method_name = name + attrname + cat?

[labath]

I noticed this issue after committing the patch that caused it. I wasn't rushing to fix it because it did not seem like a big problem. But, of course it is a big problem for reproducers...

That said, I think the proposed fix is more complicated than needed and would not handle @no_debug_info_test tests (because the fixup happens in the code which does the magic test replication).

It should be sufficient to add this to the definition of InlineTest class:

def getBuildDirBasename(self):
    return self.__class__.__name__ + "." + self.testMethodName

That will result in paths like:

lldb-test-build.noindex/functionalities/param_entry_vals/basic_entry_values/BasicEntryValues_GNU.test_dwarf/a.out
lldb-test-build.noindex/functionalities/param_entry_vals/basic_entry_values/BasicEntryValues_V5.test_dwarf/a.out

Which is better than lldb-test-build.noindex/functionalities/param_entry_vals/basic_entry_values/lldbsuite.test.lldbtest.test_dwarf/a.out (status quo) and lldb-test-build.noindex/functionalities/param_entry_vals/basic_entry_values/lldbsuite.test.lldbtest.test_BasicEntryValues_V5_dwarf/a.out (what this patch would produce).

Btw, this problem isn't really specific to inline tests. The same problem could happen with regular tests if one defined two classes with identically named test methods in the same file. Which means we could try to fix this even more generally by making the default getBuildDirBasename implementation take the class name into account. However, multiple classes in a single file are fairly strange, and this solution would make the build dir paths even longer than they are now, so fixing it this way may not be worth it...

lldb/packages/Python/lldbsuite/test/lldbinline.py

214

__inline_name__ is not a good name according to https://www.python.org/dev/peps/pep-0008/:

__double_leading_and_trailing_underscore__: "magic" objects or attributes that live in user-controlled namespaces. E.g. __init__, __import__ or __file__. Never invent such names; only use them as documented.

[JDevlieghere]

Implement Pavel's approach

[JDevlieghere]

lldb/packages/Python/lldbsuite/test/lldbinline.py

214

I did it for consistency with __no_debug_info_test__. Based on that PEP we should rename it to __no_debug_info_test?

[labath]

lldb/packages/Python/lldbsuite/test/lldbinline.py

214

I think a single underscore should be enough. I am not sure the mangling magic invoked by a double underscore will work for this case:

Note 2: Name mangling can make certain uses, such as debugging and __getattr__(), less convenient.

The PEP is very cautious about (not) encouraging double underscores, and a single leading underscore already says this is private.

[JDevlieghere]

lldb/packages/Python/lldbsuite/test/lldbinline.py

214

Alright, I'll fix that in a follow-up commit. I just wasn't sure if we relied on it, but if we do we'll find out ;-)

lldb/packages/Python/lldbsuite/test/lldbinline.py

class CommandParser:
     def handle_breakpoint(self, test, breakpoint_id):
         for breakpoint in self.breakpoints:
             if breakpoint['breakpoint'].GetID() == breakpoint_id:
                 test.execute_user_command(breakpoint['command'])
                 return
 
 
 class InlineTest(TestBase):
+    # Overrides
+
+    def getBuildDirBasename(self):
+        return self.__class__.__name__ + "." + self.testMethodName
+
     # Internal implementation
 
     def BuildMakefile(self):
         makefilePath = self.getBuildArtifact("Makefile")
         if os.path.exists(makefilePath):
             return
 
         categories = {}
def MakeInlineTest(__file, __globals, decorators=None, name=None,
     if name is None:
         # Derive the test name from the current file name
         file_basename = os.path.basename(__file)
         name, _ = os.path.splitext(file_basename)
 
     test_func = ApplyDecoratorsToFunction(InlineTest._test, decorators)
     # Build the test case
     test_class = type(name, (InlineTest,), dict(test=test_func,
         name=name, _build_dict=build_dict))
 
     # Add the test case to the globals, and hide InlineTest
     __globals.update({name: test_class})
 
     # Keep track of the original test filename so we report it
     # correctly in test results.
     test_class.test_filename = __file
     test_class.mydir = TestBase.compute_mydir(__file)
     return test_class