[PATCH] D97046: [lit] Add "late_tests" test suite config option

Thu Feb 25 05:15:12 PST 2021

davezarzycki added a comment.

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

> I also want to state that I am a bit concerned about feature creep and the the general usefulness of this effort.
>
> - Does shaving off 5% of test time for one project justify having it in upstream?

My colleagues with slower machines are excited by the ability to push low value tests to the end. So it's not just about performance.

> Think added complexity, maintenance and opportunity cost. It is difficult to decide where to draw the line and I will not block this effort, but I wanted to see where other people stand on this?

"late_tests" shouldn't be any more complicated than "early_tests" and they should share implementation details. To me, deciding that one is good and the other is bad would be hard to defend.

> - Are there workarounds we have considered? An ugly one would be naming directories to influence order.

Lexical hacks were considered during a discussion with my Swift colleagues but it really doesn't scale. People want to organize the test suite into logical groupings, not by testing order.

> - Annotation of fast and slow tests is manual and needs to be maintained.  These things tend to rot over time and the benefit will decrease.

Ahem. Nothing bad will happen if the lists are stale, therefore they don't "need" maintenance. And people that care about test ordering will keep the lists up to date. Please understand, I've tried to design the "early_tests" / "late_tests" feature with actual test suites in mind to keep the maintenance burden as low as possible. In my experience, the early/late tests for a given project tend to be fairly stable over time. Also (and in my experience), programmers aren't motivated to fix slow tests unless they're caught during pre-commit/post-commit review, so again, the list is very stable.

> Is the sole goal with early/late tests to optimize execution time or are there any other semantics attached to it?  Dave mentioned that the tests he has in mind are also low reward.  My understanding is that this means that they "rarely fail/signal an issue that's not shown by other test".

As I wrote earlier, performance is not the sole goal.

> For quick feedback, we have `--incremental` (poor name), which runs modified and failed tests first, i.e., which is an approximation of "run high reward tests first".

I'm not sure what your point is here. The `--incremental` feature is only useful if one is doing incremental builds (not everybody does, for example: bots and paranoid people). Also, the `--incremental` feature isn't trying to improve the overall test time. Finally, the `--incremental` feature is poorly implemented and it doesn't work in rigorous testing environments.

> If we want to optimize overall execution time, maybe we should track test execution times and then run tests in decreasing order the next time around.  This would have the benefit that it doesn't need manual annotation (which may rot over time).  Maybe it should even be the default?  Not to be addressed in this review of course, but I wanted to raise my points.

Again, you're assuming incremental development and yes that'd be a great feature for people doing incremental development. But clean build performance matters too. I don't think asking developers to commit a sorted list of tests to a project to improve clean build performance is worth the time and effort for most project maintainers.

To repeat myself, I tried to design the "early_tests" and "late_tests" feature to be in the sweet spot of trade offs for all involved.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97046