From 4ed0c7b1d8ccaf70bec71674763dc0dd65ad5f9f Mon Sep 17 00:00:00 2001 From: jonawals Date: Mon, 7 Jun 2021 08:52:20 +0100 Subject: [PATCH 1/3] Add drafting of failed tests --- .../Source/TestImpactCommandLineOptions.cpp | 50 ++++++++++-------- .../Source/TestImpactCommandLineOptions.h | 6 +-- .../Code/Source/TestImpactConsoleMain.cpp | 2 +- .../TestImpactFramework/TestImpactRuntime.h | 7 +-- .../TestImpactTestSequence.h | 8 +-- .../TestImpactDynamicDependencyMap.cpp | 48 +++++++++++------ .../TestImpactDynamicDependencyMap.h | 9 ++-- .../Runtime/Code/Source/TestImpactRuntime.cpp | 51 ++++++++++++++----- 8 files changed, 119 insertions(+), 62 deletions(-) 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 5ff070601a..aad9319cf9 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp @@ -28,7 +28,7 @@ namespace TestImpact Sequence, TestPrioritizationPolicy, ExecutionFailurePolicy, - ExecutionFailureDraftingPolicy, + FailedTestCoveragePolicy, TestFailurePolicy, IntegrityFailurePolicy, TestShardingPolicy, @@ -50,7 +50,9 @@ namespace TestImpact Continue, Ignore, StdOut, - File + File, + Remove, + Keep }; constexpr const char* OptionKeys[] = @@ -62,7 +64,7 @@ namespace TestImpact "sequence", "ppolicy", "epolicy", - "rexecfailures", + "cpolicy", "fpolicy", "ipolicy", "shard", @@ -84,7 +86,9 @@ namespace TestImpact "continue", "ignore", "stdout", - "file" + "file", + "remove", + "keep" }; RepoPath ParseConfigurationFile(const AZ::CommandLine& cmd) @@ -139,15 +143,15 @@ namespace TestImpact return ParseMultiStateOption(OptionKeys[ExecutionFailurePolicy], states, cmd).value_or(Policy::ExecutionFailure::Continue); } - Policy::ExecutionFailureDrafting ParseExecutionFailureDraftingPolicy(const AZ::CommandLine& cmd) + Policy::FailedTestCoverage ParseFailedTestCoveragePolicy(const AZ::CommandLine& cmd) { - const BinaryStateValue states = + const AZStd::vector> states = { - Policy::ExecutionFailureDrafting::Never, - Policy::ExecutionFailureDrafting::Always + {OptionKeys[Remove], Policy::FailedTestCoverage::Remove}, + {OptionKeys[Keep], Policy::FailedTestCoverage::Keep} }; - return ParseOnOffOption(OptionKeys[ExecutionFailureDraftingPolicy], states, cmd).value_or(Policy::ExecutionFailureDrafting::Always); + return ParseMultiStateOption(OptionKeys[FailedTestCoveragePolicy], states, cmd).value_or(Policy::FailedTestCoverage::Keep); } Policy::TestFailure ParseTestFailurePolicy(const AZ::CommandLine& cmd) @@ -275,7 +279,7 @@ namespace TestImpact m_testSequenceType = ParseTestSequenceType(cmd); m_testPrioritizationPolicy = ParseTestPrioritizationPolicy(cmd); m_executionFailurePolicy = ParseExecutionFailurePolicy(cmd); - m_executionFailureDraftingPolicy = ParseExecutionFailureDraftingPolicy(cmd); + m_failedTestCoveragePolicy = ParseFailedTestCoveragePolicy(cmd); m_testFailurePolicy = ParseTestFailurePolicy(cmd); m_integrityFailurePolicy = ParseIntegrityFailurePolicy(cmd); m_testShardingPolicy = ParseTestShardingPolicy(cmd); @@ -327,9 +331,9 @@ namespace TestImpact return m_executionFailurePolicy; } - Policy::ExecutionFailureDrafting CommandLineOptions::GetExecutionFailureDraftingPolicy() const + Policy::FailedTestCoverage CommandLineOptions::GetFailedTestCoveragePolicy() const { - return m_executionFailureDraftingPolicy; + return m_failedTestCoveragePolicy; } Policy::TestFailure CommandLineOptions::GetTestFailurePolicy() const @@ -401,7 +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" - " -rexecfailures= Attempt to execute test targets that previously failed to execute.\n" + " -cpolicy= Policy for handling the coverage data of failed tests (both test 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" " -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" @@ -409,25 +417,25 @@ namespace TestImpact " -epolicy= Policy for handling test execution failure (test targets could not be \n" " launched due to the binary not being built, incorrect paths, etc.), \n" " where abort will abort the entire test sequence upon the first test\n" - " target execution failureand report a failure(along with the return \n" + " target execution failure and report a failure(along with the return \n" " code of the test target that failed to launch), continue will continue \n" " with the test sequence in the event of test target execution failures\n" " and treat the test targets that failed to launch as as test failures\n" " (along with the return codes of the test targets that failed to \n" " launch), ignore will continue with the test sequence in the event of \n" - " test target execution failuresand treat the test targets that failed\n" - " to launch as as test passes(along with the return codes of the test \n" + " test target execution failures and treat the test targets that failed\n" + " to launch as test passes(along with the return codes of the test \n" " targets that failed to launch).\n" " -fpolicy Policy for handling test failures (test targets report failing tests), \n" - " where abort will abort the entire test sequenceupon the first test \n" - " failureand report a failure and continue will continue with the test\n" - " sequence in the event of test failuresand report the test failures.\n" + " where abort will abort the entire test sequence upon the first test \n" + " failure and report a failure and continue will continue with the test\n" + " sequence in the event of test failures and report the test failures.\n" " -ipolicy= Policy for handling coverage data integrity failures, where abort will \n" " abort the test sequenceand report a failure, seed will attempt another \n" " sequence using the seed sequence type, otherwise will abort and report \n" - " a failure (this option has no effect for regularand seed sequence \n" + " a failure (this option has no effect for regular and seed sequence \n" " types) and rerun will attempt another sequence using the regular \n" - " sequence type, otherwise will abortand report a failure(this option has \n" + " sequence type, otherwise will abort and report a failure(this option has \n" " no effect for regular sequence type).\n" " -ppolicy= Policy for prioritizing selected test targets, where none will not \n" " attempt any test target prioritization and locality will attempt to \n" diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h index 99b1b98dba..b215261a87 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.h @@ -64,8 +64,8 @@ namespace TestImpact //! Returns the test execution failure policy to use. Policy::ExecutionFailure GetExecutionFailurePolicy() const; - //! Returns the test historic test execution failure drafting policy to use. - Policy::ExecutionFailureDrafting GetExecutionFailureDraftingPolicy() const; + //! Returns failed test coverage drafting policy to use. + Policy::FailedTestCoverage GetFailedTestCoveragePolicy() const; //! Returns the test failure policy to use. Policy::TestFailure GetTestFailurePolicy() const; @@ -98,7 +98,7 @@ namespace TestImpact TestSequenceType m_testSequenceType; Policy::TestPrioritization m_testPrioritizationPolicy = Policy::TestPrioritization::None; Policy::ExecutionFailure m_executionFailurePolicy = Policy::ExecutionFailure::Continue; - Policy::ExecutionFailureDrafting m_executionFailureDraftingPolicy = Policy::ExecutionFailureDrafting::Always; + Policy::FailedTestCoverage m_failedTestCoveragePolicy = Policy::FailedTestCoverage::Keep; Policy::TestFailure m_testFailurePolicy = Policy::TestFailure::Abort; Policy::IntegrityFailure m_integrityFailurePolicy = Policy::IntegrityFailure::Abort; Policy::TestSharding m_testShardingPolicy = Policy::TestSharding::Never; diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactConsoleMain.cpp b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactConsoleMain.cpp index 4853c6da15..77b1d98b3c 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactConsoleMain.cpp +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactConsoleMain.cpp @@ -205,7 +205,7 @@ namespace TestImpact RuntimeConfigurationFactory(ReadFileContents(options.GetConfigurationFile())), options.GetSuiteFilter(), options.GetExecutionFailurePolicy(), - options.GetExecutionFailureDraftingPolicy(), + options.GetFailedTestCoveragePolicy(), options.GetTestFailurePolicy(), options.GetIntegrityFailurePolicy(), options.GetTestShardingPolicy(), diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h index ae1f78c5c7..86f2907ab7 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactRuntime.h @@ -35,6 +35,7 @@ namespace TestImpact class TestEngine; class TestTarget; class SourceCoveringTestsList; + class TestEngineInstrumentedRun; //! Callback for a test sequence that isn't using test impact analysis to determine selected tests. //! @param tests The tests that will be run for this sequence. @@ -102,7 +103,7 @@ namespace TestImpact RuntimeConfig&& config, SuiteType suiteFilter, Policy::ExecutionFailure executionFailurePolicy, - Policy::ExecutionFailureDrafting executionFailureDraftingPolicy, + Policy::FailedTestCoverage failedTestCoveragePolicy, Policy::TestFailure testFailurePolicy, Policy::IntegrityFailure integrationFailurePolicy, Policy::TestSharding testShardingPolicy, @@ -204,13 +205,13 @@ namespace TestImpact void ClearDynamicDependencyMapAndRemoveExistingFile(); //! Updates the dynamic dependency map and serializes the entire map to disk. - void UpdateAndSerializeDynamicDependencyMap(const SourceCoveringTestsList& sourceCoverageTestsList); + void UpdateAndSerializeDynamicDependencyMap(const AZStd::vector& jobs); RuntimeConfig m_config; SuiteType m_suiteFilter; RepoPath m_sparTIAFile; Policy::ExecutionFailure m_executionFailurePolicy; - Policy::ExecutionFailureDrafting m_executionFailureDraftingPolicy; + Policy::FailedTestCoverage m_failedTestCoveragePolicy; Policy::TestFailure m_testFailurePolicy; Policy::IntegrityFailure m_integrationFailurePolicy; Policy::TestSharding m_testShardingPolicy; diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h index 4e8908cf3f..5f9bff952f 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactTestSequence.h @@ -31,11 +31,11 @@ namespace TestImpact Ignore //!< Continue the test sequence and ignore the execution failures. }; - //! Policy for reattempting the execution of test targets that failed to execute in previous runs. - enum class ExecutionFailureDrafting + //! 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 { - Never, //!< Do not attempt to execute historic execution failures. - Always //!< Reattempt the exectution of historic execution failures. + Remove, //!< Remove the failed test targets from the all coverage data (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. }; //! Policy for prioritizing selected tests. diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp index ece5a7c13b..22bcc97c2a 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp @@ -56,7 +56,7 @@ namespace TestImpact for (const auto& target : m_testTargets.GetTargets()) { mapBuildTargetSources(&target); - m_testTargetSourceCoverageCount[&target] = 0; + m_testTargetSourceCoverage[&target] = {}; } } @@ -144,18 +144,15 @@ namespace TestImpact sourceCoverage.GetPath().c_str()).c_str()); auto [sourceDependencyIt, inserted] = m_sourceDependencyMap.insert(sourceCoverage.GetPath().String()); - auto& [key, sourceDependency] = *sourceDependencyIt; + auto& [source, sourceDependency] = *sourceDependencyIt; - // Knock down the source coverage count for the test targets and clear any existing coverage for the delta + // Remove the source from the test target covering sources map and clear any existing coverage for the delta for (const auto& testTarget : sourceDependency.m_coveringTestTargets) { - if (auto coveringTestTargetIt = m_testTargetSourceCoverageCount.find(testTarget); - coveringTestTargetIt != m_testTargetSourceCoverageCount.end()) + if (auto coveringTestTargetIt = m_testTargetSourceCoverage.find(testTarget); + coveringTestTargetIt != m_testTargetSourceCoverage.end()) { - if (coveringTestTargetIt->second > 0) - { - coveringTestTargetIt->second--; - } + coveringTestTargetIt->second.erase(source); } } sourceDependency.m_coveringTestTargets.clear(); @@ -169,8 +166,8 @@ namespace TestImpact // Source to covering test target mapping sourceDependency.m_coveringTestTargets.insert(testTarget); - // Test target covering sources count - m_testTargetSourceCoverageCount[testTarget]++; + // Add the source to the test target covering sources map + m_testTargetSourceCoverage[testTarget].insert(source); // Build target to covering test target mapping for (const auto& parentTarget : sourceDependency.m_parentTargets) @@ -429,12 +426,33 @@ namespace TestImpact return ChangeDependencyList(AZStd::move(createDependencies), AZStd::move(updateDependencies), AZStd::move(deleteDependencies)); } + void DynamicDependencyMap::RemoveTestTargetFromSourceCoverage(const TestTarget* testTarget) + { + if (const auto& it = m_testTargetSourceCoverage.find(testTarget); + it != m_testTargetSourceCoverage.end()) + { + for (const auto& source : it->second) + { + const auto sourceDependency = m_sourceDependencyMap.find(source); + AZ_TestImpact_Eval( + sourceDependency != m_sourceDependencyMap.end(), + DependencyException, + AZStd::string::format("Test target '%s' has covering source '%s' yet cannot be found in the dependency map", + testTarget->GetName().c_str(), source.c_str())); + + sourceDependency->second.m_coveringTestTargets.erase(testTarget); + } + + m_testTargetSourceCoverage.erase(testTarget); + } + } + AZStd::vector DynamicDependencyMap::GetCoveringTests() const { AZStd::vector covering; - for (const auto& [testTarget, coveringSources] : m_testTargetSourceCoverageCount) + for (const auto& [testTarget, coveringSources] : m_testTargetSourceCoverage) { - if (coveringSources > 0) + if (!coveringSources.empty()) { covering.push_back(testTarget); } @@ -446,9 +464,9 @@ namespace TestImpact AZStd::vector DynamicDependencyMap::GetNotCoveringTests() const { AZStd::vector notCovering; - for(const auto& [testTarget, coveringSources] : m_testTargetSourceCoverageCount) + for(const auto& [testTarget, coveringSources] : m_testTargetSourceCoverage) { - if(coveringSources == 0) + if (coveringSources.empty()) { notCovering.push_back(testTarget); } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h index f209e22823..cd01a1a728 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h @@ -100,6 +100,9 @@ namespace TestImpact //! @returns The change list as resolved to the appropriate source dependencies. [[nodiscard]] ChangeDependencyList ApplyAndResoveChangeList(const ChangeList& changeList); + //! Removes the specified test target from all source coverage. + void RemoveTestTargetFromSourceCoverage(const TestTarget* testTarget); + //! Returns the test targets that cover one or more sources in the repository. AZStd::vector GetCoveringTests() const; @@ -120,13 +123,13 @@ namespace TestImpact //! The dependency map of sources to their parent build targets and covering test targets. AZStd::unordered_map m_sourceDependencyMap; + //! Map of all test targets and the sources they cover. + AZStd::unordered_map> m_testTargetSourceCoverage; + //! The map of build targets and their covering test targets. AZStd::unordered_map> m_buildTargetCoverage; //! Mapping of autogen input sources to their generated output sources. AZStd::unordered_map> m_autogenInputToOutputMap; - - //! Number of sources that each test target in the repository covers. - AZStd::unordered_map m_testTargetSourceCoverageCount; }; } // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp index 2331c76880..44657dad21 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp @@ -70,11 +70,20 @@ namespace TestImpact }; } + template + AZStd::vector ConcatenateVectors(const AZStd::vector& v1, const AZStd::vector& v2) + { + AZStd::vector result; + result.reserve(v1.size() + v2.size()); + result.insert(result.end(), v1.begin(), v1.end()); + result.insert(result.end(), v2.begin(), v2.end()); + return result; + } Runtime::Runtime( RuntimeConfig&& config, SuiteType suiteFilter, Policy::ExecutionFailure executionFailurePolicy, - Policy::ExecutionFailureDrafting executionFailureDraftingPolicy, + Policy::FailedTestCoverage failedTestCoveragePolicy, Policy::TestFailure testFailurePolicy, Policy::IntegrityFailure integrationFailurePolicy, Policy::TestSharding testShardingPolicy, @@ -83,7 +92,7 @@ namespace TestImpact : m_config(AZStd::move(config)) , m_suiteFilter(suiteFilter) , m_executionFailurePolicy(executionFailurePolicy) - , m_executionFailureDraftingPolicy(executionFailureDraftingPolicy) + , m_failedTestCoveragePolicy(failedTestCoveragePolicy) , m_testFailurePolicy(testFailurePolicy) , m_integrationFailurePolicy(integrationFailurePolicy) , m_testShardingPolicy(testShardingPolicy) @@ -145,7 +154,7 @@ namespace TestImpact { AZ_Printf("TestImpactRuntime", AZStd::string::format( - "No test impact analysis data found for suite '%s' at %s", GetSuiteTypeName(m_suiteFilter).c_str(), m_sparTIAFile.c_str()).c_str()); + "No test impact analysis data found for suite '%s' at %s\n", GetSuiteTypeName(m_suiteFilter).c_str(), m_sparTIAFile.c_str()).c_str()); } } @@ -249,14 +258,28 @@ namespace TestImpact m_dynamicDependencyMap->ClearAllSourceCoverage(); } - void Runtime::UpdateAndSerializeDynamicDependencyMap(const SourceCoveringTestsList& sourceCoverageTestsList) + void Runtime::UpdateAndSerializeDynamicDependencyMap(const AZStd::vector& jobs) { + const auto sourceCoverageTestsList = CreateSourceCoveringTestFromTestCoverages(jobs, m_config.m_repo.m_root); 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(sparTIAData, m_sparTIAFile); @@ -323,11 +346,12 @@ namespace TestImpact AZStd::optional testCompleteCallback) { Timer timer; - AZStd::vector draftedTestTargets; + AZStd::vector draftedTestTargets = m_dynamicDependencyMap->GetNotCoveringTests(); auto [selectedTestTargets, discardedTestTargets] = SelectCoveringTestTargetsAndUpdateEnumerationCache(changeList, testPrioritizationPolicy); auto [includedSelectedTestTargets, excludedSelectedTestTargets] = SelectTestTargetsByExcludeList(selectedTestTargets); + AZStd::vector testTargetsToRun = ConcatenateVectors(includedSelectedTestTargets, draftedTestTargets); if (testSequenceStartCallback.has_value()) { (*testSequenceStartCallback)( @@ -336,10 +360,11 @@ namespace TestImpact ExtractTestTargetNames(draftedTestTargets)); } + if (dynamicDependencyMapPolicy == Policy::DynamicDependencyMap::Update) { const auto [result, testJobs] = m_testEngine->InstrumentedRun( - includedSelectedTestTargets, + testTargetsToRun, m_testShardingPolicy, m_executionFailurePolicy, Policy::IntegrityFailure::Continue, @@ -349,7 +374,7 @@ namespace TestImpact globalTimeout, TestRunCompleteCallbackHandler(testCompleteCallback)); - UpdateAndSerializeDynamicDependencyMap(CreateSourceCoveringTestFromTestCoverages(testJobs, m_config.m_repo.m_root)); + UpdateAndSerializeDynamicDependencyMap(testJobs); if (testSequenceEndCallback.has_value()) { @@ -361,7 +386,7 @@ namespace TestImpact else { const auto [result, testJobs] = m_testEngine->RegularRun( - includedSelectedTestTargets, + testTargetsToRun, m_testShardingPolicy, m_executionFailurePolicy, m_testFailurePolicy, @@ -389,12 +414,13 @@ namespace TestImpact AZStd::optional testCompleteCallback) { Timer timer; - AZStd::vector draftedTestTargets; + AZStd::vector draftedTestTargets = m_dynamicDependencyMap->GetNotCoveringTests(); auto [selectedTestTargets, discardedTestTargets] = SelectCoveringTestTargetsAndUpdateEnumerationCache(changeList, testPrioritizationPolicy); auto [includedSelectedTestTargets, excludedSelectedTestTargets] = SelectTestTargetsByExcludeList(selectedTestTargets); auto [includedDiscardedTestTargets, excludedDiscardedTestTargets] = SelectTestTargetsByExcludeList(discardedTestTargets); + AZStd::vector testTargetsToRun = ConcatenateVectors(includedSelectedTestTargets, draftedTestTargets); if (testSequenceStartCallback.has_value()) { (*testSequenceStartCallback)( @@ -403,9 +429,10 @@ namespace TestImpact ExtractTestTargetNames(draftedTestTargets)); } + // Impact analysis run of the selected test targets const auto [selectedResult, selectedTestJobs] = m_testEngine->InstrumentedRun( - includedSelectedTestTargets, + testTargetsToRun, m_testShardingPolicy, m_executionFailurePolicy, Policy::IntegrityFailure::Continue, @@ -433,7 +460,7 @@ namespace TestImpact globalTimeout, TestRunCompleteCallbackHandler(testCompleteCallback)); - UpdateAndSerializeDynamicDependencyMap(CreateSourceCoveringTestFromTestCoverages(selectedTestJobs, m_config.m_repo.m_root)); + UpdateAndSerializeDynamicDependencyMap(selectedTestJobs); if (testSequenceEndCallback.has_value()) { @@ -486,7 +513,7 @@ namespace TestImpact TestRunCompleteCallbackHandler(testCompleteCallback)); ClearDynamicDependencyMapAndRemoveExistingFile(); - UpdateAndSerializeDynamicDependencyMap(CreateSourceCoveringTestFromTestCoverages(testJobs, m_config.m_repo.m_root)); + UpdateAndSerializeDynamicDependencyMap(testJobs); if (testSequenceEndCallback.has_value()) { From 0da739ef18c41ec1ecb29cb563b69d676ef5d7cd Mon Sep 17 00:00:00 2001 From: jonawals Date: Mon, 7 Jun 2021 09:05:29 +0100 Subject: [PATCH 2/3] Add curiously missing comments --- .../Runtime/Code/Source/TestImpactRuntime.cpp | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp index 44657dad21..d83976b488 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp @@ -70,6 +70,7 @@ namespace TestImpact }; } + //! Utility for concatenating two vectors. template AZStd::vector ConcatenateVectors(const AZStd::vector& v1, const AZStd::vector& v2) { @@ -79,6 +80,7 @@ namespace TestImpact result.insert(result.end(), v2.begin(), v2.end()); return result; } + Runtime::Runtime( RuntimeConfig&& config, SuiteType suiteFilter, @@ -346,12 +348,20 @@ namespace TestImpact AZStd::optional testCompleteCallback) { Timer timer; + + // Draft in the test targets that have no coverage entries in the dynamic dependency map AZStd::vector draftedTestTargets = m_dynamicDependencyMap->GetNotCoveringTests(); + // The test targets that were selected for the change list by the dynamic dependency map and the test targets that were not auto [selectedTestTargets, discardedTestTargets] = SelectCoveringTestTargetsAndUpdateEnumerationCache(changeList, testPrioritizationPolicy); + + // The subset of selected test targets that are not on the configuration's exclude list and those that are auto [includedSelectedTestTargets, excludedSelectedTestTargets] = SelectTestTargetsByExcludeList(selectedTestTargets); + // We present to the client the included selected test targets and the drafted test targets as distinct sets but internally + // we consider the concatenated set of the two the actual set of tests to run AZStd::vector testTargetsToRun = ConcatenateVectors(includedSelectedTestTargets, draftedTestTargets); + if (testSequenceStartCallback.has_value()) { (*testSequenceStartCallback)( @@ -360,7 +370,6 @@ namespace TestImpact ExtractTestTargetNames(draftedTestTargets)); } - if (dynamicDependencyMapPolicy == Policy::DynamicDependencyMap::Update) { const auto [result, testJobs] = m_testEngine->InstrumentedRun( @@ -413,14 +422,24 @@ 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(); + // The test targets that were selected for the change list by the dynamic dependency map and the test targets that were not auto [selectedTestTargets, discardedTestTargets] = SelectCoveringTestTargetsAndUpdateEnumerationCache(changeList, testPrioritizationPolicy); + + // The subset of selected test targets that are not on the configuration's exclude list and those that are auto [includedSelectedTestTargets, excludedSelectedTestTargets] = SelectTestTargetsByExcludeList(selectedTestTargets); + + // The subset of discarded test targets that are not on the configuration's exclude list and those that are auto [includedDiscardedTestTargets, excludedDiscardedTestTargets] = SelectTestTargetsByExcludeList(discardedTestTargets); + // We present to the client the included selected test targets and the drafted test targets as distinct sets but internally + // we consider the concatenated set of the two the actual set of tests to run AZStd::vector testTargetsToRun = ConcatenateVectors(includedSelectedTestTargets, draftedTestTargets); + if (testSequenceStartCallback.has_value()) { (*testSequenceStartCallback)( @@ -429,7 +448,6 @@ namespace TestImpact ExtractTestTargetNames(draftedTestTargets)); } - // Impact analysis run of the selected test targets const auto [selectedResult, selectedTestJobs] = m_testEngine->InstrumentedRun( testTargetsToRun, From d341dab7f54d866849a2c1f96190808e7e0f622b Mon Sep 17 00:00:00 2001 From: jonawals Date: Mon, 7 Jun 2021 10:39:58 +0100 Subject: [PATCH 3/3] Address PR comments --- .../Static/Code/Source/TestImpactCommandLineOptions.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 aad9319cf9..01005718bc 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactCommandLineOptions.cpp @@ -405,9 +405,9 @@ 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 test that \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" + " 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" " -targetout= Capture of individual test run stdout, where stdout will capture \n"