[lit] Add "early_tests" config option

[davezarzycki]

Summary

With enough cores, the slowest tests can significantly change the total testing time if they happen to run late. With this change, a test suite can improve performance (for high-end systems) by listing just a few of the slowest tests up front.

[jdenny]

Seems useful.

For consistency with the existing option, shouldn't it be named early_tests?

Needs documentation and tests.

[davezarzycki]

I can rename the variable if you want. That being said, this proposal is a mere refinement to the "run early" feature. And unless I'm missing something, the "run early" feature lacks documentation and tests. Are you asking me to fix that? I'd also like to point out that timing tests are surprisingly difficult to write if one cares about not being sensitive to the performance of the underlying hardware or how busy (or not) the machine is.

[jdenny]

In D96594#2561643, @davezarzycki wrote:

I can rename the variable if you want.

Thanks. I'm open to discussion, but I think consistency is important. Also, looking at the comments for isEarlyTest, it looks like the original feature was meant to be useful beyond just slow tests, so the name is more general.

That being said, this proposal is a mere refinement to the "run early" feature. And unless I'm missing something, the "run early" feature lacks documentation and tests.

That's unfortunate.

Are you asking me to fix that?

If you're willing, that would be great and probably not much more work. But I'm only asking you to document and test your changes.

I'd also like to point out that timing tests are surprisingly difficult to write if one cares about not being sensitive to the performance of the underlying hardware or how busy (or not) the machine is.

Many of lit's tests execute with -j1 for deterministic output. In this case, we'd need an example test suite whose tests run in a different order when some are configured as early tests. Does that seem reasonable?

Thanks for working on this feature. I have a downstream test suite that I think will benefit.

llvm/utils/lit/lit/Test.py

407

It looks like this requires the new config to use / as a path separator regardless of the platform. This decision should probably be consistent with however excludes works, but I haven't looked into that. In any case, please mention this decision in the user documentation.

[davezarzycki]

I think I have addressed all of the feedback to date. Thank you @jdenny

Please note, the / separator is harmless and we need some kind of separator. It doesn't affect any filesystem APIs, just test sorting.

[jdenny]

LGTM except please update the title for the new name.

[jhenderson]

I'd like to take a look at this, but I have too many things on my plate today. I can probably make time tomorrow or Wednesday though, if you're happy to hold off until then?

[thopre]

LGTM too

[davezarzycki]

In D96594#2562990, @jhenderson wrote:

I'd like to take a look at this, but I have too many things on my plate today. I can probably make time tomorrow or Wednesday though, if you're happy to hold off until then?

That's fine. No rush. I'll assume that unless someone wants a change by next weekend, then the commit is ready to land.

[jhenderson]

Essentially looks good, aside from a few small things.

It would be nice if this option could be configured within the test itself, perhaps in a similar manner to how REQUIRES etc works. This would allow the test properties (i.e. something like "I'm slow") to be encoded within the test itself. It would also be nice if there were a lit command-line option to achieve the same thing - in some cases, people might want to see specific tests run early (because for example they are more likely to fail, so want to be observed early) without wanting to modify the testsuite. I'm happy for these to be separate patches.

llvm/utils/lit/lit/TestingConfig.py

127

There might be other reasons to run specific tests first (e.g. ones that are more likely to fail), so make this comment less specific.

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

7

I think we need to exercise the '/' separator behaviour as part of this test somehow.

llvm/utils/lit/tests/early-tests.py

1

Something I've been encouraging in newer tests is to use ## for comments (or equivalent e.g. ;;) to make true comments stand out from lit and FileCheck directives.

[davezarzycki]

I believe I have addressed all of the feedback to date.

As to having a "run early" hint within the test files, I did look into that. For better and for worse, lit parses the test files as a part of the execution path, which is long after lit has sorted the tests. Changing this would be a nontrivial redesign of lit and it would put the parsing of the test suite (as opposed to mere discovery and sorting) on the critical path before tests are run. This approach is simpler and far less controversial.

[jhenderson]

Looks good. The command-line option might still be nice, if it's viable, but it can be separate.

llvm/docs/CommandGuide/lit.rst

  reported as unsupported.  Used by: *ShTest*.
 
  **parent** The parent configuration, this is the config object for the directory
  containing the test suite, or None.
 
  **root** The root configuration.  This is the top-most :program:`lit` configuration in
  the project.
 
+ **is_early** Whether the test suite as a whole should be given a head start
+ before other test suites run.
+
+ **early_tests** An explicit set of '/' separated test paths that should be
+ given a head start before other tests run. For example, the top five or so
+ slowest tests. See also: `--time-tests`
+
  **pipefail** Normally a test using a shell pipe fails if any of the commands
  on the pipe fail. If this is not desired, setting this variable to false
  makes the test fail only if the last command in the pipe fails.
 
  **available_features** A set of features that can be used in `XFAIL`,
  `REQUIRES`, and `UNSUPPORTED` directives.
 
 TEST DISCOVERY

llvm/utils/lit/lit/Test.py

class Test:
     def isEarlyTest(self):
         """
         isEarlyTest() -> bool
 
         Check whether this test should be executed early in a particular run.
         This can be used for test suites with long running tests to maximize
         parallelism or where it is desirable to surface their failures early.
         """
+        if '/'.join(self.path_in_suite) in self.suite.config.early_tests:
+            return True
         return self.suite.config.is_early

llvm/utils/lit/lit/TestingConfig.py

def __init__(self, parent, name, suffixes, test_format,
         self.available_features = set(available_features)
         self.pipefail = pipefail
         # 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)
+        # List of slowest tests to run early.
+        self.early_tests = {}
         self.parallelism_group = parallelism_group
         self._recursiveExpansionLimit = None
 
     @property
     def recursiveExpansionLimit(self):
         return self._recursiveExpansionLimit
 
     @recursiveExpansionLimit.setter

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

+import lit.formats
+config.name = 'early-tests'
+config.suffixes = ['.txt']
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
+config.early_tests = { "test2.txt" }

llvm/utils/lit/tests/Inputs/early-tests/test1.txt

+# RUN: true

llvm/utils/lit/tests/Inputs/early-tests/test2.txt

+# RUN: true

llvm/utils/lit/tests/early-tests.py

+# Check that we can run tests early.
+
+# RUN: %{lit} -j1 %{inputs}/early-tests | FileCheck %s
+
+# CHECK:     -- Testing: 2 tests, 1 workers --
+# CHECK-NEXT: PASS: early-tests :: test2.txt
+# CHECK-NEXT: PASS: early-tests :: test1.txt
+# CHECK:     Passed: 2