Add lit config for dir with standalone tests

[thopre]

Summary

Some test systems do not use lit for test discovery but only for its
substitution and test selection because they use another way of managing
test collections, e.g. CTest. This forces those tests to be invoked with
lit --no-indirectly-run-check. When a mix of lit version is in use, it
requires to detect the availability of that option.

This commit provides a new config option standalone_tests to signal a
directory made of tests meant to run as standalone. When this option is
set, lit skips test discovery and the indirectly run check. It also adds
the missing documentation for --no-indirectly-run-check.

[thopre]

Ping?

[jdenny]

Can you explain the use case a bit more? I don't find myself moving between old and new versions of lit very often. Perhaps I'm naive, but it seems like it should be easy enough to just not specify --no-indirectly-run-check when using the old version.

[thopre]

In D94766#2512681, @jdenny wrote:

Can you explain the use case a bit more? I don't find myself moving between old and new versions of lit very often. Perhaps I'm naive, but it seems like it should be easy enough to just not specify --no-indirectly-run-check when using the old version.

Our systems use a mix of LLVM versions (whatever is available on that OS). This causes problem for this option because tests are run individually using lit (tests are managed by CTest which runs lit) and thus require the --no-indirect-run-check option to not fail due to warning but then systems with an old lit fail because the option is not accepted. Having a variable would allow to support a nice mix. It also allows to upgrade to a newer lit without having to update lit and the tests in lockstep.

The problem of --no-indirectly-run-check is it changes the behavior of lit. This is fine in LLVM but lit is used outside of LLVM and I think we should acknowledge that and aim to maintain compatibility to some degree (if cost is low, it should not become a burden to LLVM). The environment variable allow a smooth migration from one version to the next (and a mix of version in our case). A lit configuration option would work as well.

[jdenny]

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

[yln]

In D94766#2513022, @jdenny wrote:

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

+1

I feel the same as Joel. Not opposed, but my default stance is to try to avoid collecting and codifying workarounds. They almost never get removed once obsolete.

[thopre]

In D94766#2513022, @jdenny wrote:

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In our case lit is invoked for each testcase (1 lit invocation per testcase) and testcases are run in parallel. That would require detection before the tests but then you would need a fake test for that because lit does not have a mechanism to ask about whether an option exist except running a known good test with the option. But all these are specifics to our setup (we have a workaround anyway), I'm more concerned about having a migration strategy.

The new option was made a default because we fixed all testcases internally to LLVM but it remains a disruption for uses outside of LLVM. Note that I've approved that patch so I blame myself for not thinking of that. I'm just trying to fix it now by having a nice migration plan for external users. Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

[jdenny]

In D94766#2513362, @thopre wrote:

In D94766#2513022, @jdenny wrote:

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In our case lit is invoked for each testcase (1 lit invocation per testcase) and testcases are run in parallel. That would require detection before the tests but then you would need a fake test for that because lit does not have a mechanism to ask about whether an option exist except running a known good test with the option.

Yes, that's what I had in mind. It seems simple, but maybe I'm missing something.

A fake test is probably the most robust way to check. But an even simpler check that should work for now is: llvm-lit --help | grep -q -- --no-indirectly-run-check.

But all these are specifics to our setup (we have a workaround anyway), I'm more concerned about having a migration strategy.

What makes it specific to your setup? Feature tests when building/testing a project are common.

The new option was made a default because we fixed all testcases internally to LLVM but it remains a disruption for uses outside of LLVM. Note that I've approved that patch so I blame myself for not thinking of that. I'm just trying to fix it now by having a nice migration plan for external users.

Are there many external users suffering from this? If so, hopefully they can chime in here.

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

[thopre]

In D94766#2513512, @jdenny wrote:

In D94766#2513362, @thopre wrote:

In D94766#2513022, @jdenny wrote:

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In our case lit is invoked for each testcase (1 lit invocation per testcase) and testcases are run in parallel. That would require detection before the tests but then you would need a fake test for that because lit does not have a mechanism to ask about whether an option exist except running a known good test with the option.

Yes, that's what I had in mind. It seems simple, but maybe I'm missing something.

A fake test is probably the most robust way to check. But an even simpler check that should work for now is: llvm-lit --help | grep -q -- --no-indirectly-run-check.

But all these are specifics to our setup (we have a workaround anyway), I'm more concerned about having a migration strategy.

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

The new option was made a default because we fixed all testcases internally to LLVM but it remains a disruption for uses outside of LLVM. Note that I've approved that patch so I blame myself for not thinking of that. I'm just trying to fix it now by having a nice migration plan for external users.

Are there many external users suffering from this? If so, hopefully they can chime in here.

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

[jdenny]

In D94766#2513640, @thopre wrote:

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

If you're using CTest, then you're likely using CMake, which is a typical place to add feature tests for the tools your project's build/tests will use. Does that not work?

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

Ah, so you mean something like config.new_feature = 1. For some reason, I was thinking of calling a function.

Is it actually useful to implement that for any other command-line option?

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

[thopre]

In D94766#2513926, @jdenny wrote:

In D94766#2513640, @thopre wrote:

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

If you're using CTest, then you're likely using CMake, which is a typical place to add feature tests for the tools your project's build/tests will use. Does that not work?

No because the project in question is built before LLVM. Only at test time is lit guaranteed to be there. But as I said, no need to talk about my specifics, I can find a workaround if needed.

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

Ah, so you mean something like config.new_feature = 1. For some reason, I was thinking of calling a function.

Yes that's what I'm thinking about.

Is it actually useful to implement that for any other command-line option?

Just looking at lit usage message, I could see most option making sense as a config option. I don't personally need them though. I guess a config option of --no-indirect-run-check is enough and can be generalized later.

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

No that makes sense. A config option would work for me. Should I make a new patch to that effect then?

[jdenny]

In D94766#2515070, @thopre wrote:

But as I said, no need to talk about my specifics, I can find a workaround if needed.

Well, it's the only example I have so far. But we seem to be converging on another solution now.

Just looking at lit usage message, I could see most option making sense as a config option.

For me, it's just the opposite. Most seem like they are not properties of a test suite that belong in the config. Most seem like they are specific to a run of the test suite.

I don't personally need them though. I guess a config option of --no-indirect-run-check is enough and can be generalized later.

That's fine. But I am curious to hear more about the general use case. If it's useful, it will probably come up again.

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

No that makes sense. A config option would work for me. Should I make a new patch to that effect then?

Ignoring backward-compatibility, would the command-line option then be unnecessary? Whether we remove it immediately is a different question.

Before you invest time in this, it might be good to hear from someone else. @yln, does this solution seem reasonable to you?

[thopre]

In D94766#2515570, @jdenny wrote:

Ignoring backward-compatibility, would the command-line option then be unnecessary? Whether we remove it immediately is a different question.

Good question, I would leave it because it could allow for quickly testing a new test with a new suffix without updating lit and it's a small amount of code but YMMV. No strong preference.

[yln]

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

No that makes sense. A config option would work for me. Should I make a new patch to that effect then?

Ignoring backward-compatibility, would the command-line option then be unnecessary? Whether we remove it immediately is a different question.

Before you invest time in this, it might be good to hear from someone else. @yln, does this solution seem reasonable to you?

Sounds good to me, thanks! :)

[thopre]

Rework into a config option

[thopre]

In D94766#2513926, @jdenny wrote:

In D94766#2513640, @thopre wrote:

What makes it specific to your setup? Feature tests when building/testing a project are common.

Oops indeed, nothing specific to our setup in what I said. I left out that each test invoked with lit is managed by CTest which AFAIK does not allow to set a dependency on a piece of code to run first. And I don't see a way to call a script before ctest is called. This is specific to our setup though and I could do the detection on each invocation of lit. It's not very satisfying but it sure can be done.

If you're using CTest, then you're likely using CMake, which is a typical place to add feature tests for the tools your project's build/tests will use. Does that not work?

Perhaps a more palatable approach would be to make all options have a corresponding lit config rather than something specific to the --no-indirect-run-check. How does that sound?

By "lit config", do you mean a flag you can set in a lit config file, like lit.cfg.py? How do you imagine that old lit versions should handle lit configs they don't recognize?

Yes I was thinking of lit.cfg.py. Does it not only react to configuration option it knows about?

Ah, so you mean something like config.new_feature = 1. For some reason, I was thinking of calling a function.

Is it actually useful to implement that for any other command-line option?

Actually, it seems to me that --no-indirect-run-check probably should have been a lit config option in the first place. It declares a property of the test suite: it has legitimate tests that are only run individually. lit invocations shouldn't have to declare that again and again. Or have I misunderstood the use case?

How's this new approach? Feel free to suggest a better config name.

[jdenny]

llvm/docs/CommandGuide/lit.rst

381

I assume then that there's no known use case where a test directory sometimes relies on test discovery but also has standalone tests. It sounds like it's one or the other.

Then should standalone_tests complain if suffixes or excludes is set?

Actually, if suffixes isn't set (or is empty or None), should that be enough to enable --no-indirectly-run-check? Do we need standalone_tests?

llvm/utils/lit/lit/discovery.py

162–163

This comment should probably mention standalone_tests as well.

178–179

It would be more helpful if this pointed to the relevant config/command-line option(s).

llvm/utils/lit/tests/discovery.py

153

I think the above two should check that a test actually ran.

[thopre]

Address review comments

[thopre]

llvm/docs/CommandGuide/lit.rst

381

I think a mix of discoverable tests and non discoverable ones is indeed unlikely and also non desirable because then when adding a new tests if it is not discoverable you cannot know whether it was intentional or an error (e.g. suffixes should have been modified to add a new suffix).

I'm reluctant to use suffixes being unset to disable the indirect check because not setting it might have been an error (i.e. someone creates a new directory, adds tests and a config name but forgets the suffixes). Besides suffixes is not used for all lit format (googletest ignores it).

[jdenny]

LGTM except for the one minor change to the diagnostic. Thanks for all the work.

llvm/docs/CommandGuide/lit.rst

381

Besides suffixes is not used for all lit format (googletest ignores it).

Ah, makes sense.

llvm/utils/lit/lit/discovery.py

178–179

Please also mention standalone_tests as it's a lesser known way past this error.

[jdenny]

llvm/docs/CommandGuide/lit.rst

380

One last request: can we indicate what values this accepts? I understand you're mimicking other documentation, but I find "Mark a" to be unclear.

[thopre]

Address review comments

llvm/docs/CommandGuide/lit.rst

 
 .. option:: --time-tests
 
  Track the wall time individual tests take to execute and includes the results
  in the summary output.  This is useful for determining which tests in a test
  suite take the most time to execute.  Note that this option is most useful
  with ``-j 1``.
 
+.. option:: --no-indirectly-run-check
+
+ Do not error if a test would not be run if the user had specified the
+ containing directory instead of naming the test directly.
+
 .. _selection-options:
 
 SELECTION OPTIONS
 -----------------
 
 .. option:: --max-failures N
 
  Stop execution after the given number ``N`` of failures.

 
  **test_exec_root** For out-of-dir builds, the path to the test suite root inside
  the object directory.  This is where tests will be run and temporary output files
  placed.
 
  **environment** A dictionary representing the environment to use when executing
  tests in the suite.
 
+ **standalone_tests** When true, mark a directory with tests expected to be run
+ standalone. Test discovery is disabled for that directory and
+ *--no-indirectly-run-check* is in effect. *lit.suffixes* and *lit.excludes*
+ must be empty when this variable is true.
+
  **suffixes** For **lit** test formats which scan directories for tests, this
  variable is a list of suffixes to identify test files.  Used by: *ShTest*.
 
  **substitutions** For **lit** test formats which substitute variables into a test
  script, the list of substitutions to perform.  Used by: *ShTest*.
 
  **unsupported** Mark an unsupported directory, all tests within it will be
  reported as unsupported.  Used by: *ShTest*.

llvm/utils/lit/lit/TestingConfig.py

def fromdefaults(litConfig):
                              test_format = None,
                              environment = environment,
                              substitutions = [],
                              unsupported = False,
                              test_exec_root = None,
                              test_source_root = None,
                              excludes = [],
                              available_features = available_features,
-                             pipefail = True)
+                             pipefail = True,
+                             standalone_tests = False)
 
     def load_from_path(self, path, litConfig):
         """
         load_from_path(path, litConfig)
 
         Load the configuration module at the provided path into the given config
         object.
         """
def load_from_path(self, path, litConfig):
                 'unable to parse config file %r, traceback: %s' % (
                     path, traceback.format_exc()))
         self.finish(litConfig)
 
     def __init__(self, parent, name, suffixes, test_format,
                  environment, substitutions, unsupported,
                  test_exec_root, test_source_root, excludes,
                  available_features, pipefail, limit_to_features = [],
-                 is_early = False, parallelism_group = None):
+                 is_early = False, parallelism_group = None,
+                 standalone_tests = False):
         self.parent = parent
         self.name = str(name)
         self.suffixes = set(suffixes)
         self.test_format = test_format
         self.environment = dict(environment)
         self.substitutions = list(substitutions)
         self.unsupported = unsupported
         self.test_exec_root = test_exec_root
         self.test_source_root = test_source_root
         self.excludes = set(excludes)
         self.available_features = set(available_features)
         self.pipefail = pipefail
+        self.standalone_tests = standalone_tests
         # This list is used by TestRunner.py to restrict running only tests that
         # require one of the features in this list if this list is non-empty.
         # Configurations can set this list to restrict the set of tests to run.
         self.limit_to_features = set(limit_to_features)
         # Whether the suite should be tested early in a given run.
         self.is_early = bool(is_early)
         self.parallelism_group = parallelism_group
         self._recursiveExpansionLimit = None

llvm/utils/lit/lit/discovery.py

if not os.path.isdir(source_path):
         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:
+        # This check can be disabled by using --no-indirectly-run-check or
+        # setting the standalone_tests variable in the suite's configuration.
+        if (
+            indirectlyRunCheck
+            and lc.test_format is not None
+            and not lc.standalone_tests
+        ):
             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'
+                    '(e.g. suffixes or standalone_tests variables)'
                     % 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)
 
+    # Directory contains tests to be run standalone. Do not try to discover.
+    if lc.standalone_tests:
+        if lc.suffixes or lc.excludes:
+            litConfig.warning(
+                'standalone_tests set in LIT config but suffixes or excludes'
+                    ' are also set'
+            )
+        return
+
     # Search for tests.
     if lc.test_format is not None:
         for res in lc.test_format.getTestsInDirectory(ts, path_in_suite,
                                                       litConfig, lc):
             yield res
 
     # Search subdirectories.
     for filename in os.listdir(source_path):

llvm/utils/lit/tests/Inputs/standalone-tests-with-excludes/lit.cfg

+import lit.formats
+config.name = 'Standalone tests'
+config.test_format = lit.formats.ShTest()
+config.excludes = ['.test']
+config.standalone_tests = True

llvm/utils/lit/tests/Inputs/standalone-tests-with-excludes/true.txt

+# RUN: true

llvm/utils/lit/tests/Inputs/standalone-tests-with-suffixes/lit.cfg

+import lit.formats
+config.name = 'Standalone tests'
+config.test_format = lit.formats.ShTest()
+config.suffixes = ['.txt']
+config.standalone_tests = True

llvm/utils/lit/tests/Inputs/standalone-tests-with-suffixes/true.txt

+# RUN: true

llvm/utils/lit/tests/Inputs/standalone-tests/lit.cfg

+import lit.formats
+config.name = 'Standalone tests'
+config.test_format = lit.formats.ShTest()
+config.standalone_tests = True

llvm/utils/lit/tests/Inputs/standalone-tests/true.txt

+# RUN: true

llvm/utils/lit/tests/discovery.py

 #
 # 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 a standalone test with no suffixes set is run without any errors.
+#
+# RUN: %{lit} %{inputs}/standalone-tests/true.txt -j 1 > %t.out
+# RUN: FileCheck --check-prefix=CHECK-STANDALONE < %t.out %s
+#
+# CHECK-STANDALONE: PASS: Standalone tests :: true.txt
+
+# Check that an error is produced if suffixes variable is set for a suite with
+# standalone tests.
+#
+# RUN: not %{lit} %{inputs}/standalone-tests-with-suffixes -j 1 2> %t.err
+# RUN: FileCheck --check-prefixes=CHECK-STANDALONE-SUFFIXES,CHECK-STANDALONE-DISCOVERY < %t.err %s
+#
+# CHECK-STANDALONE-SUFFIXES: standalone_tests set {{.*}} but suffixes
+
+# Check that an error is produced if excludes variable is set for a suite with
+# standalone tests.
+#
+# RUN: not %{lit} %{inputs}/standalone-tests-with-excludes -j 1 2> %t.err
+# RUN: FileCheck --check-prefixes=CHECK-STANDALONE-EXCLUDES,CHECK-STANDALONE-DISCOVERY < %t.err %s
+#
+# CHECK-STANDALONE-EXCLUDES: standalone_tests set {{.*}} but {{.*}} excludes
+
+# Check that no discovery is done for testsuite with standalone tests.
+#
+# RUN: not %{lit} %{inputs}/standalone-tests -j 1 2>%t.err
+# RUN: FileCheck --check-prefix=CHECK-STANDALONE-DISCOVERY < %t.err %s
+#
+# CHECK-STANDALONE-DISCOVERY: error: did not discover any tests for provided path(s)
+
 # 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
 #