[PATCH] D96594: [lit] Add "slowest_tests" config option

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

jdenny added a comment.

In D96594#2561643 <https://reviews.llvm.org/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.



================
Comment at: llvm/utils/lit/lit/Test.py:407
         """
+        if '/'.join(self.path_in_suite) in self.suite.config.slowest_tests:
+            return True
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96594