[PATCH] D55834: [libcxx] Make sure all experimental tests are disabled when enable_experimental=False
Eric Fiselier via Phabricator
reviews at reviews.llvm.org
Wed Dec 19 09:49:36 PST 2018
EricWF added a comment.
In D55834#1334932 <https://reviews.llvm.org/D55834#1334932>, @ldionne wrote:
> In D55834#1334839 <https://reviews.llvm.org/D55834#1334839>, @EricWF wrote:
> > 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.
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.
CHANGES SINCE LAST ACTION
More information about the libcxx-commits