From 298b5b31caaa613259d0a1c56a299e8e523fd61a Mon Sep 17 00:00:00 2001 From: Craig Scott Date: Sun, 15 Jan 2017 13:06:14 +1100 Subject: CTest: Ensure setup/cleanup ordering even when fixture not required Closes: #16558 --- Source/CTest/cmCTestTestHandler.cxx | 82 +++++++++++++++++------- Tests/RunCMake/ctest_fixtures/CMakeLists.txt.in | 6 ++ Tests/RunCMake/ctest_fixtures/RunCMakeTest.cmake | 1 + Tests/RunCMake/ctest_fixtures/unused-stdout.txt | 9 +++ 4 files changed, 76 insertions(+), 22 deletions(-) create mode 100644 Tests/RunCMake/ctest_fixtures/unused-stdout.txt diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx index 4e63654..6175e50 100644 --- a/Source/CTest/cmCTestTestHandler.cxx +++ b/Source/CTest/cmCTestTestHandler.cxx @@ -829,23 +829,31 @@ void cmCTestTestHandler::UpdateForFixtures(ListOfTests& tests) const addedTests.insert(p.Name); } - // This is a lookup of fixture name to a list of indices into the - // final tests array for tests which require that fixture. It is - // needed at the end to populate dependencies of the cleanup tests - // in our final list of tests. + // These are lookups of fixture name to a list of indices into the final + // tests array for tests which require that fixture and tests which are + // setups for that fixture. They are needed at the end to populate + // dependencies of the cleanup tests in our final list of tests. std::map > fixtureRequirements; + std::map > setupFixturesAdded; // Use integer index for iteration because we append to // the tests vector as we go size_t fixtureTestsAdded = 0; std::set addedFixtures; for (size_t i = 0; i < tests.size(); ++i) { - if (tests[i].FixturesRequired.empty()) { - continue; - } - // Must copy the set of fixtures because we may invalidate + // There are two things to do for each test: + // 1. For every fixture required by this test, record that fixture as + // being required and create dependencies on that fixture's setup + // tests. + // 2. Record all setup tests in the final test list so we can later make + // cleanup tests in the test list depend on their associated setup + // tests to enforce correct ordering. + + // 1. Handle fixture requirements + // + // Must copy the set of fixtures required because we may invalidate // the tests array by appending to it - const std::set fixtures = tests[i].FixturesRequired; + std::set fixtures = tests[i].FixturesRequired; for (std::set::const_iterator fixturesIt = fixtures.begin(); fixturesIt != fixtures.end(); ++fixturesIt) { @@ -908,32 +916,62 @@ void cmCTestTestHandler::UpdateForFixtures(ListOfTests& tests) const this->Quiet); } } + + // 2. Record all setup fixtures included in the final list of tests + for (std::set::const_iterator fixturesIt = + tests[i].FixturesSetup.begin(); + fixturesIt != tests[i].FixturesSetup.end(); ++fixturesIt) { + + const std::string& setupFixtureName = *fixturesIt; + if (setupFixtureName.empty()) { + continue; + } + + setupFixturesAdded[setupFixtureName].push_back(i); + } } // Now that we have the final list of tests, we can update all cleanup - // tests to depend on those tests which require that fixture + // tests to depend on those tests which require that fixture and on any + // setup tests for that fixture. The latter is required to handle the + // pathological case where setup and cleanup tests are in the test set + // but no other test has that fixture as a requirement. for (ListOfTests::iterator tIt = tests.begin(); tIt != tests.end(); ++tIt) { cmCTestTestProperties& p = *tIt; const std::set& cleanups = p.FixturesCleanup; for (std::set::const_iterator fIt = cleanups.begin(); fIt != cleanups.end(); ++fIt) { const std::string& fixture = *fIt; + + // This cleanup test could be part of the original test list that was + // passed in. It is then possible that no other test requires the + // fIt fixture, so we have to check for this. std::map >::const_iterator cIt = fixtureRequirements.find(fixture); - if (cIt == fixtureRequirements.end()) { - // No test cases require the fixture this cleanup test is for. - // This cleanup test must have been part of the original test - // list passed in (which is not an error) - continue; + if (cIt != fixtureRequirements.end()) { + const std::vector& indices = cIt->second; + for (std::vector::const_iterator indexIt = indices.begin(); + indexIt != indices.end(); ++indexIt) { + const std::string& reqTestName = tests[*indexIt].Name; + if (std::find(p.Depends.begin(), p.Depends.end(), reqTestName) == + p.Depends.end()) { + p.Depends.push_back(reqTestName); + } + } } - const std::vector& indices = cIt->second; - for (std::vector::const_iterator indexIt = indices.begin(); - indexIt != indices.end(); ++indexIt) { - const std::string& reqTestName = tests[*indexIt].Name; - if (std::find(p.Depends.begin(), p.Depends.end(), reqTestName) == - p.Depends.end()) { - p.Depends.push_back(reqTestName); + // Ensure fixture cleanup tests always run after their setup tests, even + // if no other test cases require the fixture + cIt = setupFixturesAdded.find(fixture); + if (cIt != setupFixturesAdded.end()) { + const std::vector& indices = cIt->second; + for (std::vector::const_iterator indexIt = indices.begin(); + indexIt != indices.end(); ++indexIt) { + const std::string& setupTestName = tests[*indexIt].Name; + if (std::find(p.Depends.begin(), p.Depends.end(), setupTestName) == + p.Depends.end()) { + p.Depends.push_back(setupTestName); + } } } } diff --git a/Tests/RunCMake/ctest_fixtures/CMakeLists.txt.in b/Tests/RunCMake/ctest_fixtures/CMakeLists.txt.in index ba1c77a..ab50fdd 100644 --- a/Tests/RunCMake/ctest_fixtures/CMakeLists.txt.in +++ b/Tests/RunCMake/ctest_fixtures/CMakeLists.txt.in @@ -30,6 +30,8 @@ failTest(setupFails) # 9 passTest(wontRun) # 10 passTest(cyclicSetup) # 11 passTest(cyclicCleanup) # 12 +passTest(cleanupUnused) # 13 +passTest(setupUnused) # 14 # Define fixture dependencies and ordering set_tests_properties(setupFoo PROPERTIES FIXTURES_SETUP "Foo") @@ -50,6 +52,9 @@ set_tests_properties(two PROPERTIES FIXTURES_REQUIRED "Bar") set_tests_properties(three PROPERTIES FIXTURES_REQUIRED "Meta;Bar") set_tests_properties(wontRun PROPERTIES FIXTURES_REQUIRED "Fails") +set_tests_properties(cleanupUnused PROPERTIES FIXTURES_CLEANUP "Unused") +set_tests_properties(setupUnused PROPERTIES FIXTURES_SETUP "Unused") + @CASE_CMAKELISTS_CYCLIC_CODE@ # These are the cases verified by the main cmake build @@ -62,6 +67,7 @@ set_tests_properties(wontRun PROPERTIES FIXTURES_REQUIRED "Fails") # wontRun 9, 10 # cyclicSetup -NA- (configure fails) # cyclicCleanup -NA- (configure fails) +# unused 14, 13 # # In the case of asking for just setupFoo, since there are # no tests using the Foo fixture, we do NOT expect cleanupFoo diff --git a/Tests/RunCMake/ctest_fixtures/RunCMakeTest.cmake b/Tests/RunCMake/ctest_fixtures/RunCMakeTest.cmake index f13289a..673cf57 100644 --- a/Tests/RunCMake/ctest_fixtures/RunCMakeTest.cmake +++ b/Tests/RunCMake/ctest_fixtures/RunCMakeTest.cmake @@ -17,6 +17,7 @@ run_ctest_test(two INCLUDE two) run_ctest_test(three INCLUDE three) run_ctest_test(setupFoo INCLUDE setupFoo) run_ctest_test(wontRun INCLUDE wontRun) +run_ctest_test(unused INCLUDE Unused) #------------------------------------------------------------ # CMake configure will fail due to cyclic test dependencies diff --git a/Tests/RunCMake/ctest_fixtures/unused-stdout.txt b/Tests/RunCMake/ctest_fixtures/unused-stdout.txt new file mode 100644 index 0000000..b04a621 --- /dev/null +++ b/Tests/RunCMake/ctest_fixtures/unused-stdout.txt @@ -0,0 +1,9 @@ +Test project .*/Tests/RunCMake/ctest_fixtures/unused-build + Start 14: setupUnused +1/2 Test #14: setupUnused +\.+ +Passed +[0-9.]+ sec + Start 13: cleanupUnused +2/2 Test #13: cleanupUnused +\.+ +Passed +[0-9.]+ sec ++ +100% tests passed, 0 tests failed out of 2 ++ +Total Test time \(real\) = +[0-9.]+ sec$ -- cgit v0.12