[LIT] error if directly named test won't be run indirectly

[bd1976llvm]

Summary

Currently, a LIT test named directly (on the command line) will be run even if the name of the test file does not meet the rules to be considered a test in the LIT test configuration files for its test suite. For example, if the test does not have a recognised file extension.

This makes it relatively easy to write a LIT test that won't actually be run. I did in: https://reviews.llvm.org/D82567

This patch adds an error to avoid users doing that. There is a small performance overhead for this check. A command line option has been added so that users can opt into the old behaviour.

Note to anyone reading this review in the future: The patch was initially to warn not error. I changed to error later and adjusted the Title and Summary.

[jhenderson]

You said to me offline when you were discussing this that you were concerned by performance implications. I take it this isn't the approach you had then, or you changed your mind?

[bd1976llvm]

In D83069#2129923, @jhenderson wrote:

You said to me offline when you were discussing this that you were concerned by performance implications. I take it this isn't the approach you had then, or you changed your mind?

That was an earlier version (where I was creating a set of all possible tests and then checking that the specified test wasn't in it). Performance seems acceptable now.

[thakis]

llvm/utils/lit/lit/discovery.py

163

This looks potentially slow. Did you do any perf measurements for this patch? What's lit test discovery time for some target (say, check-llvm) for this test vs without? (min-of-5, or ministat)

[bd1976llvm]

llvm/utils/lit/lit/discovery.py

163

Sorry for the slow reply.

I didn't measure any speed difference with check-llvm; that was because check-llvm does not invoke single tests directly (which is the only code path that this change impacts). Instead, I picked a directory with a large number of tests (llvm/test/CodeGen/X86/) and added a new test that would trigger the warning. I recorded these numbers running that test:

min-of-five without this change:

~/u/1$ time ~/u/build/bin/llvm-lit llvm/test/CodeGen/X86/zlib-longest-match.not 
-- Testing: 1 tests, 1 workers --
PASS: LLVM :: CodeGen/X86/zlib-longest-match.not (1 of 1)

Testing Time: 0.22s
  Expected Passes: 1

real    0m0.295s
user    0m0.233s
sys 0m0.035s

min-of-five with this change:

~/u/1$ time ~/u/build/bin/llvm-lit llvm/test/CodeGen/X86/zlib-longest-match.not 
llvm-lit: discovery.py:169: warning: 'LLVM :: CodeGen/X86/zlib-longest-match.not' would not be run indirectly: change name or LIT config
-- Testing: 1 tests, 1 workers --
PASS: LLVM :: CodeGen/X86/zlib-longest-match.not (1 of 1)

Testing Time: 0.22s
  Expected Passes: 1

1 warning(s) in tests

real    0m0.335s
user    0m0.274s
sys 0m0.030s

I also tested on windows (where filesystem operations are often more expensive) and recorded similar numbers.

So, there is a performance impact. Personally, I think this is acceptable *unless* users are listing large numbers of tests individually on their LIT command lines. That seems unlikely (IMO). However, if that is an important use-case then I think that emitting this warning only if a single test has been specified would be reasonable.

[bd1976llvm]

ping!

[bd1976llvm]

ping!

[bd1976llvm]

ping!

[probinson]

Adding @jdenny and @thopre who I think have been messing with lit recently.

[bd1976llvm]

Improvements:

• Error rather than warn - lit tests are regression tests!
• Add option to supress check (there is no performance cost if this option is specified).
• Standardized on the terms "direct" and "indirect" in comments and diagnostics.
• Added missing test file.

[thopre]

LGTM but I'm not familiar with lit to tell whether there would be a more idiomatic way to write this. I also agree with the assumption that invoking lit with a large number of tests arguments rather than a directory is unlikely.

llvm/utils/lit/tests/discovery.py

144

Why not test the full error message?

[bd1976llvm]

llvm/utils/lit/tests/discovery.py

144

I try to keep negative test cases from being too specific to try to prevent them from "failing for the wrong reason" (e.g. if in the future the error message is altered slightly then if we were checking the full error message the check would pass even though the, slightly altered, message is being emitted). However, looking at this again I don't think checking the output is worthwhile so I have removed it.

[bd1976llvm]

Removed check for error message not being emitted.

[bd1976llvm]

In D83069#2338104, @thopre wrote:

LGTM but I'm not familiar with lit to tell whether there would be a more idiomatic way to write this. I also agree with the assumption that invoking lit with a large number of tests arguments rather than a directory is unlikely.

Thanks for looking at this. If you're happy could you accept the change so that I can commit.

[thopre]

LGTM

[thopre]

Hi @bd1976llvm

I've just realized in many projects I'm involved in testsuites are managed by cmake and lit is only ever invoked on single testcase. Without passing --no-indirectly-run-check tests fail but when using an old lit I get an error if using this option. Can we allow to turn off the feature via environment variable or configuration?

Thanks in advance.

[bd1976llvm]

In D83069#2496184, @thopre wrote:

Hi @bd1976llvm

I've just realized in many projects I'm involved in testsuites are managed by cmake and lit is only ever invoked on single testcase. Without passing --no-indirectly-run-check tests fail but when using an old lit I get an error if using this option. Can we allow to turn off the feature via environment variable or configuration?

Thanks in advance.

Hi @thopre,

Sorry - I missed this after the Christmas break somehow. Reading https://reviews.llvm.org/D94766 it seems that you have decided that a config option would be better than a command line option to control this check? +1 to the config option. However, I think that we might still need --no-indirectly-run-check. My scenario would be to allow someone to add and run experimental tests within a teststuite that has the config option set without having to rename them or adjust the testsuite rules. I'm assuming that it is easier to add a command line option than to try to find and disable the relevant config option. Not sure if this is plausible as a scenario but I wanted to raise it. Having raised that scenario, maybe it's still better to add a config option and remove the command line option and then wait to see if there are any usability reports raised?

[thopre]

In D83069#2530787, @bd1976llvm wrote:

In D83069#2496184, @thopre wrote:

Hi @bd1976llvm

I've just realized in many projects I'm involved in testsuites are managed by cmake and lit is only ever invoked on single testcase. Without passing --no-indirectly-run-check tests fail but when using an old lit I get an error if using this option. Can we allow to turn off the feature via environment variable or configuration?

Thanks in advance.

Hi @thopre,

Sorry - I missed this after the Christmas break somehow. Reading https://reviews.llvm.org/D94766 it seems that you have decided that a config option would be better than a command line option to control this check? +1 to the config option. However, I think that we might still need --no-indirectly-run-check. My scenario would be to allow someone to add and run experimental tests within a teststuite that has the config option set without having to rename them or adjust the testsuite rules. I'm assuming that it is easier to add a command line option than to try to find and disable the relevant config option. Not sure if this is plausible as a scenario but I wanted to raise it. Having raised that scenario, maybe it's still better to add a config option and remove the command line option and then wait to see if there are any usability reports raised?

I'm not looking at removing the option. All I want is a mechanism to disable the check in a way that is ignored by old lit versions. So config + command-line is fine by me

llvm/utils/lit/lit/LitTestCase.py

lit_config = lit.LitConfig.LitConfig(
         valgrindLeakCheck=False,
         valgrindArgs=[],
         noExecute=False,
         debug=False,
         isWindows=windows,
         params={})
 
     # Perform test discovery.
-    tests = lit.discovery.find_tests_for_inputs(lit_config, inputs)
+    tests = lit.discovery.find_tests_for_inputs(lit_config, inputs, False)
     test_adaptors = [LitTestCase(t, lit_config) for t in tests]
 
     # Return a unittest test suite which just runs the tests in order.
-    return unittest.TestSuite(test_adaptors)
+    return unittest.TestSuite(test_adaptors)
 No newline at end of file

llvm/utils/lit/lit/cl_arguments.py

execution_group.add_argument("--timeout",
                  "0 means no time limit. [Default: 0]",
             type=_non_negative_int) # TODO(yln): --[no-]test-timeout, instead of 0 allowed
     execution_group.add_argument("--max-failures",
             help="Stop execution after the given number of failures.",
             type=_positive_int)
     execution_group.add_argument("--allow-empty-runs",
             help="Do not fail the run if all tests are filtered out",
             action="store_true")
+    execution_group.add_argument("--no-indirectly-run-check",
+            dest="indirectlyRunCheck",
+            help="Do not error if a test would not be run if the user had "
+                 "specified the containing directory instead of naming the "
+                 "test directly.",
+            action="store_false")
 
     selection_group = parser.add_argument_group("Test Selection")
     selection_group.add_argument("--max-tests",
             metavar="N",
             help="Maximum number of tests to run",
             type=_positive_int)
     selection_group.add_argument("--max-time", #TODO(yln): --timeout
             dest="timeout",

llvm/utils/lit/lit/discovery.py

def search(path_in_suite):
         key = (ts, path_in_suite)
         res = cache.get(key)
         if res is None:
             cache[key] = res = search1(path_in_suite)
         return res
 
     return search(path_in_suite)
 
-def getTests(path, litConfig, testSuiteCache, localConfigCache):
+def getTests(path, litConfig, testSuiteCache,
+             localConfigCache, indirectlyRunCheck):
     # Find the test suite for this input and its relative path.
     ts,path_in_suite = getTestSuite(path, litConfig, testSuiteCache)
     if ts is None:
         litConfig.warning('unable to find test suite for %r' % path)
         return (),()
 
     if litConfig.debug:
         litConfig.note('resolved input %r to %r::%r' % (path, ts.name,
                                                         path_in_suite))
 
     return ts, getTestsInSuite(ts, path_in_suite, litConfig,
-                               testSuiteCache, localConfigCache)
+                               testSuiteCache, localConfigCache, indirectlyRunCheck)
 
 def getTestsInSuite(ts, path_in_suite, litConfig,
-                    testSuiteCache, localConfigCache):
+                    testSuiteCache, localConfigCache, indirectlyRunCheck):
     # Check that the source path exists (errors here are reported by the
     # caller).
     source_path = ts.getSourcePath(path_in_suite)
     if not os.path.exists(source_path):
         return
 
     # Check if the user named a test directly.
     if not os.path.isdir(source_path):
-        lc = getLocalConfig(ts, path_in_suite[:-1], litConfig, localConfigCache)
-        yield Test.Test(ts, path_in_suite, lc)
+        test_dir_in_suite = path_in_suite[:-1]
+        lc = getLocalConfig(ts, test_dir_in_suite, litConfig, localConfigCache)
+        test = Test.Test(ts, path_in_suite, lc)
+
+        # Issue a error if the specified test would not be run if
+        # the user had specified the containing directory instead of
+        # of naming the test directly. This helps to avoid writing
+        # tests which are not executed. The check adds some performance
+        # overhead which might be important if a large number of tests
+        # are being run directly.
+        # --no-indirectly-run-check: skips this check.
+        if indirectlyRunCheck and lc.test_format is not None:
+            found = False
+            for res in lc.test_format.getTestsInDirectory(ts, test_dir_in_suite,
+                                                          litConfig, lc):
+                if test.getFullName() == res.getFullName():
+                    found = True
+                    break
+            if not found:
+                litConfig.error(
+                    '%r would not be run indirectly: change name or LIT config'
+                    % test.getFullName())
+
+        yield test
         return
 
     # Otherwise we have a directory to search for tests, start by getting the
     # local configuration.
     lc = getLocalConfig(ts, path_in_suite, litConfig, localConfigCache)
 
     # Search for tests.
     if lc.test_format is not None:
for filename in os.listdir(source_path):
         # disregard it (this can happen if the exec root is located inside the
         # current test suite, for example).
         if sub_ts is ts:
             continue
 
         # Otherwise, load from the nested test suite, if present.
         if sub_ts is not None:
             subiter = getTestsInSuite(sub_ts, subpath_in_suite, litConfig,
-                                      testSuiteCache, localConfigCache)
+                                      testSuiteCache, localConfigCache,
+                                      indirectlyRunCheck)
         else:
             subiter = getTestsInSuite(ts, subpath, litConfig, testSuiteCache,
-                                      localConfigCache)
+                                      localConfigCache, indirectlyRunCheck)
 
         N = 0
         for res in subiter:
             N += 1
             yield res
         if sub_ts and not N:
             litConfig.warning('test suite %r contained no tests' % sub_ts.name)
 
-def find_tests_for_inputs(lit_config, inputs):
+def find_tests_for_inputs(lit_config, inputs, indirectlyRunCheck):
     """
     find_tests_for_inputs(lit_config, inputs) -> [Test]
 
     Given a configuration object and a list of input specifiers, find all the
     tests to execute.
     """
 
     # Expand '@...' form in inputs.
for input in inputs:
             actual_inputs.append(input)
 
     # Load the tests from the inputs.
     tests = []
     test_suite_cache = {}
     local_config_cache = {}
     for input in actual_inputs:
         prev = len(tests)
-        tests.extend(getTests(input, lit_config,
-                              test_suite_cache, local_config_cache)[1])
+        tests.extend(getTests(input, lit_config, test_suite_cache,
+                              local_config_cache, indirectlyRunCheck)[1])
         if prev == len(tests):
             lit_config.warning('input %r contained no tests' % input)
 
     # If there were any errors during test discovery, exit now.
     if lit_config.numErrors:
         sys.stderr.write('%d errors, exiting.\n' % lit_config.numErrors)
         sys.exit(2)
 
-    return tests
+    return tests
 No newline at end of file

llvm/utils/lit/lit/main.py

lit_config = lit.LitConfig.LitConfig(
         valgrindArgs=opts.valgrindArgs,
         noExecute=opts.noExecute,
         debug=opts.debug,
         isWindows=is_windows,
         params=params,
         config_prefix=opts.configPrefix,
         echo_all_commands=opts.echoAllCommands)
 
-    discovered_tests = lit.discovery.find_tests_for_inputs(lit_config, opts.test_paths)
+    discovered_tests = lit.discovery.find_tests_for_inputs(lit_config, opts.test_paths,
+                                                           opts.indirectlyRunCheck)
     if not discovered_tests:
         sys.stderr.write('error: did not discover any tests for provided path(s)\n')
         sys.exit(2)
 
     if opts.show_suites or opts.show_tests:
         print_discovered(discovered_tests, opts.show_suites, opts.show_tests)
         sys.exit(0)
 

llvm/utils/lit/tests/Inputs/discovery/test.not-txt

+# RUN: true

llvm/utils/lit/tests/discovery.py

 # CHECK-CONFIG-MAP-OUT:   config-map :: test1.txt
 # CHECK-CONFIG-MAP-OUT:   config-map :: test2.txt
 
 # CHECK-CONFIG-MAP-ERR: loading suite config '{{.*}}lit.alt.cfg'
 # CHECK-CONFIG-MAP-ERR: loaded config '{{.*}}lit.alt.cfg'
 # CHECK-CONFIG-MAP-ERR: resolved input '{{.*(/|\\\\)config-map-discovery(/|\\\\)main-config}}' to 'config-map'::()
 
 
-# Check discovery when exact test names are given.
+# Check discovery when tests are named directly.
 #
 # RUN: %{lit} \
 # RUN:     %{inputs}/discovery/subdir/test-three.py \
 # RUN:     %{inputs}/discovery/subsuite/test-one.txt \
 # RUN:   -j 1 --show-tests --show-suites -v > %t.out
-# RUN: FileCheck --check-prefix=CHECK-EXACT-TEST < %t.out %s
+# RUN: FileCheck --check-prefix=CHECK-DIRECT-TEST < %t.out %s
 #
-# CHECK-EXACT-TEST: -- Available Tests --
-# CHECK-EXACT-TEST: sub-suite :: test-one
-# CHECK-EXACT-TEST: top-level-suite :: subdir/test-three
+# CHECK-DIRECT-TEST: -- Available Tests --
+# CHECK-DIRECT-TEST: sub-suite :: test-one
+# CHECK-DIRECT-TEST: top-level-suite :: subdir/test-three
 
 # Check discovery when config files end in .py
 # RUN: %{lit} %{inputs}/py-config-discovery \
 # RUN:   -j 1 --debug --show-tests --show-suites \
 # RUN:   -v > %t.out 2> %t.err
 # RUN: FileCheck --check-prefix=CHECK-PYCONFIG-OUT < %t.out %s
 # RUN: FileCheck --check-prefix=CHECK-PYCONFIG-ERR < %t.err %s
 #

 #
 # CHECK-ASEXEC-OUT: -- Available Tests --
 # CHECK-ASEXEC-OUT: sub-suite :: test-one
 # CHECK-ASEXEC-OUT: sub-suite :: test-two
 # CHECK-ASEXEC-OUT: top-level-suite :: subdir/test-three
 # CHECK-ASEXEC-OUT: top-level-suite :: test-one
 # CHECK-ASEXEC-OUT: top-level-suite :: test-two
 
-# Check discovery when exact test names are given.
+# Check discovery when tests are named directly.
 #
 # FIXME: Note that using a path into a subsuite doesn't work correctly here.
 #
 # RUN: %{lit} \
 # RUN:     %{inputs}/exec-discovery/subdir/test-three.py \
 # RUN:   -j 1 --show-tests --show-suites -v > %t.out
-# RUN: FileCheck --check-prefix=CHECK-ASEXEC-EXACT-TEST < %t.out %s
+# RUN: FileCheck --check-prefix=CHECK-ASEXEC-DIRECT-TEST < %t.out %s
 #
-# CHECK-ASEXEC-EXACT-TEST: -- Available Tests --
-# CHECK-ASEXEC-EXACT-TEST: top-level-suite :: subdir/test-three
+# CHECK-ASEXEC-DIRECT-TEST: -- Available Tests --
+# CHECK-ASEXEC-DIRECT-TEST: top-level-suite :: subdir/test-three
 
+# Check an error is emitted when the directly named test would not be run
+# indirectly (e.g. when the directory containing the test is specified).
+#
+# RUN: not %{lit} \
+# RUN:     %{inputs}/discovery/test.not-txt -j 1 2>%t.err
+# RUN: FileCheck --check-prefix=CHECK-ERROR-INDIRECT-RUN-CHECK < %t.err %s
+#
+# CHECK-ERROR-INDIRECT-RUN-CHECK: error: 'top-level-suite :: test.not-txt' would not be run indirectly
+
+# Check that no error is emitted with --no-indirectly-run-check.
+#
+# RUN: %{lit} \
+# RUN:     %{inputs}/discovery/test.not-txt -j 1 --no-indirectly-run-check
 
 # Check that we don't recurse infinitely when loading an site specific test
 # suite located inside the test source root.
 #
 # RUN: %{lit} \
 # RUN:     %{inputs}/exec-discovery-in-tree/obj/ \
 # RUN:   -j 1 --show-tests --show-suites -v > %t.out
 # RUN: FileCheck --check-prefix=CHECK-ASEXEC-INTREE < %t.out %s
 #
 # Try it again after cd'ing into the test suite using a short relative path.
 #
 # RUN: cd %{inputs}/exec-discovery-in-tree/obj/
 # RUN: %{lit} . \
 # RUN:   -j 1 --show-tests --show-suites -v > %t.out
 # RUN: FileCheck --check-prefix=CHECK-ASEXEC-INTREE < %t.out %s
 #
 #      CHECK-ASEXEC-INTREE:   exec-discovery-in-tree-suite - 1 tests
 # CHECK-ASEXEC-INTREE-NEXT:     Source Root: {{.*[/\\]exec-discovery-in-tree$}}
 # CHECK-ASEXEC-INTREE-NEXT:     Exec Root  : {{.*[/\\]exec-discovery-in-tree[/\\]obj$}}
 # CHECK-ASEXEC-INTREE-NEXT:     Available Features:
 # CHECK-ASEXEC-INTREE-NEXT:     Available Substitutions:
 # CHECK-ASEXEC-INTREE-NEXT: -- Available Tests --
-# CHECK-ASEXEC-INTREE-NEXT: exec-discovery-in-tree-suite :: test-one
+# CHECK-ASEXEC-INTREE-NEXT: exec-discovery-in-tree-suite :: test-one
 No newline at end of file

llvm/utils/lit/tests/unit/TestRunner.py

def load_keyword_parser_lit_tests():
                                              isWindows=(
                                                platform.system() == 'Windows'),
                                              params={})
         TestIntegratedTestKeywordParser.litConfig = lit_config
         # Perform test discovery.
         test_path = os.path.dirname(os.path.dirname(__file__))
         inputs = [os.path.join(test_path, 'Inputs/testrunner-custom-parsers/')]
         assert os.path.isdir(inputs[0])
-        tests = lit.discovery.find_tests_for_inputs(lit_config, inputs)
+        tests = lit.discovery.find_tests_for_inputs(lit_config, inputs, False)
         assert len(tests) == 1 and "there should only be one test"
         TestIntegratedTestKeywordParser.inputTestCase = tests[0]
 
     @staticmethod
     def make_parsers():
         def custom_parse(line_number, line, output):
             if output is None:
                 output = []
def test_recursive_substitution_invalid_value(self):
                 lit.TestRunner.applySubstitutions(script, substitutions, recursion_limit=limit)
                 self.fail("applySubstitutions should have raised an exception")
             except AssertionError:
                 pass
 
 
 if __name__ == '__main__':
     TestIntegratedTestKeywordParser.load_keyword_parser_lit_tests()
-    unittest.main(verbosity=2)
+    unittest.main(verbosity=2)
 No newline at end of file