From 105eca52d7f3b06fb72f2f9905246f5c449d073a Mon Sep 17 00:00:00 2001 From: jonawals Date: Wed, 2 Jun 2021 11:16:36 +0100 Subject: [PATCH] Address PR comments --- .../TestImpactRuntimeConfigurationFactory.cpp | 182 +++++++++++++----- .../TestImpactConfiguration.h | 18 +- .../Runtime/Code/Source/TestImpactRuntime.cpp | 12 +- 3 files changed, 147 insertions(+), 65 deletions(-) diff --git a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactRuntimeConfigurationFactory.cpp b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactRuntimeConfigurationFactory.cpp index 4b473b7190..9db99d3c2e 100644 --- a/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactRuntimeConfigurationFactory.cpp +++ b/Code/Tools/TestImpactFramework/Frontend/Console/Static/Code/Source/TestImpactRuntimeConfigurationFactory.cpp @@ -18,9 +18,101 @@ #include #include - namespace TestImpact { + namespace Config + { + // Keys for pertinent JSON elements + constexpr const char* Keys[] = + { + "root", + "platform", + "relative_paths", + "artifact_dir", + "enumeration_cache_dir", + "test_impact_data_file", + "temp", + "active", + "target_sources", + "static", + "autogen", + "static", + "include_filters", + "input_output_pairer", + "input", + "dir", + "matchers", + "target_dependency_file", + "target_vertex", + "file", + "test_runner", + "instrumentation", + "bin", + "exclude", + "shard", + "fixture_contiguous", + "fixture_interleaved", + "test_contiguous", + "test_interleaved", + "never", + "target", + "policy", + "artifacts", + "meta", + "repo", + "workspace", + "build_target_descriptor", + "dependency_graph_data", + "test_target_meta", + "test_engine", + "target" + }; + + enum + { + Root = 0, + PlatformName, + RelativePaths, + ArtifactDir, + EnumerationCacheDir, + TestImpactDataFile, + TempWorkspace, + ActiveWorkspace, + TargetSources, + StaticSources, + AutogenSources, + StaticArtifacts, + SourceIncludeFilters, + AutogenInputOutputPairer, + AutogenInputSources, + Directory, + DependencyGraphMatchers, + TargetDependencyFileMatcher, + TargetVertexMatcher, + TestTargetMetaFile, + TestRunner, + TestInstrumentation, + BinaryFile, + TargetExcludeFilter, + TestSharding, + ContinuousFixtureSharding, + InterleavedFixtureSharding, + ContinuousTestSharding, + InterleavedTestSharding, + NeverShard, + TargetName, + TestShardingPolicy, + Artifacts, + Meta, + Repository, + Workspace, + BuildTargetDescriptor, + DependencyGraphData, + TestTargetMeta, + TestEngine, + TargetConfig + }; + } //! Returns an absolute path for a path relative to the specified root. RepoPath GetAbsPathFromRelPath(const RepoPath& root, const RepoPath& rel) @@ -31,54 +123,55 @@ namespace TestImpact ConfigMeta ParseConfigMeta(const rapidjson::Value& meta) { ConfigMeta configMeta; - configMeta.m_platform = meta["platform"].GetString(); + configMeta.m_platform = meta[Config::Keys[Config::PlatformName]].GetString(); return configMeta; } RepoConfig ParseRepoConfig(const rapidjson::Value& repo) { RepoConfig repoConfig; - repoConfig.m_root = repo["root"].GetString(); + repoConfig.m_root = repo[Config::Keys[Config::Root]].GetString(); return repoConfig; } WorkspaceConfig::Temp ParseTempWorkspaceConfig(const rapidjson::Value& tempWorkspace) { WorkspaceConfig::Temp tempWorkspaceConfig; - tempWorkspaceConfig.m_root = tempWorkspace["root"].GetString(); + tempWorkspaceConfig.m_root = tempWorkspace[Config::Keys[Config::Root]].GetString(); tempWorkspaceConfig.m_artifactDirectory = - GetAbsPathFromRelPath(tempWorkspaceConfig.m_root, tempWorkspace["relative_paths"]["artifact_dir"].GetString()); + GetAbsPathFromRelPath( + tempWorkspaceConfig.m_root, tempWorkspace[Config::Keys[Config::RelativePaths]][Config::Keys[Config::ArtifactDir]].GetString()); return tempWorkspaceConfig; } WorkspaceConfig::Active ParseActiveWorkspaceConfig(const rapidjson::Value& activeWorkspace) { WorkspaceConfig::Active activeWorkspaceConfig; - const auto& relativePaths = activeWorkspace["relative_paths"]; - activeWorkspaceConfig.m_root = activeWorkspace["root"].GetString(); + const auto& relativePaths = activeWorkspace[Config::Keys[Config::RelativePaths]]; + activeWorkspaceConfig.m_root = activeWorkspace[Config::Keys[Config::Root]].GetString(); activeWorkspaceConfig.m_enumerationCacheDirectory - = GetAbsPathFromRelPath(activeWorkspaceConfig.m_root, relativePaths["enumeration_cache_dir"].GetString()); + = GetAbsPathFromRelPath(activeWorkspaceConfig.m_root, relativePaths[Config::Keys[Config::EnumerationCacheDir]].GetString()); activeWorkspaceConfig.m_sparTIAFile - = GetAbsPathFromRelPath(activeWorkspaceConfig.m_root, relativePaths["test_impact_data_file"].GetString()); + = GetAbsPathFromRelPath(activeWorkspaceConfig.m_root, relativePaths[Config::Keys[Config::TestImpactDataFile]].GetString()); return activeWorkspaceConfig; } WorkspaceConfig ParseWorkspaceConfig(const rapidjson::Value& workspace) { WorkspaceConfig workspaceConfig; - workspaceConfig.m_temp = ParseTempWorkspaceConfig(workspace["temp"]); - workspaceConfig.m_active = ParseActiveWorkspaceConfig(workspace["active"]); + workspaceConfig.m_temp = ParseTempWorkspaceConfig(workspace[Config::Keys[Config::TempWorkspace]]); + workspaceConfig.m_active = ParseActiveWorkspaceConfig(workspace[Config::Keys[Config::ActiveWorkspace]]); return workspaceConfig; } BuildTargetDescriptorConfig ParseBuildTargetDescriptorConfig(const rapidjson::Value& buildTargetDescriptor) { BuildTargetDescriptorConfig buildTargetDescriptorConfig; - const auto& targetSources = buildTargetDescriptor["target_sources"]; - const auto& staticTargetSources = targetSources["static"]; - const auto& autogenTargetSources = targetSources["autogen"]; - buildTargetDescriptorConfig.m_mappingDirectory = buildTargetDescriptor["dir"].GetString(); - const auto& staticInclusionFilters = staticTargetSources["include_filters"].GetArray(); + const auto& targetSources = buildTargetDescriptor[Config::Keys[Config::TargetSources]]; + const auto& staticTargetSources = targetSources[Config::Keys[Config::StaticSources]]; + const auto& autogenTargetSources = targetSources[Config::Keys[Config::AutogenSources]]; + buildTargetDescriptorConfig.m_mappingDirectory = buildTargetDescriptor[Config::Keys[Config::Directory]].GetString(); + const auto& staticInclusionFilters = staticTargetSources[Config::Keys[Config::SourceIncludeFilters]].GetArray(); buildTargetDescriptorConfig.m_staticInclusionFilters.reserve(staticInclusionFilters.Size()); for (const auto& staticInclusionFilter : staticInclusionFilters) @@ -86,8 +179,9 @@ namespace TestImpact buildTargetDescriptorConfig.m_staticInclusionFilters.push_back(staticInclusionFilter.GetString()); } - buildTargetDescriptorConfig.m_inputOutputPairer = autogenTargetSources["input_output_pairer"].GetString(); - const auto& inputInclusionFilters = autogenTargetSources["input"]["include_filters"].GetArray(); + buildTargetDescriptorConfig.m_inputOutputPairer = autogenTargetSources[Config::Keys[Config::AutogenInputOutputPairer]].GetString(); + const auto& inputInclusionFilters = + autogenTargetSources[Config::Keys[Config::AutogenInputSources]][Config::Keys[Config::SourceIncludeFilters]].GetArray(); buildTargetDescriptorConfig.m_inputInclusionFilters.reserve(inputInclusionFilters.Size()); for (const auto& inputInclusionFilter : inputInclusionFilters) { @@ -100,62 +194,62 @@ namespace TestImpact DependencyGraphDataConfig ParseDependencyGraphDataConfig(const rapidjson::Value& dependencyGraphData) { DependencyGraphDataConfig dependencyGraphDataConfig; - const auto& matchers = dependencyGraphData["matchers"]; - dependencyGraphDataConfig.m_graphDirectory = dependencyGraphData["dir"].GetString(); - dependencyGraphDataConfig.m_targetDependencyFileMatcher = matchers["target_dependency_file"].GetString(); - dependencyGraphDataConfig.m_targetVertexMatcher = matchers["target_vertex"].GetString(); + const auto& matchers = dependencyGraphData[Config::Keys[Config::DependencyGraphMatchers]]; + dependencyGraphDataConfig.m_graphDirectory = dependencyGraphData[Config::Keys[Config::Directory]].GetString(); + dependencyGraphDataConfig.m_targetDependencyFileMatcher = matchers[Config::Keys[Config::TargetDependencyFileMatcher]].GetString(); + dependencyGraphDataConfig.m_targetVertexMatcher = matchers[Config::Keys[Config::TargetVertexMatcher]].GetString(); return dependencyGraphDataConfig; } TestTargetMetaConfig ParseTestTargetMetaConfig(const rapidjson::Value& testTargetMeta) { TestTargetMetaConfig testTargetMetaConfig; - testTargetMetaConfig.m_metaFile = testTargetMeta["file"].GetString(); + testTargetMetaConfig.m_metaFile = testTargetMeta[Config::Keys[Config::TestTargetMetaFile]].GetString(); return testTargetMetaConfig; } TestEngineConfig ParseTestEngineConfig(const rapidjson::Value& testEngine) { TestEngineConfig testEngineConfig; - testEngineConfig.m_testRunner.m_binary = testEngine["test_runner"]["bin"].GetString(); - testEngineConfig.m_instrumentation.m_binary = testEngine["instrumentation"]["bin"].GetString(); + testEngineConfig.m_testRunner.m_binary = testEngine[Config::Keys[Config::TestRunner]][Config::Keys[Config::BinaryFile]].GetString(); + testEngineConfig.m_instrumentation.m_binary = testEngine[Config::Keys[Config::TestInstrumentation]][Config::Keys[Config::BinaryFile]].GetString(); return testEngineConfig; } TargetConfig ParseTargetConfig(const rapidjson::Value& target) { TargetConfig targetConfig; - targetConfig.m_outputDirectory = target["dir"].GetString(); - const auto& testExcludes = target["exclude"].GetArray(); + targetConfig.m_outputDirectory = target[Config::Keys[Config::Directory]].GetString(); + const auto& testExcludes = target[Config::Keys[Config::TargetExcludeFilter]].GetArray(); targetConfig.m_excludedTestTargets.reserve(testExcludes.Size()); for (const auto& testExclude : testExcludes) { targetConfig.m_excludedTestTargets.push_back(testExclude.GetString()); } - const auto& testShards = target["shard"].GetArray(); + const auto& testShards = target[Config::Keys[Config::TestSharding]].GetArray(); targetConfig.m_shardedTestTargets.reserve(testShards.Size()); for (const auto& testShard : testShards) { const auto getShardingConfiguration = [](const AZStd::string& config) { - if (config == "fixture_contiguous") + if (config == Config::Keys[Config::ContinuousFixtureSharding]) { return ShardConfiguration::FixtureContiguous; } - else if (config == "fixture_interleaved") + else if (config == Config::Keys[Config::InterleavedFixtureSharding]) { return ShardConfiguration::FixtureInterleaved; } - else if (config == "test_contiguous") + else if (config == Config::Keys[Config::ContinuousTestSharding]) { return ShardConfiguration::TestContiguous; } - else if (config == "test_interleaved") + else if (config == Config::Keys[Config::InterleavedTestSharding]) { return ShardConfiguration::TestInterleaved; } - else if (config == "never") + else if (config == Config::Keys[Config::NeverShard]) { return ShardConfiguration::Never; } @@ -166,8 +260,8 @@ namespace TestImpact }; TargetConfig::ShardedTarget shard; - shard.m_name = testShard["target"].GetString(); - shard.m_configuration = getShardingConfiguration(testShard["policy"].GetString()); + shard.m_name = testShard[Config::Keys[Config::TargetName]].GetString(); + shard.m_configuration = getShardingConfiguration(testShard[Config::Keys[Config::TestShardingPolicy]].GetString()); targetConfig.m_shardedTestTargets.push_back(AZStd::move(shard)); } @@ -184,15 +278,15 @@ namespace TestImpact } RuntimeConfig runtimeConfig; - const auto& staticArtifacts = configurationFile["artifacts"]["static"]; - runtimeConfig.m_meta = ParseConfigMeta(configurationFile["meta"]); - runtimeConfig.m_repo = ParseRepoConfig(configurationFile["repo"]); - runtimeConfig.m_workspace = ParseWorkspaceConfig(configurationFile["workspace"]); - runtimeConfig.m_buildTargetDescriptor = ParseBuildTargetDescriptorConfig(staticArtifacts["build_target_descriptor"]); - runtimeConfig.m_dependencyGraphData = ParseDependencyGraphDataConfig(staticArtifacts["dependency_graph_data"]); - runtimeConfig.m_testTargetMeta = ParseTestTargetMetaConfig(staticArtifacts["static"]["test_target_meta"]); - runtimeConfig.m_testEngine = ParseTestEngineConfig(configurationFile["test_engine"]); - runtimeConfig.m_target = ParseTargetConfig(configurationFile["target"]); + const auto& staticArtifacts = configurationFile[Config::Keys[Config::Artifacts]][Config::Keys[Config::StaticArtifacts]]; + runtimeConfig.m_meta = ParseConfigMeta(configurationFile[Config::Keys[Config::Meta]]); + runtimeConfig.m_repo = ParseRepoConfig(configurationFile[Config::Keys[Config::Repository]]); + runtimeConfig.m_workspace = ParseWorkspaceConfig(configurationFile[Config::Keys[Config::Workspace]]); + runtimeConfig.m_buildTargetDescriptor = ParseBuildTargetDescriptorConfig(staticArtifacts[Config::Keys[Config::BuildTargetDescriptor]]); + runtimeConfig.m_dependencyGraphData = ParseDependencyGraphDataConfig(staticArtifacts[Config::Keys[Config::DependencyGraphData]]); + runtimeConfig.m_testTargetMeta = ParseTestTargetMetaConfig(staticArtifacts[Config::Keys[Config::TestTargetMeta]]); + runtimeConfig.m_testEngine = ParseTestEngineConfig(configurationFile[Config::Keys[Config::TestEngine]]); + runtimeConfig.m_target = ParseTargetConfig(configurationFile[Config::Keys[Config::TargetConfig]]); return runtimeConfig; } diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactConfiguration.h b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactConfiguration.h index 3406789c9e..91d748edfe 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactConfiguration.h +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Include/TestImpactFramework/TestImpactConfiguration.h @@ -38,28 +38,16 @@ namespace TestImpact //! Temporary workspace configuration. struct Temp { - //! Paths relative to root. - struct RelativePaths - { - RepoPath m_artifactDirectory; //!< Path to read and write runtime artifacts to and from. - }; - RepoPath m_root; //!< Path to the temporary workspace (cleaned prior to use). - RelativePaths m_relativePaths; + RepoPath m_artifactDirectory; //!< Path to read and write runtime artifacts to and from. }; //! Active persistent data workspace configuration. struct Active { - //! Paths relative to root. - struct RelativePaths - { - RepoPath m_sparTIAFile; //!< Path to the test impact analysis data. - RepoPath m_enumerationCacheDirectory; //!< Path to the test enumerations cache. - }; - RepoPath m_root; //!< Path to the persistent workspace tracked by the repository. - RelativePaths m_relativePaths; + RepoPath m_sparTIAFile; //!< Path to the test impact analysis data. + RepoPath m_enumerationCacheDirectory; //!< Path to the test enumerations cache. }; Temp m_temp; diff --git a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp index e02bee03c6..83808f4ed2 100644 --- a/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp +++ b/Code/Tools/TestImpactFramework/Runtime/Code/Source/TestImpactRuntime.cpp @@ -101,8 +101,8 @@ namespace TestImpact m_testEngine = AZStd::make_unique( m_config.m_repo.m_root, m_config.m_target.m_outputDirectory, - m_config.m_workspace.m_active.m_relativePaths.m_enumerationCacheDirectory, - m_config.m_workspace.m_temp.m_relativePaths.m_artifactDirectory, + m_config.m_workspace.m_active.m_enumerationCacheDirectory, + m_config.m_workspace.m_temp.m_artifactDirectory, m_config.m_testEngine.m_testRunner.m_binary, m_config.m_testEngine.m_instrumentation.m_binary, m_maxConcurrency); @@ -110,7 +110,7 @@ namespace TestImpact try { // Populate the dynamic dependency map with the existing source coverage data (if any) - const auto tiaDataRaw = ReadFileContents(m_config.m_workspace.m_active.m_relativePaths.m_sparTIAFile); + const auto tiaDataRaw = ReadFileContents(m_config.m_workspace.m_active.m_sparTIAFile); const auto tiaData = DeserializeSourceCoveringTestsList(tiaDataRaw); if (tiaData.GetNumSources()) { @@ -136,7 +136,7 @@ namespace TestImpact } catch ([[maybe_unused]]const Exception& e) { - AZ_Printf("No test impact analysis data found at %s", m_config.m_workspace.m_active.m_relativePaths.m_sparTIAFile.c_str()); + AZ_Printf("No test impact analysis data found at %s", m_config.m_workspace.m_active.m_sparTIAFile.c_str()); } } @@ -233,7 +233,7 @@ namespace TestImpact void Runtime::ClearDynamicDependencyMapAndRemoveExistingFile() { - DeleteFile(m_config.m_workspace.m_active.m_relativePaths.m_sparTIAFile); + DeleteFile(m_config.m_workspace.m_active.m_sparTIAFile); m_dynamicDependencyMap->ClearAllSourceCoverage(); } @@ -247,7 +247,7 @@ namespace TestImpact m_dynamicDependencyMap->ReplaceSourceCoverage(sourceCoverageTestsList); const auto sparTIA = m_dynamicDependencyMap->ExportSourceCoverage(); const auto sparTIAData = SerializeSourceCoveringTestsList(sparTIA); - WriteFileContents(sparTIAData, m_config.m_workspace.m_active.m_relativePaths.m_sparTIAFile); + WriteFileContents(sparTIAData, m_config.m_workspace.m_active.m_sparTIAFile); m_hasImpactAnalysisData = true; }