From 39ba583ad7ae1761e13d356bd50b02ae29e3f6b4 Mon Sep 17 00:00:00 2001 From: jonawals Date: Fri, 30 Apr 2021 12:58:48 +0100 Subject: [PATCH] Address PR comments. --- .../TestImpactDynamicDependencyMap.cpp | 110 +++++++++--------- .../TestImpactDynamicDependencyMap.h | 5 +- .../TestImpactSourceCoveringTestsList.cpp | 7 +- .../TestImpactSourceCoveringTestsList.h | 3 +- .../Dependency/TestImpactSourceDependency.cpp | 19 +-- .../Dependency/TestImpactSourceDependency.h | 11 +- .../Source/Target/TestImpactBuildTarget.h | 10 ++ 7 files changed, 89 insertions(+), 76 deletions(-) diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp index f3edbe2507..f99300ad4e 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.cpp @@ -25,7 +25,8 @@ namespace TestImpact { for (const auto& source : target->GetSources().m_staticSources) { - if (auto mapping = m_sourceDependencyMap.find(source); mapping != m_sourceDependencyMap.end()) + if (auto mapping = m_sourceDependencyMap.find(source); + mapping != m_sourceDependencyMap.end()) { // This is an existing entry in the dependency map so update the parent build targets with this target mapping->second.m_parentTargets.insert(target); @@ -97,13 +98,15 @@ namespace TestImpact return buildTarget; } - AZStd::variant DynamicDependencyMap::GetTarget(const AZStd::string& name) const + OptionalTarget DynamicDependencyMap::GetTarget(const AZStd::string& name) const { - if (auto testTarget = m_testTargets.GetTarget(name); testTarget != nullptr) + if (const auto testTarget = m_testTargets.GetTarget(name); + testTarget != nullptr) { return testTarget; } - else if (auto productionTarget = m_productionTargets.GetTarget(name); productionTarget != nullptr) + else if (auto productionTarget = m_productionTargets.GetTarget(name); + productionTarget != nullptr) { return productionTarget; } @@ -111,9 +114,9 @@ namespace TestImpact return AZStd::monostate{}; } - AZStd::variant DynamicDependencyMap::GetTargetOrThrow(const AZStd::string& name) const + Target DynamicDependencyMap::GetTargetOrThrow(const AZStd::string& name) const { - AZStd::variant buildTarget; + Target buildTarget; AZStd::visit([&buildTarget, &name](auto&& target) { if constexpr (IsProductionTarget || IsTestTarget) @@ -148,8 +151,8 @@ namespace TestImpact // Update the dependency with any new coverage data for (const auto& unresolvedTestTarget : sourceCoverage.GetCoveringTestTargets()) { - const TestTarget* testTarget = m_testTargets.GetTarget(unresolvedTestTarget); - if (testTarget) + if (const TestTarget* testTarget = m_testTargets.GetTarget(unresolvedTestTarget); + testTarget) { // Source to covering test target mapping sourceDependency.m_coveringTestTargets.insert(testTarget); @@ -179,12 +182,13 @@ namespace TestImpact { for (const auto& path : paths) { - if (const auto outputSources = m_autogenInputToOutputMap.find(path); outputSources != m_autogenInputToOutputMap.end()) + if (const auto outputSources = m_autogenInputToOutputMap.find(path); + outputSources != m_autogenInputToOutputMap.end()) { // Clearing the coverage data of an autogen input source instead clears the coverage data of its output sources for (const auto& outputSource : outputSources->second) { - ReplaceSourceCoverage(SourceCoveringTestsList({ SourceCoveringTests(outputSource, { }) })); + ReplaceSourceCoverage(SourceCoveringTestsList({ SourceCoveringTests(outputSource) })); } } else @@ -207,7 +211,8 @@ namespace TestImpact AZStd::vector DynamicDependencyMap::GetCoveringTestTargetsForProductionTarget(const ProductionTarget& productionTarget) const { AZStd::vector coveringTestTargets; - if (const auto coverage = m_buildTargetCoverage.find(&productionTarget); coverage != m_buildTargetCoverage.end()) + if (const auto coverage = m_buildTargetCoverage.find(&productionTarget); + coverage != m_buildTargetCoverage.end()) { coveringTestTargets.reserve(coverage->second.size()); AZStd::copy(coverage->second.begin(), coverage->second.end(), AZStd::back_inserter(coveringTestTargets)); @@ -280,7 +285,7 @@ namespace TestImpact coverage.push_back(SourceCoveringTests(path, AZStd::move(souceCoveringTests))); } - return coverage; + return SourceCoveringTestsList(AZStd::move(coverage)); } AZStd::vector DynamicDependencyMap::GetOrphanSourceFiles() const @@ -313,31 +318,18 @@ namespace TestImpact auto sourceDependency = GetSourceDependency(createdFile); if (sourceDependency.has_value()) { + if (sourceDependency->GetNumCoveringTestTargets()) + { + const AZStd::string msg = AZStd::string::format("The newly-created file %s belongs to a build target yet " + "still has coverage data in the source covering test list implying that a delete CRUD operation has been " + "missed, thus the integrity of the source covering test list has been compromised", createdFile.c_str()); + AZ_Error("File Creation", false, msg.c_str()); + throw DependencyException(msg); + } + if (sourceDependency->GetNumParentTargets()) { - if (sourceDependency->GetNumCoveringTestTargets()) - { - const AZStd::string msg = AZStd::string::format("The newly-created file %s belongs to a build target yet " - "still has coverage data in the source covering test list implying that a delete CRUD operation has been " - "missed, thus the integrity of the source covering test list has been compromised", createdFile.c_str()); - AZ_Error("File Creation", false, msg.c_str()); - throw DependencyException(msg); - } - else - { - createDependencies.emplace_back(AZStd::move(*sourceDependency)); - } - } - else - { - if (sourceDependency->GetNumCoveringTestTargets()) - { - const AZStd::string msg = AZStd::string::format("The newly-created file %s does not belong to a build target " - "yet still has coverage data in the source covering test list, implying that a delete CRUD operation has been " - "missed, thus the integrity of the source covering test list has been compromised", createdFile.c_str()); - AZ_Error("File Creation", false, msg.c_str()); - throw DependencyException(msg); - } + createDependencies.emplace_back(AZStd::move(*sourceDependency)); } } } @@ -373,33 +365,35 @@ namespace TestImpact for (const auto& deletedFile : changeList.m_deletedFiles) { auto sourceDependency = GetSourceDependency(deletedFile); - if (sourceDependency.has_value()) + if (!sourceDependency.has_value()) { - if (sourceDependency->GetNumParentTargets()) + continue; + } + + if (sourceDependency->GetNumParentTargets()) + { + if (sourceDependency->GetNumCoveringTestTargets()) { - if (sourceDependency->GetNumCoveringTestTargets()) - { - const AZStd::string msg = AZStd::string::format("The deleted file %s still belongs to a build target and still " - "has coverage data in the source covering test list, implying that the integrity of both the source to target " - "mappings and the source covering test list has been compromised", deletedFile.c_str()); - AZ_Error("File Delete", false, msg.c_str()); - throw DependencyException(msg); - } - else - { - const AZStd::string msg = AZStd::string::format("The deleted file %s still belongs to a build target implying " - "that the integrity of the source to target mappings has been compromised", deletedFile.c_str()); - AZ_Error("File Delete", false, msg.c_str()); - throw DependencyException(msg); - } + const AZStd::string msg = AZStd::string::format("The deleted file %s still belongs to a build target and still " + "has coverage data in the source covering test list, implying that the integrity of both the source to target " + "mappings and the source covering test list has been compromised", deletedFile.c_str()); + AZ_Error("File Delete", false, msg.c_str()); + throw DependencyException(msg); } else { - if (sourceDependency->GetNumCoveringTestTargets()) - { - deleteDependencies.emplace_back(AZStd::move(*sourceDependency)); - coverageToDelete.push_back(deletedFile); - } + const AZStd::string msg = AZStd::string::format("The deleted file %s still belongs to a build target implying " + "that the integrity of the source to target mappings has been compromised", deletedFile.c_str()); + AZ_Error("File Delete", false, msg.c_str()); + throw DependencyException(msg); + } + } + else + { + if (sourceDependency->GetNumCoveringTestTargets()) + { + deleteDependencies.emplace_back(AZStd::move(*sourceDependency)); + coverageToDelete.push_back(deletedFile); } } } @@ -411,4 +405,4 @@ namespace TestImpact return ChangeDependencyList(AZStd::move(createDependencies), AZStd::move(updateDependencies), AZStd::move(deleteDependencies)); } -} +} // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h index 0057fdc264..527a6555c6 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactDynamicDependencyMap.h @@ -24,7 +24,6 @@ #include #include #include -#include namespace TestImpact { @@ -56,11 +55,11 @@ namespace TestImpact //! Attempts to get the specified target's specialized type. //! @param name The name of the target to get. //! @returns If found, the pointer to the specialized target, otherwise AZStd::monostate. - AZStd::variant GetTarget(const AZStd::string& name) const; + OptionalTarget GetTarget(const AZStd::string& name) const; //! Attempts to get the specified target's specialized type or throw TargetException. //! @param name The name of the target to get. - AZStd::variant GetTargetOrThrow(const AZStd::string& name) const; + Target GetTargetOrThrow(const AZStd::string& name) const; //! Get the list of production targets in the repository. const ProductionTargetList& GetProductionTargetList() const; diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.cpp index 57a11e9f69..792eb1f0b2 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.cpp @@ -16,6 +16,11 @@ namespace TestImpact { + SourceCoveringTests::SourceCoveringTests(const AZStd::string& path) + : m_path(path) + { + } + SourceCoveringTests::SourceCoveringTests(const AZStd::string& path, AZStd::vector&& coveringTestTargets) : m_path(path) , m_coveringTestTargets(AZStd::move(coveringTestTargets)) @@ -55,4 +60,4 @@ namespace TestImpact { return m_coverage; } -} +} // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.h b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.h index 68b09d9e5a..52f3a7c3f6 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceCoveringTestsList.h @@ -21,6 +21,7 @@ namespace TestImpact class SourceCoveringTests { public: + explicit SourceCoveringTests(const AZStd::string& path); SourceCoveringTests(const AZStd::string& path, AZStd::vector&& coveringTestTargets); //! Returns the path of this source file. @@ -40,7 +41,7 @@ namespace TestImpact class SourceCoveringTestsList { public: - SourceCoveringTestsList(AZStd::vector&& sourceCoveringTests); + explicit SourceCoveringTestsList(AZStd::vector&& sourceCoveringTests); //! Returns the number of source files in the collection. size_t GetNumSources() const; diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.cpp index 49d82e341a..39193d4dd9 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.cpp @@ -20,28 +20,33 @@ namespace TestImpact { ParentTarget::ParentTarget(const TestTarget* target) - : m_buildTarget(target) - , m_target(target) + : m_target(target) { } ParentTarget::ParentTarget(const ProductionTarget* target) - : m_buildTarget(target) - , m_target(target) + : m_target(target) { } bool ParentTarget::operator==(const ParentTarget& other) const { - return m_buildTarget == other.m_buildTarget; + return GetBuildTarget() == other.GetBuildTarget(); } const BuildTarget* ParentTarget::GetBuildTarget() const { - return m_buildTarget; + const BuildTarget* buildTarget; + AZStd::visit([&buildTarget](auto&& target) + { + buildTarget = target; + + }, m_target); + + return buildTarget; } - const AZStd::variant& ParentTarget::GetTarget() const + const Target& ParentTarget::GetTarget() const { return m_target; } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.h b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.h index e6cd89df9d..a3b6783d13 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Dependency/TestImpactSourceDependency.h @@ -12,15 +12,15 @@ #pragma once +#include + #include #include #include #include -#include namespace TestImpact { - class BuildTarget; class ProductionTarget; class TestTarget; @@ -38,12 +38,11 @@ namespace TestImpact const BuildTarget* GetBuildTarget() const; //! Returns the specialized target pointer for this parent. - const AZStd::variant& GetTarget() const; + const Target& GetTarget() const; bool operator==(const ParentTarget& other) const; private: - const BuildTarget* m_buildTarget; //! The base built target pointer for this parent. - AZStd::variant m_target; //! The specialized target pointer for this parent. + Target m_target; //! The specialized target pointer for this parent. }; } @@ -91,6 +90,6 @@ namespace TestImpact const AZStd::unordered_set& GetCoveringTestTargets() const; private: AZStd::string m_path; //!< The path of this source file. - DependencyData m_dependencyData; //!< + DependencyData m_dependencyData; //!< The dependency data for this source file. }; } // namespace TestImpact diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Target/TestImpactBuildTarget.h b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Target/TestImpactBuildTarget.h index d4346c15ef..0cd6369472 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/Target/TestImpactBuildTarget.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/Target/TestImpactBuildTarget.h @@ -14,10 +14,20 @@ #include +#include #include namespace TestImpact { + class TestTarget; + class ProductionTarget; + + //! Holder for specializations of BuildTarget. + using Target = AZStd::variant; + + //! Optional holder for specializations of BuildTarget. + using OptionalTarget = AZStd::variant; + //! Type id for querying specialized derived target types from base pointer/reference. enum class TargetType : bool {