moves tests into directories matching their stable names so that the
tests can reflect the concept name
Details
- Reviewers
ldionne EricWF • Quuxplusone Mordante curdeius zoecarver - Group Reviewers
Restricted Project - Commits
- rG24dd2d2f9e27: [libcxx] rearranges all concept tests
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
I'm fine with this but I would like to see CI passing. Is this based on top of another patch that isn't in main? Also, if we agree that we don't like .compile.pass.cpp tests, I think we should get rid of that test format completely. I must admit I agree with @EricWF's comment about .compile.pass.cpp tests potentially causing runtime tests to be skipped.
FWIW, we use a convention that main in a compile-only test is annotated (int main() {} // COMPILE-ONLY) to avoid such problems. This convention predates us actually having technology in our test runners to support compile-only tests, or we'd probably be using the superior int main() { return 1; } // COMPILE-ONLY. (Hmmm, I may go make a PR for that change right now.)
I won't oppose this patch, but FWIW, I would rather keep the .compile.pass.cpp tests and apply a comment as @CaseyCarter suggested. If there's a comment at the end of the function, I don't think anyone will accidentally modify it (and if they do, it will be easy to catch in review). Then we don't have to slow down the test suite (actually, it would be super interesting to see how much slower the test suite is after this change).
libcxx/test/std/concepts/concepts.callable/concept.regularinvocable/regular_invocable.pass.cpp | ||
---|---|---|
20 | What do you think about doing this in two patches?
|
@zoecarver wrote:
If there's a comment at the end of the function, I don't think anyone will accidentally modify it (and if they do, it will be easy to catch in review).
The failure mode to defend against is "someone creates a new .compile.pass.cpp test (maybe brain fart, or maybe because they think that's just the naming convention), without adding such a comment; and then adds runtime assertions to it." But suppose we have no .compile.pass.cpp tests checked in, so that nobody ever creates a new one by accident (because there's no old ones to copy); then that failure mode goes away forever. Also, any need to explain the difference to newbies also goes away. I think that getting into that world would be worth some (but not an arbitrarily large) buildbot slowdown.
It might be interesting for someone to measure the slowdown from globally changing all .compile.pass.cpp tests into .pass.cpp tests.
libcxx/test/std/concepts/concepts.callable/concept.regularinvocable/regular_invocable.pass.cpp | ||
---|---|---|
20 | What's the motivation? I'm not against it, just want to make sure the effort is justified. |
The original motivation for .compile.pass.cpp is to avoid running the tests when it's not needed. It doesn't actually matter in any current configuration we test in the CI, but it does matter a bit more when the executor is slow, for example when running the tests through SSH or when running on an emulator for some embedded chip. I still think it would be reasonable to get rid of them for the reasons explained by others, but I wanted to clarify what the actual purpose of that test format is.
Thanks for separating the patches. Now it's super easy to verify correctness. This LGTM (sans the comment about functions.h).
libcxx/test/std/concepts/callable/invocable.compile.pass.cpp | ||
---|---|---|
21 | Where else is functions.h used? Maybe we should move it into this directory. Or maybe in the support folder? |
Where else is functions.h used? Maybe we should move it into this directory. Or maybe in the support folder?