diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp index 01005718bc..992b466a54 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp +++ b/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> 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= Break any test targets with a sharding policy into the number of \n" " shards according to the maximum concurrency value.\n" - " -cpolicy= Policy for handling the coverage data of failed tests (both tests that \n" - " failed to execute and tests that ran but failed), where remove will \n" - " remove the failed tests from the all coverage data (causing them to be \n" - " drafted into future test runs) and keep will keep any existing coverage \n" - " data and update the coverage data for failed tests that produce coverage.\n" + " -cpolicy= Policy for handling the coverage data of failing tests, where discard \n" + " will discard the coverage data produced by the failing tests, causing \n" + " them to be drafted into future test runs and keep will keep any existing \n" + " coverage data and update the coverage data for failed tests that produce \n" + " coverage.\n" " -targetout= 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" diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h index 86f2907ab7..2a7720ee55 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h @@ -81,7 +81,8 @@ namespace TestImpact using SafeTestSequenceCompleteCallback = AZStd::function; + 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> SelectTestTargetsByExcludeList( AZStd::vector 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& 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(); diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h index 5f9bff952f..5a5a6196e2 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h +++ b/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. }; diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp index 22bcc97c2a..28b817e32c 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp +++ b/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 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& 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(RepoPath(path)) })); + const auto& [path, coverage] = *it; + ReplaceSourceCoverageInternal(SourceCoveringTestsList(AZStd::vector{ SourceCoveringTests(RepoPath(path)) }), false); + if (coverage.m_coveringTestTargets.empty() && coverage.m_parentTargets.empty()) + { + it = m_sourceDependencyMap.erase(it); + } } } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h index cd01a1a728..701f69e68d 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h +++ b/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 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& paths); @@ -127,6 +133,7 @@ namespace TestImpact AZStd::unordered_map> 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> m_buildTargetCoverage; //! Mapping of autogen input sources to their generated output sources. diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Enumeration/TestImpactTestEnumerator.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Enumeration/TestImpactTestEnumerator.cpp index 58ffb85cb3..17784dd937 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Enumeration/TestImpactTestEnumerator.cpp +++ b/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); } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactInstrumentedTestRunner.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactInstrumentedTestRunner.cpp index aa8ff04657..5a0ef74472 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactInstrumentedTestRunner.cpp +++ b/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; } } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactTestRunner.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactTestRunner.cpp index 7ac13f85b2..16717a1fe5 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestEngine/Run/TestImpactTestRunner.cpp +++ b/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; } } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp index d83976b488..79f5269188 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp +++ b/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); } - void Runtime::UpdateAndSerializeDynamicDependencyMap(const AZStd::vector& jobs) + SourceCoveringTestsList Runtime::CreateSourceCoveringTestFromTestCoverages(const AZStd::vector& jobs) { - const auto sourceCoverageTestsList = CreateSourceCoveringTestFromTestCoverages(jobs, m_config.m_repo.m_root); - if (!sourceCoverageTestsList.GetNumSources()) + AZStd::unordered_map> coverage; + for (const auto& job : jobs) { - return; - } + // 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; + } - m_dynamicDependencyMap->ReplaceSourceCoverage(sourceCoverageTestsList); + for (const auto& source : job.GetTestCoverge().value().GetSourcesCovered()) + { + coverage[source.String()].insert(job.GetTestTarget()->GetName()); + } + } + } - if (m_failedTestCoveragePolicy == Policy::FailedTestCoverage::Remove) + AZStd::vector sourceCoveringTests; + sourceCoveringTests.reserve(coverage.size()); + for (auto&& [source, testTargets] : coverage) { - for (const auto& job : jobs) + if (const auto sourcePath = RepoPath(source); + sourcePath.IsRelativeTo(m_config.m_repo.m_root)) { - if (job.GetTestResult() != Client::TestRunResult::AllTestsPass || - !job.GetTestCoverge().has_value()) - { - m_dynamicDependencyMap->RemoveTestTargetFromSourceCoverage(job.GetTestTarget()); - } + 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& jobs) + { + const auto sourceCoverageTestsList = CreateSourceCoveringTestFromTestCoverages(jobs); + if (!sourceCoverageTestsList.GetNumSources()) + { + return; + } + + m_dynamicDependencyMap->ReplaceSourceCoverage(sourceCoverageTestsList); const auto sparTIA = m_dynamicDependencyMap->ExportSourceCoverage(); const auto sparTIAData = SerializeSourceCoveringTestsList(sparTIA); WriteFileContents(sparTIAData, m_sparTIAFile); @@ -422,7 +467,7 @@ namespace TestImpact AZStd::optional testSequenceEndCallback, AZStd::optional testCompleteCallback) { - Timer timer; + Timer timer; // Draft in the test targets that have no coverage entries in the dynamic dependency map AZStd::vector 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; } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.cpp index 47c0e83233..68482ab721 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.cpp @@ -82,54 +82,4 @@ namespace TestImpact return testNames; } - - SourceCoveringTestsList CreateSourceCoveringTestFromTestCoverages(const AZStd::vector& jobs, const RepoPath& root) - { - AZStd::unordered_map> 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.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)); - } -} +} // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.h b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.h index 943b0cbec4..807911d854 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntimeUtils.h @@ -40,12 +40,7 @@ namespace TestImpact const AZStd::vector& excludedTestTargets); //! Extracts the name information from the specified test targets. - AZStd::vector ExtractTestTargetNames(const AZStd::vector testTargets); - - //! Creates the consolidates source covering tests list from the test engine instrumented run jobs. - SourceCoveringTestsList CreateSourceCoveringTestFromTestCoverages( - const AZStd::vector& jobs, - const RepoPath& root); + AZStd::vector ExtractTestTargetNames(const AZStd::vector testTargets); //! Generates a test run failure report from the specified test engine job information. //! @tparam TestJob The test engine job type.