[llvm] [lit] Support forcing lexical test order

[mgorny]

Summary

Add a new --order option to choose between available test orders:
the default "smart" order, predictable "lexical" order or "random"
order. Default to using lexical order and one job in the lit test
suite.

[jdenny]

Thanks for working on this!

The patch doesn't spell it consistently: sometimes --lexical and sometimes --lexical-order.

Shouldn't we put --lexical-order directly into lit's test suite's %{lit} substitution? That way, if another of lit's current or future tests is susceptible to this problem, no one has to waste time trying to reproduce the race and figure out what's happening. Blindly using %{lit} in lit's test suite would just work.

As far as I know, reorder.py is the only test in lit's test suite that requires the reordering. Either it can use a %{lit-reordered} substitution that drops --lexical-order, or it can use a new command-line option that selects the default order and overrides any previous --lexical-order or --shuffle.

llvm/utils/lit/tests/shtest-shell.py

10–11

Any reason to keep this? It might be worthwhile to grep for lit_test_times, if you didn't already.

[jdenny]

Also, the docs should be updated: https://llvm.org/docs/CommandGuide/lit.html

[mgorny]

In D107695#2932751, @jdenny wrote:

Thanks for working on this!

The patch doesn't spell it consistently: sometimes --lexical and sometimes --lexical-order.

Sorry, I've forgotten how I named it :-D.

Shouldn't we put --lexical-order directly into lit's test suite's %{lit} substitution? That way, if another of lit's current or future tests is susceptible to this problem, no one has to waste time trying to reproduce the race and figure out what's happening. Blindly using %{lit} in lit's test suite would just work.

I don't mind that. However, should we also put -j 1 then?

As far as I know, reorder.py is the only test in lit's test suite that requires the reordering. Either it can use a %{lit-reordered} substitution that drops --lexical-order, or it can use a new command-line option that selects the default order and overrides any previous --lexical-order or --shuffle.

I suppose I could do that, and then replace the mutually exclusive group approach with 'last option wins'.

[mgorny]

llvm/utils/lit/tests/shtest-shell.py

10–11

I was grepping for order FIXMEs ;-). Fixed now.

[mgorny]

Add --default-order. Last option specified takes precedence. Include --lexical-order in %{lit} substitution.

[jdenny]

Thanks for continuing to work on all this.

In D107695#2932775, @mgorny wrote:

Shouldn't we put --lexical-order directly into lit's test suite's %{lit} substitution? That way, if another of lit's current or future tests is susceptible to this problem, no one has to waste time trying to reproduce the race and figure out what's happening. Blindly using %{lit} in lit's test suite would just work.

I don't mind that. However, should we also put -j 1 then?

Good point. I haven't looked to see whether the default value of -j is ever expected in lit's test suite. Anyway, maybe that should be a different patch to make reviewing/debugging easier.

In D107695#2932760, @jdenny wrote:

Also, the docs should be updated: https://llvm.org/docs/CommandGuide/lit.html

Please don't forget this.

llvm/utils/lit/lit/cl_arguments.py

154

Instead of --default-order, I'd prefer something that describes the order. I'm not sure of the best name, especially given that it's always possible we'll think of other tweaks to the default order in the future. Perhaps something like --auto-order or --smart-order to at least suggest it's something clever. What do you think?

llvm/utils/lit/tests/lit.cfg

64

I think it would be helpful to extend the above comments to explain why --lexical-order is needed.

llvm/utils/lit/tests/reorder.py

4

As far as I can tell, we now have no test that --default-order is actually the default. Sorry, I didn't think of that when I made that suggestion. If we ever broke that behavior (for example, someone could reorganize the arg specification in a subtly broken way), I wonder how long it would be before anyone realized that test suites started taking much longer than necessary.

What if we have something like a %{lit-no-order-opt} that doesn't specify any order options? reorder.py could then run lit twice, once to ensure --default-order is indeed the default, and once to ensure it overrides --lexical-order and --shuffle when specified afterward.

I guess that means --default-order isn't so useful in lit's own test suite. It's just another feature to test. However, as I mentioned in D107427, I'd like to use it in clang/test/utils too for testing test-suite-generation scripts.

[jdenny]

llvm/utils/lit/tests/reorder.py

4

I guess that means --default-order isn't so useful in lit's own test suite. It's just another feature to test. However, as I mentioned in D107427, I'd like to use it in clang/test/utils too for testing test-suite-generation scripts.

Well, that actually makes no sense. I want --lexical-order there not --default-order.

Hmm. I'm not opposed to keeping something like --default-order for completeness, but I'm now having trouble seeing a real use case. What do others think?

[mgorny]

llvm/utils/lit/tests/reorder.py

4

To be honest, I don't really see a use case for testing which order is the default. After all, setting the default order is a matter of changing the parameter to default=… A test for this sounds like asking people to change the default in two places.

[jdenny]

llvm/utils/lit/tests/reorder.py

4

I don't agree with the argument that a behavior whose current implementation looks simple doesn't need a regression test, and I've previously found that the apparent simplicity of an argparse config can be deceptive. If we're going to implement these options, then I think we should test them fully, especially that the intended default behavior is actually the default. If the default ever changes accidentally, it could quietly affect many people. Moreover, testing it is straight-forward: we just need another lit substitution to use in reorder.py.

After further thought, I do think having a different option for each possible order is likely to prove useful, so I vote we keep --default-order (or whatever we end up calling it).

[yln]

Thanks for working on this! :)

llvm/utils/lit/lit/cl_arguments.py

14

While we are at it, can we also give DEFAULT a more descriptive name here?

Order predicate: (not t.previous_failure, -t.previous_elapsed, t.getFullName())

1. Failures first
2. Longest tests first
3. Then by name

Since putting all of the above in an enum name is quite a mouthful, I am advocating for smart as suggested by Joel.

Together with my other comment (see below) the help text could then be:
--order=(smart|aaa|bbb|...) [default: smart]

This would align the names used for options and names in the code.

154

I would also prefer descriptive names instead of "default".

If we already put the effort i to improve ordering, how about: --order=(aaa|bbb|...) and make the old ones aliases for the new ones: --shuffle --> --order=random.

I think argparse spells out all possible options and even shows which one is the default when printing the help: --order=(smart|aaa|bbb|...) [default: smart]

So it shows the user the available options, which option is the default, and the explanation of these options is right next to it.
Having all options documented right next to each other also improves discoverability and makes contrasting the differences between options easier.

[yln]

llvm/utils/lit/lit/cl_arguments.py

154

If it wasn't clear, I am advocating for using parser.add_argument('--order', choices=['smart', aaa', 'bbb']) here.
https://docs.python.org/3/library/argparse.html#choices

[mgorny]

Switched to choice-based --order= option. Renamed 'default' to 'smart'. Added a new lit subst not to include order args. Made %{lit} include -j1 as well. Added the comment.

I think this addresses all the comments.

I have also removed the enum since it seemed to make little sense now... but I have a new idea, so I might reintroduce it in a minute ;-).

[mgorny]

Restore the TestOrder enum. It doesn't change much logically but gives some abstraction that'll hopefully make uncaught typos less likely in the future.

[jdenny]

--order is nice.

Also, the docs should be updated: https://llvm.org/docs/CommandGuide/lit.html

This is still needed.

[mgorny]

Updated docs.

[mgorny]

Ping.

[jdenny]

LGTM. Thanks.

[MatzeB]

Hey, I don't know if this has anything to do with this change.

But I see the "llvm-clang-x86_64-sie-win" buildbot (https://lab.llvm.org/buildbot/#/builders/216) failing the reorder.py testcase randomly every 10-20 builds.

See https://lab.llvm.org/buildbot/#/builders/216 for the list of recent builds.

I didn't look deeply into the root cause here, but thought it would be good to ping here in case anyone has an idea to get the bot more stable.

[dyung]

In D107695#3617159, @MatzeB wrote:

Hey, I don't know if this has anything to do with this change.

But I see the "llvm-clang-x86_64-sie-win" buildbot (https://lab.llvm.org/buildbot/#/builders/216) failing the reorder.py testcase randomly every 10-20 builds.

See https://lab.llvm.org/buildbot/#/builders/216 for the list of recent builds.

I didn't look deeply into the root cause here, but thought it would be good to ping here in case anyone has an idea to get the bot more stable.

I'm the owner of that buildbot, but unfortunately have not been able to get the failure to reproduce outside of the buildbot initiated build environment, so I haven't been able to really look into the issue. Any ideas how to possibly debug and/or fix the issue would be greatly appreciated as I've been a bit stuck on how to get it more stable!

[ychen]

In D107695#3620822, @dyung wrote:

In D107695#3617159, @MatzeB wrote:

Hey, I don't know if this has anything to do with this change.

But I see the "llvm-clang-x86_64-sie-win" buildbot (https://lab.llvm.org/buildbot/#/builders/216) failing the reorder.py testcase randomly every 10-20 builds.

See https://lab.llvm.org/buildbot/#/builders/216 for the list of recent builds.

I didn't look deeply into the root cause here, but thought it would be good to ping here in case anyone has an idea to get the bot more stable.

I'm the owner of that buildbot, but unfortunately have not been able to get the failure to reproduce outside of the buildbot initiated build environment, so I haven't been able to really look into the issue. Any ideas how to possibly debug and/or fix the issue would be greatly appreciated as I've been a bit stuck on how to get it more stable!

It's kind of concerning that the test (reorder.py) writes to the source-controlled input file. I'm not sure if using a temporary directory to save intermediate files (like most other tests do) would fix the issue.
Alternatively, as the last resort, I'd suggest temporarily inserting some extra RUN lines in the test to help debug the random failure.

llvm/utils/lit/lit/cl_arguments.py

 import argparse
 import enum
 import os
 import shlex
 import sys
 
 import lit.reports
 import lit.util
 
 
 class TestOrder(enum.Enum):
     DEFAULT = enum.auto()
     RANDOM = enum.auto()
+    LEXICAL = enum.auto()
 
 
 def parse_args():
     parser = argparse.ArgumentParser(prog='lit')
     parser.add_argument('test_paths',
             nargs='+',
             metavar="TEST_PATH",
             help='File or path to include in the test suite')
selection_group.add_argument("--max-tests",
             metavar="N",
             help="Maximum number of tests to run",
             type=_positive_int)
     selection_group.add_argument("--max-time",
             dest="timeout",
             metavar="N",
             help="Maximum time to spend testing (in seconds)",
             type=_positive_int)
+    selection_group.add_argument("--default-order",
+            dest="order",
+            help="Run tests in default order",
+            action="store_const",
+            const=TestOrder.DEFAULT,
+            default=TestOrder.DEFAULT)
+    selection_group.add_argument("--lexical-order",
+            dest="order",
+            help="Run tests in lexical order",
+            action="store_const",
+            const=TestOrder.LEXICAL)
     selection_group.add_argument("--shuffle",
+            dest="order",
             help="Run tests in random order",
-            action="store_true")
+            action="store_const",
+            const=TestOrder.RANDOM)
     selection_group.add_argument("-i", "--incremental",
             help="Run failed tests first (DEPRECATED: now always enabled)",
             action="store_true")
     selection_group.add_argument("--filter",
             metavar="REGEX",
             type=_case_insensitive_regex,
             help="Only run tests with paths matching the given regular expression",
             default=os.environ.get("LIT_FILTER", ".*"))
def parse_args():
 
     # Validate command line options
     if opts.echoAllCommands:
         opts.showOutput = True
 
     if opts.incremental:
         print('WARNING: --incremental is deprecated. Failing tests now always run first.')
 
-    if opts.shuffle:
-        opts.order = TestOrder.RANDOM
-    else:
-        opts.order = TestOrder.DEFAULT
-
     if opts.numShards or opts.runShard:
         if not opts.numShards or not opts.runShard:
             parser.error("--num-shards and --run-shard must be used together")
         if opts.runShard > opts.numShards:
             parser.error("--run-shard must be between 1 and --num-shards (inclusive)")
         opts.shard = (opts.runShard, opts.numShards)
     else:
         opts.shard = None

llvm/utils/lit/lit/main.py

if show_tests:
             print('  %s' % t.getFullName())
 
 
 def determine_order(tests, order):
     from lit.cl_arguments import TestOrder
     if order == TestOrder.RANDOM:
         import random
         random.shuffle(tests)
+    elif order == TestOrder.LEXICAL:
+        tests.sort(key=lambda t: t.getFullName())
     else:
         assert order == TestOrder.DEFAULT, 'Unknown TestOrder value'
         tests.sort(key=lambda t: (not t.previous_failure, -t.previous_elapsed, t.getFullName()))
 
 
 def filter_by_shard(tests, run, shards, lit_config):
     test_ixs = range(run - 1, len(tests), shards)
     selected_tests = [tests[i] for i in test_ixs]

llvm/utils/lit/tests/custom-result-category.py

 # UNSUPPORTED: system-windows
 # Test lit.main.add_result_category() extension API.
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/custom-result-category/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 %{inputs}/custom-result-category | FileCheck %s
 
 # CHECK: CUSTOM_PASS: custom-result-category :: test1.txt
 # CHECK: CUSTOM_FAILURE: custom-result-category :: test2.txt
 
 # CHECK-NOT: My Passed Tests (1)
 # CHECK-NOT:   custom-result-category :: test1.txt
 # CHECK:     My Failed Tests (1)
 # CHECK:       custom-result-category :: test2.txt
 
 # CHECK: My Passed: 1
 # CHECK: My Failed: 1

llvm/utils/lit/tests/googletest-format.py

 # Check the various features of the GoogleTest format.
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/googletest-format/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -v %{inputs}/googletest-format > %t.out
 # FIXME: Temporarily dump test output so we can debug failing tests on
 # buildbots.
 # RUN: cat %t.out
 # RUN: FileCheck < %t.out %s
 #
 # END.
 

llvm/utils/lit/tests/googletest-upstream-format.py

 # Check the various features of the GoogleTest format.
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/googletest-upstream-format/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -v %{inputs}/googletest-upstream-format > %t.out
 # RUN: FileCheck < %t.out %s
 #
 # END.
 
 # CHECK: -- Testing:
 # CHECK: PASS: googletest-upstream-format :: [[PATH:[Dd]ummy[Ss]ub[Dd]ir/]][[FILE:OneTest\.py]]/FirstTest.subTestA
 # CHECK: FAIL: googletest-upstream-format :: [[PATH]][[FILE]]/[[TEST:FirstTest\.subTestB]]

llvm/utils/lit/tests/lit.cfg

 # This test suite calls %{lit} to test lit's behavior for the sample test
 # suites in %{inputs}.  This test suite's results are then determined in part
 # by %{lit}'s textual output, which includes the output of FileCheck calls
 # within %{inputs}'s test suites.  Thus, %{lit} clears environment variables
 # that can affect FileCheck's output.
 config.substitutions.append(('%{inputs}', os.path.join(
     config.test_source_root, 'Inputs')))
 config.substitutions.append(('%{lit}',
-    "{env} %{{python}} {lit}".format(
+    "{env} %{{python}} {lit} --lexical-order".format(
         env="env -u FILECHECK_OPTS",
         lit=os.path.join(lit_path, 'lit.py'))))
 config.substitutions.append(('%{python}', '"%s"' % (sys.executable)))
 
 # Enable coverage.py reporting, assuming the coverage module has been installed
 # and sitecustomize.py in the virtualenv has been modified appropriately.
 if lit_config.params.get('check-coverage', None):
     config.environment['COVERAGE_PROCESS_START'] = os.path.join(

llvm/utils/lit/tests/progress-bar.py

 # Check the simple progress bar.
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/progress-bar/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -s %{inputs}/progress-bar > %t.out
 # RUN: FileCheck < %t.out %s
 #
 # CHECK: Testing:
 # CHECK: FAIL: progress-bar :: test-1.txt (1 of 4)
 # CHECK: Testing:  0.. 10.. 20
 # CHECK: FAIL: progress-bar :: test-2.txt (2 of 4)
 # CHECK: Testing:  0.. 10.. 20.. 30.. 40..
 # CHECK: FAIL: progress-bar :: test-3.txt (3 of 4)
 # CHECK: Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70
 # CHECK: FAIL: progress-bar :: test-4.txt (4 of 4)
 # CHECK: Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..

llvm/utils/lit/tests/reorder.py

 ## Check that we can reorder test runs.
 
 # RUN: cp %{inputs}/reorder/.lit_test_times.txt %{inputs}/reorder/.lit_test_times.txt.orig
-# RUN: %{lit} -j1 %{inputs}/reorder > %t.out
+# RUN: %{lit} --default-order -j1 %{inputs}/reorder > %t.out
 # RUN: cp %{inputs}/reorder/.lit_test_times.txt %{inputs}/reorder/.lit_test_times.txt.new
 # RUN: cp %{inputs}/reorder/.lit_test_times.txt.orig %{inputs}/reorder/.lit_test_times.txt
 # RUN: not diff %{inputs}/reorder/.lit_test_times.txt.new %{inputs}/reorder/.lit_test_times.txt.orig
 # RUN: FileCheck --check-prefix=TIMES --implicit-check-not= < %{inputs}/reorder/.lit_test_times.txt.new %s
 # RUN: FileCheck < %t.out %s
 # END.
 
 # TIMES: not-executed.txt

llvm/utils/lit/tests/shtest-env.py

 # Check the env command
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/shtest-env/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -a -v %{inputs}/shtest-env \
 # RUN: | FileCheck -match-full-lines %s
 #
 # END.
 
 # Make sure env commands are included in printed commands.
 
 # CHECK: -- Testing: 16 tests{{.*}}

llvm/utils/lit/tests/shtest-format.py

 # Check the various features of the ShTest format.
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/shtest-format/.lit_test_times.txt
-
 # RUN: rm -f %t.xml
 # RUN: not %{lit} -j 1 -v %{inputs}/shtest-format --xunit-xml-output %t.xml > %t.out
 # RUN: FileCheck < %t.out %s
 # RUN: FileCheck --check-prefix=XUNIT < %t.xml %s
 
 # END.
 
 # CHECK: -- Testing:

llvm/utils/lit/tests/shtest-keyword-parse-errors.py

-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/shtest-keyword-parse-errors/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -vv %{inputs}/shtest-keyword-parse-errors > %t.out
 # RUN: FileCheck -input-file %t.out %s
 #
 # END.
 
 # CHECK: Testing: 3 tests
 
 # CHECK-LABEL: UNRESOLVED: shtest-keyword-parse-errors :: empty.txt
 # CHECK:       {{^}}Test has no 'RUN:' line{{$}}
 
 # CHECK-LABEL: UNRESOLVED: shtest-keyword-parse-errors :: multiple-allow-retries.txt
 # CHECK:       {{^}}Test has more than one ALLOW_RETRIES lines{{$}}
 
 # CHECK-LABEL: UNRESOLVED: shtest-keyword-parse-errors :: unterminated-run.txt
 # CHECK:       {{^}}Test has unterminated 'RUN:' lines (with '\'){{$}}

llvm/utils/lit/tests/shtest-not.py

 # Check the not command
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/shtest-not/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -a -v %{inputs}/shtest-not \
 # RUN: | FileCheck -match-full-lines %s
 #
 # END.
 
 # Make sure not and env commands are included in printed commands.
 
 # CHECK: -- Testing: 17 tests{{.*}}

llvm/utils/lit/tests/shtest-run-at-line.py

 # Check that -vv makes the line number of the failing RUN command clear.
 # (-v is actually sufficient in the case of the internal shell.)
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/shtest-run-at-line/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -vv %{inputs}/shtest-run-at-line > %t.out
 # RUN: FileCheck --input-file %t.out %s
 #
 # END.
 
 
 # CHECK: Testing: 4 tests
 

llvm/utils/lit/tests/shtest-shell.py

 # Check the internal shell handling component of the ShTest format.
 
-# FIXME: this test depends on order of tests
-# RUN: rm -f %{inputs}/shtest-shell/.lit_test_times.txt
-
 # RUN: not %{lit} -j 1 -v %{inputs}/shtest-shell > %t.out
 # FIXME: Temporarily dump test output so we can debug failing tests on
 # buildbots.
 # RUN: cat %t.out
 # RUN: FileCheck --input-file %t.out %s
 #
 # Test again in non-UTF shell to catch potential errors with python 2 seen
 # on stdout-encoding.txt
-# FIXME: lit's testing sets source_root == exec_root which complicates running lit more than once per test.
-# RUN: rm -f %{inputs}/shtest-shell/.lit_test_times.txt
 # RUN: env PYTHONIOENCODING=ascii not %{lit} -j 1 -a %{inputs}/shtest-shell > %t.ascii.out
 # FIXME: Temporarily dump test output so we can debug failing tests on
 # buildbots.
 # RUN: cat %t.ascii.out
 # RUN: FileCheck --input-file %t.ascii.out %s
 #
 # END.
 
 # CHECK: -- Testing: