CTest: fix regressions introduced by the ctest-fix-run-serial topic

The first regression resulted in endless looping due to unrun test
dependencies. The second regression prioritized all tests with dependencies
in serial test runs.

6 files changed, 179 insertions, 4 deletions

diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx
index 4c39d105b9..729f14a1b4 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.cxx
+++ b/Source/CTest/cmCTestMultiProcessHandler.cxx
@@ -139,6 +139,13 @@ void cmCTestMultiProcessHandler::StartTestProcess(int test)
     }
   else
     {
+
+    for(TestMap::iterator j = this->Tests.begin();
+      j != this->Tests.end(); ++j)
+      {
+      j->second.erase(test);
+      }
+
     this->UnlockResources(test);
     this->Completed++;
     this->TestFinishMap[test] = true;
@@ -441,6 +448,19 @@ int cmCTestMultiProcessHandler::SearchByName(std::string name)
 //---------------------------------------------------------
 void cmCTestMultiProcessHandler::CreateTestCostList()
 {
+  if(this->ParallelLevel > 1)
+    {
+    CreateParallelTestCostList();
+    }
+  else
+    {
+    CreateSerialTestCostList();
+    }
+}
+
+//---------------------------------------------------------
+void cmCTestMultiProcessHandler::CreateParallelTestCostList()
+{
   TestSet alreadySortedTests;
 
   std::list<TestSet> priorityStack;
@@ -452,8 +472,7 @@ void cmCTestMultiProcessHandler::CreateTestCostList()
   for(TestMap::const_iterator i = this->Tests.begin();
     i != this->Tests.end(); ++i)
     {
-    if(this->ParallelLevel > 1 &&
-       std::find(this->LastTestsFailed.begin(), this->LastTestsFailed.end(),
+    if(std::find(this->LastTestsFailed.begin(), this->LastTestsFailed.end(),
        this->Properties[i->first]->Name) != this->LastTestsFailed.end())
       {
       //If the test failed last time, it should be run first.
@@ -466,8 +485,9 @@ void cmCTestMultiProcessHandler::CreateTestCostList()
       }
     }
 
-  // Repeatedly move dependencies of the tests on the current dependency level
-  // to the next level until no further dependencies exist.
+  // In parallel test runs repeatedly move dependencies of the tests on
+  // the current dependency level to the next level until no
+  // further dependencies exist.
   while(priorityStack.back().size())
     {
     TestSet &previousSet = priorityStack.back();
@@ -526,6 +546,65 @@ void cmCTestMultiProcessHandler::CreateTestCostList()
 }
 
 //---------------------------------------------------------
+void cmCTestMultiProcessHandler::GetAllTestDependencies(
+    int test, TestList& dependencies)
+{
+  TestSet const& dependencySet = this->Tests[test];
+  for(TestSet::const_iterator i = dependencySet.begin();
+    i != dependencySet.end(); ++i)
+    {
+    GetAllTestDependencies(*i, dependencies);
+    dependencies.push_back(*i);
+    }
+}
+
+//---------------------------------------------------------
+void cmCTestMultiProcessHandler::CreateSerialTestCostList()
+{
+  TestList presortedList;
+
+  for(TestMap::iterator i = this->Tests.begin();
+    i != this->Tests.end(); ++i)
+    {
+    presortedList.push_back(i->first);
+    }
+
+  TestComparator comp(this);
+  std::stable_sort(presortedList.begin(), presortedList.end(), comp);
+
+  TestSet alreadySortedTests;
+
+  for(TestList::const_iterator i = presortedList.begin();
+    i != presortedList.end(); ++i)
+    {
+      int test = *i;
+
+      if(alreadySortedTests.find(test) != alreadySortedTests.end())
+        {
+        continue;
+        }
+
+      TestList dependencies;
+      GetAllTestDependencies(test, dependencies);
+
+      for(TestList::const_iterator j = dependencies.begin();
+        j != dependencies.end(); ++j)
+        {
+        int testDependency = *j;
+
+        if(alreadySortedTests.find(testDependency) == alreadySortedTests.end())
+          {
+          alreadySortedTests.insert(testDependency);
+          this->SortedTests.push_back(testDependency);
+          }
+        }
+
+      alreadySortedTests.insert(test);
+      this->SortedTests.push_back(test);
+    }
+}
+
+//---------------------------------------------------------
 void cmCTestMultiProcessHandler::WriteCheckpoint(int index)
 {
   std::string fname = this->CTest->GetBinaryDir()
diff --git a/Source/CTest/cmCTestMultiProcessHandler.h b/Source/CTest/cmCTestMultiProcessHandler.h
index 439a8f368b..1b53ec7c2f 100644
--- a/Source/CTest/cmCTestMultiProcessHandler.h
+++ b/Source/CTest/cmCTestMultiProcessHandler.h
@@ -72,6 +72,12 @@ protected:
   int SearchByName(std::string name);
 
   void CreateTestCostList();
+
+  void GetAllTestDependencies(int test, TestList& dependencies);
+  void CreateSerialTestCostList();
+
+  void CreateParallelTestCostList();
+
   // Removes the checkpoint file
   void MarkFinished();
   void EraseTest(int index);
diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt
index 5ea604ffea..063bd2da2f 100644
--- a/Tests/CMakeLists.txt
+++ b/Tests/CMakeLists.txt
@@ -2219,6 +2219,15 @@ ${CMake_BINARY_DIR}/bin/cmake -DVERSION=master -P ${CMake_SOURCE_DIR}/Utilities/
   ADD_TEST_MACRO(CTestTestSerialInDepends ${CMAKE_CTEST_COMMAND} -j 4
     --output-on-failure -C "\${CTestTest_CONFIG}")
 
+  ADD_TEST_MACRO(CTestTestMissingDependsExe ${CMAKE_CTEST_COMMAND}
+    --output-on-failure -C "\${CTestTest_CONFIG}")
+  set_tests_properties(CTestTestMissingDependsExe PROPERTIES
+    PASS_REGULAR_EXPRESSION "\\*\\*\\*Not Run"
+  )
+
+  ADD_TEST_MACRO(CTestTestSerialOrder ${CMAKE_CTEST_COMMAND}
+    --output-on-failure -C "\${CTestTest_CONFIG}")
+
   if(NOT BORLAND)
     set(CTestLimitDashJ_CTEST_OPTIONS --force-new-ctest-process)
     add_test_macro(CTestLimitDashJ ${CMAKE_CTEST_COMMAND} -j 4
diff --git a/Tests/CTestTestMissingDependsExe/CMakeLists.txt b/Tests/CTestTestMissingDependsExe/CMakeLists.txt
new file mode 100644
index 0000000000..9826da661a
--- /dev/null
+++ b/Tests/CTestTestMissingDependsExe/CMakeLists.txt
@@ -0,0 +1,10 @@
+cmake_minimum_required(VERSION 2.8.12)
+
+project(CTestTestMissingDependsExe)
+
+enable_testing()
+
+add_test(test1 ${CMAKE_COMMAND} -E echo test)
+add_test(test2 non-existent-command)
+
+set_tests_properties(test1 PROPERTIES DEPENDS test2)
diff --git a/Tests/CTestTestSerialOrder/CMakeLists.txt b/Tests/CTestTestSerialOrder/CMakeLists.txt
new file mode 100644
index 0000000000..69c11fcde1
--- /dev/null
+++ b/Tests/CTestTestSerialOrder/CMakeLists.txt
@@ -0,0 +1,40 @@
+cmake_minimum_required(VERSION 2.8.12)
+
+project(CTestTestSerialOrder)
+
+set(TEST_OUTPUT_FILE "${CMAKE_CURRENT_BINARY_DIR}/test_output.txt")
+
+enable_testing()
+
+function(add_serial_order_test TEST_NAME)
+  add_test(NAME ${TEST_NAME}
+    COMMAND ${CMAKE_COMMAND}
+      "-DTEST_OUTPUT_FILE=${TEST_OUTPUT_FILE}"
+      "-DTEST_NAME=${TEST_NAME}"
+      -P "${CMAKE_CURRENT_SOURCE_DIR}/test.cmake"
+  )
+
+  if(ARGC GREATER 1)
+    set_tests_properties(${TEST_NAME} PROPERTIES ${ARGN})
+  endif()
+endfunction()
+
+add_serial_order_test(initialization COST 1000)
+add_serial_order_test(test1)
+add_serial_order_test(test2)
+add_serial_order_test(test3)
+add_serial_order_test(test4 DEPENDS test5)
+
+add_serial_order_test(test5)
+set_tests_properties(test5 PROPERTIES DEPENDS "test6;test7b;test7a")
+
+add_serial_order_test(test6 COST -2)
+add_serial_order_test(test7a COST -1)
+add_serial_order_test(test7b COST -1)
+add_serial_order_test(test8 COST 10)
+add_serial_order_test(test9 COST 20)
+add_serial_order_test(test10 COST 0)
+add_serial_order_test(test11)
+add_serial_order_test(test12 COST 0)
+
+add_serial_order_test(verification COST -1000)
diff --git a/Tests/CTestTestSerialOrder/test.cmake b/Tests/CTestTestSerialOrder/test.cmake
new file mode 100644
index 0000000000..8479cae953
--- /dev/null
+++ b/Tests/CTestTestSerialOrder/test.cmake
@@ -0,0 +1,31 @@
+list(APPEND EXPECTED_OUTPUT
+  initialization
+  test9
+  test8
+  test1
+  test2
+  test3
+  test6
+  test7a
+  test7b
+  test5
+  test4
+  test10
+  test11
+  test12
+)
+
+
+if("${TEST_NAME}" STREQUAL "initialization")
+  file(WRITE ${TEST_OUTPUT_FILE} "${TEST_NAME}")
+
+elseif("${TEST_NAME}" STREQUAL "verification")
+  file(READ ${TEST_OUTPUT_FILE} ACTUAL_OUTPUT)
+  if(NOT "${ACTUAL_OUTPUT}" STREQUAL "${EXPECTED_OUTPUT}")
+    message(FATAL_ERROR "Actual test order [${ACTUAL_OUTPUT}] differs from expected test order [${EXPECTED_OUTPUT}]")
+  endif()
+
+else()
+  file(APPEND ${TEST_OUTPUT_FILE} ";${TEST_NAME}")
+
+endif()