Fix coverage clear bug and other minor refactors

Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp

@@ -51,7 +51,7 @@ namespace TestImpact
             Ignore,
             StdOut,
             File,
-            Remove,
+            Discard,
             Keep
         };
 
@@ -87,7 +87,7 @@ namespace TestImpact
             "ignore",
             "stdout",
             "file",
-            "remove",
+            "discard",
             "keep"
         };
 
@@ -147,7 +147,7 @@ namespace TestImpact
         {
             const AZStd::vector<AZStd::pair<AZStd::string, Policy::FailedTestCoverage>> states =
             {
-                {OptionKeys[Remove], Policy::FailedTestCoverage::Remove},
+                {OptionKeys[Discard], Policy::FailedTestCoverage::Discard},
                 {OptionKeys[Keep], Policy::FailedTestCoverage::Keep}
             };
 
@@ -405,11 +405,11 @@ namespace TestImpact
             "                                                    tests are run regardless).\n"
             "    -shard=<on,off>                                 Break any test targets with a sharding policy into the number of \n"
             "                                                    shards according to the maximum concurrency value.\n"
-            "    -cpolicy=<remove, keep>                         Policy for handling the coverage data of failed tests (both tests that \n"
+            "    -cpolicy=<remove, keep>                         Policy for handling the coverage data of failing tests, where discard \n"
-            "                                                    failed to execute and tests that ran but failed), where remove will \n"
+            "                                                    will discard the coverage data produced by the failing tests, causing \n"
-            "                                                    remove the failed tests from the all coverage data (causing them to be \n"
+            "                                                    them to be drafted into future test runs and keep will keep any existing \n"
-            "                                                    drafted into future test runs) and keep will keep any existing coverage \n"
+            "                                                    coverage data and update the coverage data for failed tests that produce \n"
-            "                                                    data and update the coverage data for failed tests that produce coverage.\n"
+            "                                                    coverage.\n"
             "    -targetout=<sdtout, file>                       Capture of individual test run stdout, where stdout will capture \n"
             "                                                    each individual test target's stdout and output each one to stdout \n"
             "                                                    and file will capture each individual test target's stdout and output \n"

Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h

@@ -81,7 +81,8 @@ namespace TestImpact
     using SafeTestSequenceCompleteCallback = AZStd::function<void(
         Client::SequenceFailure&& selectedFailureReport,
         Client::SequenceFailure&& discardedFailureReport,
-        AZStd::chrono::milliseconds duration)>;
+        AZStd::chrono::milliseconds selectedDuration,
+        AZStd::chrono::milliseconds discardedDuration)>;
 
     //! Callback for test runs that have completed for any reason.
     //! @param selectedTests The test that has completed.
@@ -201,6 +202,10 @@ namespace TestImpact
         AZStd::pair<AZStd::vector<const TestTarget*>, AZStd::vector<const TestTarget*>> SelectTestTargetsByExcludeList(
             AZStd::vector<const TestTarget*> testTargets) const;
 
+        //! Prunes the existing coverage for the specified jobs and creates the consolidates source covering tests list from the
+        //! test engine instrumented run jobs.
+        SourceCoveringTestsList CreateSourceCoveringTestFromTestCoverages(const AZStd::vector<TestEngineInstrumentedRun>& jobs);
+
         //! Prepares the dynamic dependency map for a seed update by clearing all existing data and deleting the file that will be serialized.
         void ClearDynamicDependencyMapAndRemoveExistingFile();
 

Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h

@@ -34,7 +34,7 @@ namespace TestImpact
         //! Policy for handling the coverage data of failed tests targets (both test that failed to execute and tests that ran but failed).
         enum class FailedTestCoverage
         {
-            Remove, //!< Remove the failed test targets from the all coverage data (causing them to be drafted into future test runs).
+            Discard, //!< Discard the coverage data produced by the failing tests, causing them to be drafted into future test runs.
             Keep //!< Keep any existing coverage data and update the coverage data for failed test targetss that produce coverage.
         };
 

Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp

@@ -133,8 +133,9 @@ namespace TestImpact
         return buildTarget;
     }
 
-    void DynamicDependencyMap::ReplaceSourceCoverage(const SourceCoveringTestsList& sourceCoverageDelta)
+    void DynamicDependencyMap::ReplaceSourceCoverageInternal(const SourceCoveringTestsList& sourceCoverageDelta, bool pruneIfNoParentsOrCoverage)
     {
+        AZStd::vector<AZStd::string> killList;
         for (const auto& sourceCoverage : sourceCoverageDelta.GetCoverage())
         {
             // Autogen input files are not compiled sources and thus supplying coverage data for them makes no sense
@@ -146,7 +147,12 @@ namespace TestImpact
             auto [sourceDependencyIt, inserted] = m_sourceDependencyMap.insert(sourceCoverage.GetPath().String());
             auto& [source, sourceDependency] = *sourceDependencyIt;
 
-            // Remove the source from the test target covering sources map and clear any existing coverage for the delta
+            // Before we can replace the coverage for this source dependency, we must:
+            // 1. Remove the source from the test target covering sources map
+            // 2. Prune the covered targets for the parent test target(s) of this source dependency
+            // 3. Clear any existing coverage for the delta
+
+            // 1.
             for (const auto& testTarget : sourceDependency.m_coveringTestTargets)
             {
                 if (auto coveringTestTargetIt = m_testTargetSourceCoverage.find(testTarget);
@@ -154,7 +160,20 @@ namespace TestImpact
                 {
                     coveringTestTargetIt->second.erase(source);
                 }
+
+                
             }
+
+            // 2.
+            // This step is prohibitively expensive as it requires iterating over all of the sources of the build targets covered by
+            // the parent test targets of this source dependency to ensure that this is in fact the last source being cleared and thus
+            // it can be determined that the parent test target is no longer covering the given build target
+            //
+            // The implications of this are that multiple calls to the test selector and priritizor's SelectTestTargets method will end
+            // up pulling in more test targets than needed for newly-created production sources until the next time the dynamic dependency
+            // map reconstructed, however until this use case materializes the implications described will not be addressed
+
+            // 3.
             sourceDependency.m_coveringTestTargets.clear();
 
             // Update the dependency with any new coverage data
@@ -183,13 +202,18 @@ namespace TestImpact
             }
 
             // If the new coverage data results in a parentless and coverageless entry, consider it a dead entry and remove accordingly
-            if (sourceDependency.m_coveringTestTargets.empty() && sourceDependency.m_parentTargets.empty())
+            if (sourceDependency.m_coveringTestTargets.empty() && sourceDependency.m_parentTargets.empty() && pruneIfNoParentsOrCoverage)
             {
                 m_sourceDependencyMap.erase(sourceDependencyIt);
             }
         }
     }
 
+    void DynamicDependencyMap::ReplaceSourceCoverage(const SourceCoveringTestsList& sourceCoverageDelta)
+    {
+        ReplaceSourceCoverageInternal(sourceCoverageDelta, true);
+    }
+
     void DynamicDependencyMap::ClearSourceCoverage(const AZStd::vector<RepoPath>& paths)
     {
         for (const auto& path : paths)
@@ -212,9 +236,14 @@ namespace TestImpact
 
     void DynamicDependencyMap::ClearAllSourceCoverage()
     {
-        for (const auto& [path, coverage] : m_sourceDependencyMap)
+        for (auto it = m_sourceDependencyMap.begin(); it != m_sourceDependencyMap.end(); ++it)
         {
-            ReplaceSourceCoverage(SourceCoveringTestsList(AZStd::vector<SourceCoveringTests>{ SourceCoveringTests(RepoPath(path)) }));
+            const auto& [path, coverage] = *it;
+            ReplaceSourceCoverageInternal(SourceCoveringTestsList(AZStd::vector<SourceCoveringTests>{ SourceCoveringTests(RepoPath(path)) }), false);
+            if (coverage.m_coveringTestTargets.empty() && coverage.m_parentTargets.empty())
+            {
+                it = m_sourceDependencyMap.erase(it);
+            }
         }
     }
 

Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h

@@ -81,7 +81,6 @@ namespace TestImpact
         SourceDependency GetSourceDependencyOrThrow(const RepoPath& path) const;
 
         //! Replaces the source coverage of the specified sources with the specified source coverage.
-        //! @note The covering targets for the parent test target(s) will not be pruned if those covering targets are removed.
         //! @param sourceCoverageDelta The source coverage delta to replace in the dependency map.
         void ReplaceSourceCoverage(const SourceCoveringTestsList& sourceCoverageDelta);
 
@@ -110,6 +109,13 @@ namespace TestImpact
         AZStd::vector<const TestTarget*> GetNotCoveringTests() const;
 
     private:
+        //! Internal handler for ReplaceSourceCoverage where the pruning of parentless and coverageless source depenencies after the
+        //! source coverage has been replaced must be explicitly stated.
+        //! @note The covered targets for the source dependency's parent test target(s) will not be pruned if those covering targets are removed.
+        //! @param sourceCoverageDelta The source coverage delta to replace in the dependency map.
+        //! @param pruneIfNoParentsOrCoverage Flag to specify whether or not newly parentless and coverageless dependencies will be removed.
+        void ReplaceSourceCoverageInternal(const SourceCoveringTestsList& sourceCoverageDelta, bool pruneIfNoParentsOrCoverage);
+
         //! Clears the source coverage of the specified sources.
         //! @note The covering targets for the parent test target(s) will not be pruned if those covering targets are removed.
         void ClearSourceCoverage(const AZStd::vector<RepoPath>& paths);
@@ -127,6 +133,7 @@ namespace TestImpact
         AZStd::unordered_map<const TestTarget*, AZStd::unordered_set<AZStd::string>> m_testTargetSourceCoverage;
 
         //! The map of build targets and their covering test targets.
+        //! @note As per the note for ReplaceSourceCoverageInternal, this map is currently not pruned when source coverage is replaced.
         AZStd::unordered_map<const BuildTarget*, AZStd::unordered_set<const TestTarget*>> m_buildTargetCoverage;
 
         //! Mapping of autogen input sources to their generated output sources.

Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Enumeration/TestImpactTestEnumerator.cpp

@@ -73,7 +73,7 @@ namespace TestImpact
                     }
                     catch (const TestEngineException& e)
                     {
-                        AZ_Printf("Enumerate", "Enumeration cache error: %s", e.what());
+                        AZ_Printf("Enumerate", AZStd::string::format("Enumeration cache error: %s\n", e.what()).c_str());
                         DeleteFile(jobInfo->GetCache()->m_file);
                     }
 

Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactInstrumentedTestRunner.cpp

@@ -72,7 +72,7 @@ namespace TestImpact
                     }
                     catch (const Exception& e)
                     {
-                        AZ_Printf("RunInstrumentedTests", e.what());
+                        AZ_Printf("RunInstrumentedTests", AZStd::string::format("%s\n", e.what()).c_str());
                         runs[jobId] = AZStd::nullopt;
                     }
                 }

Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactTestRunner.cpp

@@ -46,7 +46,7 @@ namespace TestImpact
                     }
                     catch (const Exception& e)
                     {
-                        AZ_Printf("RunTests", e.what());
+                        AZ_Printf("RunTests", AZStd::string::format("%s\n", e.what()).c_str());
                         runs[jobId] = AZStd::nullopt;
                     }
                 }

Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp

@@ -256,32 +256,77 @@ namespace TestImpact
 
     void Runtime::ClearDynamicDependencyMapAndRemoveExistingFile()
     {
-        DeleteFile(m_sparTIAFile);
         m_dynamicDependencyMap->ClearAllSourceCoverage();
+        DeleteFile(m_sparTIAFile);
+    }
+
+    SourceCoveringTestsList Runtime::CreateSourceCoveringTestFromTestCoverages(const AZStd::vector<TestEngineInstrumentedRun>& jobs)
+    {
+        AZStd::unordered_map<AZStd::string, AZStd::unordered_set<AZStd::string>> coverage;
+        for (const auto& job : jobs)
+        {
+            // First we must remove any existing coverage for the test target so as to not end up with source remnants from previous
+            // coverage that is no longer covered by this revision of the test target
+            m_dynamicDependencyMap->RemoveTestTargetFromSourceCoverage(job.GetTestTarget());
+
+            // Next we will update the coverage of test targets that completed (with or without failures), unless the failed test coverage
+            // policy dictates we should instead discard the coverage of test targets with failing tests
+            if (const auto testResult = job.GetTestResult();
+                testResult == Client::TestRunResult::AllTestsPass ||
+                (m_failedTestCoveragePolicy == Policy::FailedTestCoverage::Keep && testResult == Client::TestRunResult::TestFailures))
+            {
+                if (testResult == Client::TestRunResult::AllTestsPass)
+                {
+                    // Passing tests should have coverage data, otherwise something is very wrong
+                    AZ_TestImpact_Eval(
+                        job.GetTestCoverge().has_value(),
+                        RuntimeException,
+                        AZStd::string::format(
+                            "Test target '%s' completed its test run successfully but produced no coverage data",
+                            job.GetTestTarget()->GetName().c_str()));
+                }
+                else if (!job.GetTestCoverge().has_value())
+                {
+                    // When a test run completes with failing tests but produces no coverage artifact that's typically a sign of the
+                    // test aborting due to an unhandled exception, in which case ignore it and let it be picked up in the failure report
+                    continue;
+                }
+
+                for (const auto& source : job.GetTestCoverge().value().GetSourcesCovered())
+                {
+                    coverage[source.String()].insert(job.GetTestTarget()->GetName());
+                }
+            }
+        }
+
+        AZStd::vector<SourceCoveringTests> sourceCoveringTests;
+        sourceCoveringTests.reserve(coverage.size());
+        for (auto&& [source, testTargets] : coverage)
+        {
+            if (const auto sourcePath = RepoPath(source);
+                sourcePath.IsRelativeTo(m_config.m_repo.m_root))
+            {
+                sourceCoveringTests.push_back(
+                    SourceCoveringTests(RepoPath(sourcePath.LexicallyRelative(m_config.m_repo.m_root)), AZStd::move(testTargets)));
+            }
+            else
+            {
+                AZ_Warning("TestImpact", false, "Ignoring source, source it outside of repo: '%s'", sourcePath.c_str());
+            }
+        }
+
+        return SourceCoveringTestsList(AZStd::move(sourceCoveringTests));
     }
 
     void Runtime::UpdateAndSerializeDynamicDependencyMap(const AZStd::vector<TestEngineInstrumentedRun>& jobs)
     {
-        const auto sourceCoverageTestsList = CreateSourceCoveringTestFromTestCoverages(jobs, m_config.m_repo.m_root);
+        const auto sourceCoverageTestsList = CreateSourceCoveringTestFromTestCoverages(jobs);
         if (!sourceCoverageTestsList.GetNumSources())
         {
             return;
         }
 
         m_dynamicDependencyMap->ReplaceSourceCoverage(sourceCoverageTestsList);
-
-        if (m_failedTestCoveragePolicy == Policy::FailedTestCoverage::Remove)
-        {
-            for (const auto& job : jobs)
-            {
-                if (job.GetTestResult() != Client::TestRunResult::AllTestsPass ||
-                    !job.GetTestCoverge().has_value()) 
-                {
-                    m_dynamicDependencyMap->RemoveTestTargetFromSourceCoverage(job.GetTestTarget());
-                }
-            }
-        }
-
         const auto sparTIA = m_dynamicDependencyMap->ExportSourceCoverage();
         const auto sparTIAData = SerializeSourceCoveringTestsList(sparTIA);
         WriteFileContents<RuntimeException>(sparTIAData, m_sparTIAFile);
@@ -422,7 +467,7 @@ namespace TestImpact
         AZStd::optional<SafeTestSequenceCompleteCallback> testSequenceEndCallback,
         AZStd::optional<TestRunCompleteCallback> testCompleteCallback)
     {
-        Timer timer;       
+        Timer timer;
 
         // Draft in the test targets that have no coverage entries in the dynamic dependency map
         AZStd::vector<const TestTarget*> draftedTestTargets = m_dynamicDependencyMap->GetNotCoveringTests();
@@ -459,6 +504,8 @@ namespace TestImpact
             testTargetTimeout,
             globalTimeout,
             TestRunCompleteCallbackHandler(testCompleteCallback));
+        
+        const auto selectedDuraton = timer.Elapsed();
 
         // Carry the remaining global sequence time over to the discarded test run
         if (globalTimeout.has_value())
@@ -478,16 +525,18 @@ namespace TestImpact
             globalTimeout,
             TestRunCompleteCallbackHandler(testCompleteCallback));
 
-        UpdateAndSerializeDynamicDependencyMap(selectedTestJobs);
+        const auto discardedDuraton = timer.Elapsed();
 
         if (testSequenceEndCallback.has_value())
         {
             (*testSequenceEndCallback)(
                 GenerateSequenceFailureReport(selectedTestJobs),
                 GenerateSequenceFailureReport(discardedTestJobs),
-                timer.Elapsed());
+                selectedDuraton,
+                discardedDuraton);
         }
 
+        UpdateAndSerializeDynamicDependencyMap(selectedTestJobs);
         return { selectedResult, discardedResult };
     }
 
@@ -530,14 +579,14 @@ namespace TestImpact
             globalTimeout,
             TestRunCompleteCallbackHandler(testCompleteCallback));
 
-        ClearDynamicDependencyMapAndRemoveExistingFile();
-        UpdateAndSerializeDynamicDependencyMap(testJobs);
-
         if (testSequenceEndCallback.has_value())
         {
             (*testSequenceEndCallback)(GenerateSequenceFailureReport(testJobs), timer.Elapsed());
         }
 
+        ClearDynamicDependencyMapAndRemoveExistingFile();
+        UpdateAndSerializeDynamicDependencyMap(testJobs);
+
         return result;
     }
 

Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.cpp

@@ -82,54 +82,4 @@ namespace TestImpact
 
         return testNames;
     }
-
+} // namespace TestImpact
-    SourceCoveringTestsList CreateSourceCoveringTestFromTestCoverages(const AZStd::vector<TestEngineInstrumentedRun>& jobs, const RepoPath& root)
-    {
-        AZStd::unordered_map<AZStd::string, AZStd::unordered_set<AZStd::string>> coverage;
-        for (const auto& job : jobs)
-        {
-            if (const auto testResult = job.GetTestResult();
-                testResult == Client::TestRunResult::AllTestsPass || testResult == Client::TestRunResult::TestFailures)
-            {
-                if (testResult == Client::TestRunResult::AllTestsPass)
-                {
-                    // Passing tests should have coverage data, otherwise something is very wrong
-                    AZ_TestImpact_Eval(
-                        job.GetTestCoverge().has_value(),
-                        RuntimeException,
-                        AZStd::string::format(
-                            "Test target '%s' completed its test run successfully but produced no coverage data",
-                            job.GetTestTarget()->GetName().c_str()));
-                }
-                else if (!job.GetTestCoverge().has_value())
-                {
-                    // When a test run completes with failing tests but produces no coverage artifact that's typically a sign of the
-                    // test aborting due to an unhandled exception, in which case ignore it and let it be picked up in the failure report
-                    continue;
-                }
-
-                for (const auto& source : job.GetTestCoverge().value().GetSourcesCovered())
-                {
-                    coverage[source.String()].insert(job.GetTestTarget()->GetName());
-                }
-            }
-        }
-
-        AZStd::vector<SourceCoveringTests> sourceCoveringTests;
-        sourceCoveringTests.reserve(coverage.size());
-        for (auto&& [source, testTargets] : coverage)
-        {
-            if (const auto sourcePath = RepoPath(source);
-                sourcePath.IsRelativeTo(root))
-            {
-                sourceCoveringTests.push_back(SourceCoveringTests(RepoPath(sourcePath.LexicallyRelative(root)), AZStd::move(testTargets)));
-            }
-            else
-            {
-                AZ_Warning("TestImpact", false, "Ignoring source, source it outside of repo: %s", sourcePath.c_str());
-            }
-        }
-
-        return SourceCoveringTestsList(AZStd::move(sourceCoveringTests));
-    }
-}

Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.h

@@ -40,12 +40,7 @@ namespace TestImpact
         const AZStd::vector<AZStd::string>& excludedTestTargets);
 
     //! Extracts the name information from the specified test targets.
-    AZStd::vector<AZStd::string> ExtractTestTargetNames(const AZStd::vector<const TestTarget*> testTargets);
+    AZStd::vector<AZStd::string> ExtractTestTargetNames(const AZStd::vector<const TestTarget*> testTargets);    
-
-    //! Creates the consolidates source covering tests list from the test engine instrumented run jobs.
-    SourceCoveringTestsList CreateSourceCoveringTestFromTestCoverages(
-        const AZStd::vector<TestEngineInstrumentedRun>& jobs,
-        const RepoPath& root);
 
     //! Generates a test run failure report from the specified test engine job information.
     //! @tparam TestJob The test engine job type.