I’m wondering if we might be able to change the default value of the new DISCOVERY_MODE option for gtest_discover_tests(), added in the CMake 3.18 release. We are still in release candidate stage, so it’s not too late to consider it. At the moment, the default is POST_BUILD, which maps to the existing behaviour. The other valid value is PRE_TEST, which delays discovery until ctest actually runs. There are robustness benefits to PRE_TEST, as explained in the description of the merge request that added this feature.
Ordinarily, we would provide a policy for any change in behaviour, but I’m really wondering if that would be necessary here. My rationale is that the test must be runnable at test time, so we know it should be possible to extract the test list from it then. That means if it worked the old way, it should still work with the new way. The only way you should legitimately be obtaining the list of tests from a POST_BUILD method would be to ask ctest to tell you, so PRE_TEST will also still work for that scenario. Basically, I don’t think we are losing any observable behavior, just gaining more robustness. Seems like making the default more robust would be a good idea.
Also note that the default can be changed globally with the CMAKE_GTEST_DISCOVER_TESTS_DISCOVERY_MODE variable, so there’s an easy way to get the old behavior if someone has a strange corner case (likely an unsupported scenario) and they genuinely need it. I can’t think of such a case, but my point is that even if there is, there’s a trivial way to get the old behavior back.
I also just realised that we don’t mention CMAKE_GTEST_DISCOVER_TESTS_DISCOVERY_MODE in the release notes. Maybe we should mention it there.
@craig.scott I think that’s worth a try. Please open a 3.18-bound merge request to change the default (and update the docs). If a problem comes up we can just change the default back during the 3.18 series.
I’m ok with changing the default behavior to PRE_TEST. The PRE_TEST behavior is also arguably more correct for multi-config generators anyway (i.e. each test configuration gets its own unique list of enabled/disabled tests). See commit for more details.
But since the behavior is different between PRE_TEST and POST_BUILD, we opted to keep the existing behavior the default. Not because the old behavior was better, just because it was first
The only real observable difference between the two, that I can tell, is tests that would timeout in POST_BUILD would result in a link failure in the parent project. Again, not sure why this behavior would be desirable, but it is another notable difference.
As I’m implementing this, it is feeling more like we should remove the DISCOVERY_MODE option and the CMAKE_GTEST_DISCOVER_TEST_DISCOVERY_MODE variable and instead use a policy. But unlike the usual practice for policies, we would be looking to make this policy default to NEW unilaterally and only use the OLD behavior if CMAKE_POLICY_DEFAULT_CMPxxxx told us to. That would be using existing mechanisms and avoids introducing a new keyword and new variable that no-one should really be using anyway. The CMAKE_POLICY_DEFAULT_CMPxxxx variable already exists and seems well-suited to the sort of last-ditch workaround/override that we are looking for here. Thoughts?
Or we could just let the policy behave in the normal way and let its default be determined by the usual cmake_minimum_required() or cmake_policy() calls. Upon reflection, this might be more consistent overall.
If the default is NEW then it doesn’t need to be a policy. If this were done as a policy, it would have the normal default-OLD behavior and users could set CMAKE_POLICY_DEFAULT_CMPxxxx to enable it on projects that have not been updated.
It’s too late in the 3.18 release cycle to add a policy now. Options to proceed include:
Revert the whole change and re-introduce it for 3.19 with a policy.
Remove the option and simply change the implementation to the new approach. If problems come up we can always revert for 3.18 later.
Leave the change as-is for 3.18 and add a policy in 3.19 to change the default DISCOVERY_MODE to the new mode.
Of those, I think option 3 strikes the best balance. It makes the feature available for 3.18 for those who need it. Keeping the existing behavior by default minimises risk of a regression if we’ve missed some corner case and adding a policy to change it in 3.19 retains a workaround if a regression emerges later. I’ll put up a MR with some minor cleanup and missing mention of CMAKE_GTEST_DISCOVER_TESTS_DISCOVERY_MODE in the release notes shortly. The addition of the policy can happen later.
