Page MenuHomePhabricator

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

Authored by JDevlieghere on Jun 9 2020, 4:14 PM.

Details

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.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jun 9 2020, 4:14 PM

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 added inline comments.Jun 9 2020, 4:58 PM
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?

JDevlieghere marked 2 inline comments as done.
labath requested changes to this revision.Jun 10 2020, 1:04 AM

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.
This revision now requires changes to proceed.Jun 10 2020, 1:04 AM

Implement Pavel's approach

JDevlieghere marked 2 inline comments as done.Jun 10 2020, 9:17 AM
JDevlieghere added inline comments.
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 accepted this revision.Jun 11 2020, 5:25 AM
labath added inline comments.
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.

This revision is now accepted and ready to land.Jun 11 2020, 5:25 AM
JDevlieghere marked 3 inline comments as done.Jun 11 2020, 9:34 AM
JDevlieghere added inline comments.
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 ;-)

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2020, 9:55 AM