[PATCH] D107695: [llvm] [lit] Support forcing lexical test order

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

Thanks for continuing to work on all this.

In D107695#2932775 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D107695#2932760>, @jdenny wrote:

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

Please don't forget this.



================
Comment at: llvm/utils/lit/lit/cl_arguments.py:154
             type=_positive_int)
+    selection_group.add_argument("--default-order",
+            dest="order",
----------------
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?


================
Comment at: llvm/utils/lit/tests/lit.cfg:64
 config.substitutions.append(('%{lit}',
-    "{env} %{{python}} {lit}".format(
+    "{env} %{{python}} {lit} --lexical-order".format(
         env="env -u FILECHECK_OPTS",
----------------
I think it would be helpful to extend the above comments to explain why `--lexical-order` is needed.


================
Comment at: llvm/utils/lit/tests/reorder.py:4
 # 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
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107695/new/

https://reviews.llvm.org/D107695