[PATCH] D97046: [lit] Generalize `early_tests`

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki added a comment.

In D97046#2607752 <https://reviews.llvm.org/D97046#2607752>, @yln wrote:

> @lebedev.ri
> I don't think we need a RFC for this as long as nothing changes for people who don't use the early_tests or new, generalized ordering feature.  We shouldn't make Dave pay for past debts. (early_tests was introduced 5 years ago!)
>
> @davezarzycki
> Thanks for explaining so patiently!
>
> I now understand that there are two separate, but related features: record-and-reorder, and manually declared test phases.  Let me summarize my understanding so you can check it.
>
> **record-and-reorder**:
> The sole goal here is to optimize execution time.  lit can do this transparently, so maybe it would even make a good default so everyone can benefit. Making it the default is maybe something that benefit from an RFC so we can learn about people's concerns.
> The one detail I want to push back on is that Dave argued that this would not apply to clean/full builds.  Assume we store the order into a file.  With enough motivation there is certainly a way to configure CI bots not to delete this file (or place it somewhere where it wouldn't be deleted).
>
> **manually-declared order**:
> Multiple goals: optimize execution time, execute high/low value tests first/last.  Any other reasons to control order?
> Although it is unfortunate that we have some overlap with `--incremental` here, I get that it doesn't accomplish all of these goals at the same time.
>
> For me, the question is now whether the additional utility of "manually-declared order" justifies having it or if we should just got with "record-and-reorder".  I think Dave is trying to make the argument that the answer here is "yes".  I can see his arguments and would be happy to sign-off on an implementation, especially if the underlying machinery could be re-used to implement the other.
>
> Design-wise I would prefer a solution that directly states test path prefixes, not "test phases".  I think your sketch already did this, but let me restate:
>
>   // simply a list or dict of `path prefix -> priority`
>   config.test_order = [
>   'very/slow/test.c',
>   'directory/of/slow-tests/',
>   '<magic-value-for_all-other-tests>',  // name just for demonstration purposes ;)
>   'directory/of/low-value-tests/',
>   ]
>
> In lit configs, prepending to this list would mean "execute first", appending would mean "execute last".
> And I imagine that "record-and-reorder" could be implemented (in a separate effort) on top of this by expanding `<magic-value-for_all-other-tests>` with the recorded order. (This would mean that the manually declared order is always respected.)
>
> Let me know what you think

Hi @yln,

My opinions and approach to this change request have evolved as the conversation has progressed. At this point, I've come around to the record-and-reorder approach. It's more work and it's not ideal for every scenario that I care about but it's what I want most of the time. Also, record-and-reorder seems to excite people more in the conversation so far; and the hinting "start phase" based approach conflates performance goals with testing prioritization in general, which confuses people.

I'm strongly considering abandoning this change in favor of a new change proposal with the record-and-reorder approach.

And if people want to simplify `lit` then I think a case could be made to rip out all of the code to manually run tests earlier.

Thoughts?

Dave


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97046