[lldb] Make duplicate test names a conditional exception

[kastiglione]

When two or more tests have the same name, dotest will raise an exception. However, when
using a test name pattern (-p) which does not match the duplicate test names, there
seems to be no reason to prevent the user from running the tests they wish to run.

This changes the exception to be raised only when a pattern matches duplicate tests.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dave Lee (kastiglione)

Changes

When two or more tests have the same name, dotest will raise an exception. However, when
using a test name pattern (-p) which does not match the duplicate test names, there
seems to be no reason to prevent the user from running they wish to run.

This changes the exception to be raised only when a pattern matches duplicate tests.

---

Full diff: https://github.com/llvm/llvm-project/pull/137681.diff

1 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/dotest.py (+18-19)

diff --git a/lldb/packages/Python/lldbsuite/test/dotest.py b/lldb/packages/Python/lldbsuite/test/dotest.py
index 7cc8f2985043e..e9b5985e03c11 100644
--- a/lldb/packages/Python/lldbsuite/test/dotest.py
+++ b/lldb/packages/Python/lldbsuite/test/dotest.py
@@ -619,12 +619,12 @@ def visit_file(dir, name):
     if configuration.regexp:
         if not re.search(configuration.regexp, name):
             # We didn't match the regex, we're done.
-            return
+            return False
 
     if configuration.skip_tests:
         for file_regexp in configuration.skip_tests:
             if re.search(file_regexp, name):
-                return
+                return False
 
     # We found a match for our test.  Add it to the suite.
 
@@ -659,22 +659,20 @@ def iter_filters():
                     if check(value, parts):
                         yield key + "." + filterspec
 
-    filtered = False
-    for filterspec in iter_filters():
-        filtered = True
-        print("adding filter spec %s to module %s" % (filterspec, repr(module)))
-        tests = unittest.defaultTestLoader.loadTestsFromName(filterspec, module)
-        configuration.suite.addTests(tests)
-
-    # Forgo this module if the (base, filterspec) combo is invalid
-    if configuration.filters and not filtered:
-        return
+    if configuration.filters:
+        filtered = False
+        for filterspec in iter_filters():
+            filtered = True
+            print(f"adding filter spec {filterspec} to module {module!r}")
+            tests = unittest.defaultTestLoader.loadTestsFromName(filterspec, module)
+            configuration.suite.addTests(tests)
+        return filtered
 
-    if not filtered:
-        # Add the entire file's worth of tests since we're not filtered.
-        # Also the fail-over case when the filterspec branch
-        # (base, filterspec) combo doesn't make sense.
-        configuration.suite.addTests(unittest.defaultTestLoader.loadTestsFromName(base))
+    # Add the entire file's worth of tests since we're not filtered.
+    # Also the fail-over case when the filterspec branch
+    # (base, filterspec) combo doesn't make sense.
+    configuration.suite.addTests(unittest.defaultTestLoader.loadTestsFromName(base))
+    return True
 
 
 def visit(prefix, dir, names):
@@ -699,10 +697,11 @@ def visit(prefix, dir, names):
         # to disambiguate these, so we shouldn't need this constraint.
         if name in configuration.all_tests:
             raise Exception("Found multiple tests with the name %s" % name)
-        configuration.all_tests.add(name)
 
         # Run the relevant tests in the python file.
-        visit_file(dir, name)
+        if visit_file(dir, name):
+            # Only add to all_tests if the test wasn't skipped/filtered.
+            configuration.all_tests.add(name)
 
 
 # ======================================== #

[kastiglione]

This came up last week, where a duplicated test name (#137262) prevented me from running my own tests with lldb-dotest -p <TestWhatever>.

[JDevlieghere]

Rather than making this more lenient, could we go the opposite direction and make it so that we catch this issue when lit runs and the test suite falls over? I assume this is going unnoticed until someone uses dotest with the -p argument.

[kastiglione]

@JDevlieghere Does lit run all tests, even those that have duplicate names? If so, it seems like a step back to break lit from doing what it's capable of doing.

This code contains the below comment that proposes eliminating the test name uniqueness constraint. If that is the desired goal, then it's another reason to not put more uniqueness constraints in place.

# Future improvement: find all the places where we work with base
# names and convert to full paths.  We have directory structure
# to disambiguate these, so we shouldn't need this constraint.

[jimingham]

I'm not sure I agree that that "future improvement" suggestion is really an improvement. When you see the test fail, the name is what gets printed. You can grub around to find the path, but if you see a test fail, then succeed, then fail a bit later, what you will see will be the test name, not the test path, so it would be nice not to have to peer deeply into the log to see that those actually WEREN'T the same test failing...

[kastiglione]

@JDevlieghere do you agree with Jim? I was originally wanting to go in the direction of making things work better in the presence of duplicate test names, but if nobody likes that idea, I'm good with going the other direction (making lit error upon duplicate test names).

[labath]

A bit of history: In the past, our test suite created a "lldb-test-traces" directory, which contained some data ("traces") about each test run. The names of the trace files in this directory were based on the test name, which kind of meant that the test names had to be unique. There was nothing checking this though. In most cases this just meant that tests would overwrite each other's logs. However, if you were unlucky to run the two equally-named tests in parallel (and maybe if you were running on windows), the tests could actually fail due to a race in creating/updating the trace file.

This is why the check for was added to catch this and bail out early. It worked like Jonas suggests in #137681 (comment), in that it caused the entire test suite to fail if we had two files with the same name. I'm guessing that changed when we switched to the lit test runner (probably because lit runs dotest in a specific directory, so it never sees the conflicting files).

That said, it seems that, at some point (judging by the state of my build tree, some time after 2020), we stopped creating these trace files, so it's quite possible there isn't anything that actually depends on file names being unique.

@JDevlieghere do you agree with Jim? I was originally wanting to go in the direction of making things work better in the presence of duplicate test names, but if nobody likes that idea, I'm good with going the other direction (making lit error upon duplicate test names).

FWIW, I like that idea. I agree with the comment in #137681 (comment) -- we have directories to disambiguate things. At first I thought it was I who wrote that, but it look's like it was Todd -- and I just reviewed it :P (although, from the looks of things, that wasn't the main point of the patch). I'm not saying we should call all tests Test.py, but I don't think we need to be so pedantic about test name uniqueness. No other llvm test suite has that.