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

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

jdenny added inline comments.


================
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
----------------
mgorny wrote:
> jdenny wrote:
> > jdenny wrote:
> > > 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.
> > > 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?
> 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.
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).


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

https://reviews.llvm.org/D107695