From f4e8ecd35e07a9cebcaa9126e7f36f5ae1932a62 Mon Sep 17 00:00:00 2001 From: jonawals Date: Thu, 12 Aug 2021 10:21:33 +0100 Subject: [PATCH] TIAF script fixes (#3044) * Fix typo in MARS document key. Signed-off-by: John * Add diff support for commits on different branches. Signed-off-by: John * Restore archiving of historic data objects. Signed-off-by: John * Correctly handle commit diffs for PR builds. Signed-off-by: John * Comment out archiving of historic data objects. Signed-off-by: John * Fix python typo Signed-off-by: John * Remove comment pertaining to s3 bucket. Signed-off-by: John * Fix test selection efficiency for MARS. Signed-off-by: John --- scripts/build/TestImpactAnalysis/git_utils.py | 13 ++++++-- .../build/TestImpactAnalysis/mars_utils.py | 4 +-- scripts/build/TestImpactAnalysis/tiaf.py | 32 +++++++++++++------ .../tiaf_persistent_storage_s3.py | 6 ++++ 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/scripts/build/TestImpactAnalysis/git_utils.py b/scripts/build/TestImpactAnalysis/git_utils.py index 3561b337f3..ddc17ca146 100644 --- a/scripts/build/TestImpactAnalysis/git_utils.py +++ b/scripts/build/TestImpactAnalysis/git_utils.py @@ -21,12 +21,14 @@ class Repo: branch = self._repo.active_branch return branch.name - def create_diff_file(self, src_commit_hash: str, dst_commit_hash: str, output_path: pathlib.Path): + def create_diff_file(self, src_commit_hash: str, dst_commit_hash: str, output_path: pathlib.Path, multi_branch: bool): """ Attempts to create a diff from the src and dst commits and write to the specified output file. @param src_commit_hash: The hash for the source commit. @param dst_commit_hash: The hash for the destination commit. + @param multi_branch: The two commits are on different branches so view the changes on the + branch containing and up to dst_commit, starting at a common ancestor of both. @param output_path: The path to the file to write the diff to. """ @@ -39,8 +41,15 @@ class Repo: except EnvironmentError as e: raise RuntimeError(f"Could not create path for output file '{output_path}'") + args = ["git", "diff", "--name-status", f"--output={output_path}"] + if multi_branch: + args.append(f"{src_commit_hash}...{dst_commit_hash}") + else: + args.append(src_commit_hash) + args.append(dst_commit_hash) + # git diff will only write to the output file if both commit hashes are valid - subprocess.run(["git", "diff", "--name-status", f"--output={output_path}", src_commit_hash, dst_commit_hash]) + subprocess.run(args) if not output_path.is_file(): raise RuntimeError(f"Source commit '{src_commit_hash}' and/or destination commit '{dst_commit_hash}' are invalid") diff --git a/scripts/build/TestImpactAnalysis/mars_utils.py b/scripts/build/TestImpactAnalysis/mars_utils.py index 69374512a3..e2394ed6e1 100644 --- a/scripts/build/TestImpactAnalysis/mars_utils.py +++ b/scripts/build/TestImpactAnalysis/mars_utils.py @@ -15,7 +15,7 @@ logger = get_logger(__file__) MARS_JOB_KEY = "job" SRC_COMMIT_KEY = "src_commit" -DST_COMMIT_KEY = "src_commit" +DST_COMMIT_KEY = "dst_commit" COMMIT_DISTANCE_KEY = "commit_distance" SRC_BRANCH_KEY = "src_branch" DST_BRANCH_KEY = "dst_branch" @@ -318,7 +318,7 @@ def generate_mars_sequence(sequence_report: dict, mars_job: dict, change_list:di test_run_selection = {} test_run_selection[SELECTED_KEY] = generate_mars_test_run_selections(sequence_report[SELECTED_TEST_RUNS_KEY], sequence_report[SELECTED_TEST_RUN_REPORT_KEY], t0_timestamp) if sequence_report[SEQUENCE_TYPE_KEY] == IMPACT_ANALYSIS_SEQUENCE_TYPE_KEY or sequence_report[SEQUENCE_TYPE_KEY] == SAFE_IMPACT_ANALYSIS_SEQUENCE_TYPE_KEY: - total_test_runs = sequence_report[TOTAL_NUM_TEST_RUNS_KEY] + total_test_runs = sequence_report[TOTAL_NUM_TEST_RUNS_KEY] + len(sequence_report[DISCARDED_TEST_RUNS_KEY]) if total_test_runs > 0: test_run_selection[SELECTED_KEY][EFFICIENCY_KEY] = (1.0 - (test_run_selection[SELECTED_KEY][TOTAL_NUM_TEST_RUNS_KEY] / total_test_runs)) * 100 else: diff --git a/scripts/build/TestImpactAnalysis/tiaf.py b/scripts/build/TestImpactAnalysis/tiaf.py index 3c94c2b5f4..255a34759a 100644 --- a/scripts/build/TestImpactAnalysis/tiaf.py +++ b/scripts/build/TestImpactAnalysis/tiaf.py @@ -50,7 +50,7 @@ class TestImpact: logger.warning(f"Could not find TIAF binary at location {self._tiaf_bin}, TIAF will be turned off.") self._use_test_impact_analysis = False else: - logger.info(f"Runtime binary found at location {self._tiaf_bin}") + logger.info(f"Runtime binary found at location '{self._tiaf_bin}'") # Workspaces self._active_workspace = self._config["workspace"]["active"]["root"] @@ -71,21 +71,33 @@ class TestImpact: self._has_change_list = False self._change_list_path = None + self._src_commit = last_commit_hash # Check whether or not a previous commit hash exists (no hash is not a failure) - self._src_commit = last_commit_hash if self._src_commit: - if self._repo.is_descendent(self._src_commit, self._dst_commit) == False: - logger.info(f"Source commit '{self._src_commit}' and destination commit '{self._dst_commit}' are not related.") - return - self._commit_distance = self._repo.commit_distance(self._src_commit, self._dst_commit) - diff_path = pathlib.Path(pathlib.PurePath(self._temp_workspace).joinpath(f"changelist.{instance_id}.diff")) + if self._is_source_of_truth_branch: + # For branch builds, the dst commit must be descended from the src commit + if not self._repo.is_descendent(self._src_commit, self._dst_commit): + logger.error(f"Source commit '{self._src_commit}' and destination commit '{self._dst_commit}' must be related for branch builds.") + return + + # Calculate the distance (in commits) between the src and dst commits + self._commit_distance = self._repo.commit_distance(self._src_commit, self._dst_commit) + logger.info(f"The distance between '{self._src_commit}' and '{self._dst_commit}' commits is '{self._commit_distance}' commits.") + multi_branch = False + else: + # For pull request builds, the src and dst commits are on different branches so we need to ensure a common ancestor is used for the diff + multi_branch = True + try: - self._repo.create_diff_file(self._src_commit, self._dst_commit, diff_path) + # Attempt to generate a diff between the src and dst commits + logger.error(f"Source '{self._src_commit}' and destination '{self._dst_commit}' will be diff'd.") + diff_path = pathlib.Path(pathlib.PurePath(self._temp_workspace).joinpath(f"changelist.{instance_id}.diff")) + self._repo.create_diff_file(self._src_commit, self._dst_commit, diff_path, multi_branch) except RuntimeError as e: logger.error(e) return - + # A diff was generated, attempt to parse the diff and construct the change list logger.info(f"Generated diff between commits '{self._src_commit}' and '{self._dst_commit}': '{diff_path}'.") with open(diff_path, "r") as diff_data: @@ -189,7 +201,7 @@ class TestImpact: self._is_source_of_truth_branch = True self._source_of_truth_branch = self._src_branch else: - # PR builds use their destination as the source of truth and never update the coverage data for the source of truth + # Pull request builds use their destination as the source of truth and never update the coverage data for the source of truth self._is_source_of_truth_branch = False self._source_of_truth_branch = self._dst_branch diff --git a/scripts/build/TestImpactAnalysis/tiaf_persistent_storage_s3.py b/scripts/build/TestImpactAnalysis/tiaf_persistent_storage_s3.py index 09b4df0564..24101c7cba 100644 --- a/scripts/build/TestImpactAnalysis/tiaf_persistent_storage_s3.py +++ b/scripts/build/TestImpactAnalysis/tiaf_persistent_storage_s3.py @@ -48,6 +48,12 @@ class PersistentStorageS3(PersistentStorage): for object in self._bucket.objects.filter(Prefix=self._historic_data_key): logger.info(f"Historic data found for branch '{branch}'.") + # Archive the existing object with the name of the existing last commit hash + #archive_key = f"{self._dir}/archive/{self._last_commit_hash}.{object_extension}" + #logger.info(f"Archiving existing historic data to '{archive_key}' in bucket '{self._bucket.name}'...") + #self._bucket.copy({"Bucket": self._bucket.name, "Key": self._historic_data_key}, archive_key) + #logger.info(f"Archiving complete.") + # Decode the historic data object into raw bytes logger.info(f"Attempting to decode historic data object...") response = object.get()