Address PR comments

2021-06-02 11:16:36 +01:00
parent 9be54d4e5b
commit 105eca52d7
3 changed files with 147 additions and 65 deletions
@@ -18,9 +18,101 @@
 #include <AzCore/std/functional.h>
 #include <AzCore/std/optional.h>

-
 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;
    }
@@ -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;
@@ -101,8 +101,8 @@ namespace TestImpact
        m_testEngine = AZStd::make_unique<TestEngine>(
            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<Exception>(m_config.m_workspace.m_active.m_relativePaths.m_sparTIAFile);
+            const auto tiaDataRaw = ReadFileContents<Exception>(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<RuntimeException>(sparTIAData, m_config.m_workspace.m_active.m_relativePaths.m_sparTIAFile);
+        WriteFileContents<RuntimeException>(sparTIAData, m_config.m_workspace.m_active.m_sparTIAFile);
        m_hasImpactAnalysisData = true;
    }