Previously, we'd run some experimental tests even when enable_experimental=False
was used with lit.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
If the tests are header only, why disable them? We are losing code coverage on things people may be depending on.
I don't expect most people to be using enable_experimental=False, so that shouldn't actually reduce the test coverage. The problem I hit was running CI on older OS X where a test was failing (because of a problem with exceptions in the old dylib). The experimental part doesn't actually use anything in the dylib, but the test as a whole does, and enabling it when I explicitly say enable_experimental=False was surprising certainly not what I expected.
In other words, I fully agree that we should strive to run experimental tests as often as possible (when it makes sense), and we do have the right default here. But when I explicitly say "don't give me experimental tests", I don't want them to run.
If we'd rather not give that option at all, we could consider inferring the c++experimental LIT feature automatically based on stuff like whether use_system_cxx_lib was used and other things, but I don't think it makes a lot of sense.
WDYT?
In other words, I fully agree that we should strive to run experimental tests as often as possible (when it makes sense), and we do have the right default here. But when I explicitly say "don't give me experimental tests", I don't want them to run.
I'm OK with this.
I think you're missing the point of the feature. It's set by CMake when the user disables the experimental library build. But even then, header only experimental stuff should work; which is what this was testing. Now we have no test coverage that we aren't regressing header only functionality.
You're right. However, where I'm coming from is running the LIT test suite by itself and passing enable_experimental=False. For example, imagine an implementation that does not implement experimental features -- how would such an implementation use the test suite?
Now we have no test coverage that we aren't regressing header only functionality.
I find this statement too strong. We do have coverage by default because the experimental library is built by default. This really only makes a difference if you're explicitly requesting for the experimental library _not_ to be built but would still want to test the code in headers. I find this use case questionable, and of smaller value than having the ability to turn off the experimental tests on their own.
Are you convinced?