Page MenuHomePhabricator

[libcxx] Make sure all experimental tests are disabled when enable_experimental=False
ClosedPublic

Authored by ldionne on Dec 18 2018, 9:45 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.Dec 18 2018, 9:45 AM

If the tests are header only, why disable them? We are losing code coverage on things people may be depending on.

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?

mclow.lists accepted this revision.Dec 18 2018, 3:23 PM

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.

This revision is now accepted and ready to land.Dec 18 2018, 3:23 PM

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?

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.

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?

I think you're missing the point of the feature. It's set by CMake when the user disables the experimental library build.

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?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2019, 3:24 AM