Simplify git logic in regression tests

.github/workflows/regression_ci.yml

@@ -55,6 +55,7 @@ jobs:
       - name: Run scalar performance test (mainline)
         env:
           PERF_MODE: valgrind
+          COMMIT_TYPE: baseline
         run: cargo test --release --manifest-path=tests/regression/Cargo.toml
 
       # Checkout pull request branch
@@ -69,6 +70,7 @@ jobs:
       - name: Run scalar performance test (PR branch)
         env:
           PERF_MODE: valgrind
+          COMMIT_TYPE: altered
         run: cargo test --release --manifest-path=tests/regression/Cargo.toml
 
       # Run the differential performance test

tests/regression/README.md

@@ -39,17 +39,17 @@ This will recursively call all tests with valgrind enabled so the performance ou
 ## Running the Harnesses between versions (differential performance)
 Run the scalar performance for all harnesses on the current branch version of s2n-tls
 ```
-PERF_MODE=valgrind cargo test
+PERF_MODE=valgrind COMMIT_TYPE=altered cargo test
 ```
 `git checkout` or `git switch` to mainline/version being compared to. Make sure you have stashed or committed any changes.
 ```
-PERF_MODE=valgrind cargo test
+PERF_MODE=valgrind COMMIT_TYPE=baseline cargo test
 ```
 `git checkout` or `git switch` back to the original version. At this point you should have two annotated performance outputs for each test. If you have more, the diff test will not be able to recognize the versions being compared.
 ```
 PERF_MODE=diff cargo test
 ```
-This will assert on the performance difference of the current version minus the previous. If the regression exceeds the const `MAX_DIFF`, the test fails. Performance output profiles are stored by their commit id in `/target/commit_id`:
+This will assert on the performance difference of the current version minus the previous. If the regression exceeds the const `MAX_DIFF`, the test fails. Performance output profiles are stored by their commit id in `/target/baseline` or `target/altered`:
 - `raw_profile` for the unannotated cachegrind output result
 - `annotated_profile` for the annotated cachegrind output (scalar)
 - `target/diff` contains the annotated differential profile between two commits
@@ -63,9 +63,9 @@ cargo test
 This will run the tests without valgrind to test if the harnesses complete as expected
 
 ## Output Files
-- `target/$commit_id/test_name.raw`: Contains the raw cachegrind profile. On its own, the file is pretty much unreadable but is useful for the cg_annotate --diff functionality or to visualize the profile via tools like [KCachegrind](https://kcachegrind.github.io/html/Home.html).
-- `target/$commit_id/test_name.annotated`: The scalar annotated profile associated with that particular commit id. This file contains detailed information on the contribution of functions, files, and lines of code to the overall scalar performance count.
-- `target/diff/test_name.diff`: The annotated performance difference between two commits. This file contains the overall performance difference and also details the instruction counts, how many instructions a particular file/function account for, and the contribution of individual lines of code to the overall instruction count difference.
+- `target/regression_artifact/$commit_type/$commit_id_test_name.raw`: Contains the raw cachegrind profile. On its own, the file is pretty much unreadable but is useful for the cg_annotate --diff functionality or to visualize the profile via tools like [KCachegrind](https://kcachegrind.github.io/html/Home.html).
+- `target/regression_artifacts/$commit_type/$commit_id_test_name.annotated`: The scalar annotated profile associated with that particular commit id. This file contains detailed information on the contribution of functions, files, and lines of code to the overall scalar performance count.
+- `target/regression_artifacts/diff/test_name.diff`: The annotated performance difference between two commits. This file contains the overall performance difference and also details the instruction counts, how many instructions a particular file/function account for, and the contribution of individual lines of code to the overall instruction count difference.
 
 ## Sample Output for Valgrind test (differential)
 

tests/regression/src/lib.rs

@@ -17,46 +17,16 @@ pub mod git {
             .to_string()
     }
 
-    /// Returns true if `commit1` is older than `commit2`
-    pub fn is_older_commit(commit1: &str, commit2: &str) -> bool {
-        let status = Command::new("git")
-            .args(["merge-base", "--is-ancestor", commit1, commit2])
-            .status()
-            .expect("Failed to execute git merge-base");
-        status.success()
-    }
-
     pub fn extract_commit_hash(file: &str) -> String {
-        // input: "target/regression_artifacts/$commit_id/test_name.raw"
-        // output: "$commit_id"
+        // input: "target/regression_artifacts/{commit_type}/{commit_id_test_name.raw}"
+        // output: "{commit_id}"
+
         file.split("target/regression_artifacts/")
-            .nth(1)
-            .and_then(|s| s.split('/').next())
+            .nth(1) // Get the part after "target/regression_artifacts/"
+            .and_then(|s| s.split('/').nth(1)) // Get the part after "{commit_type}/"
+            .and_then(|s| s.split('_').next()) // Get the commit_id before the first underscore
             .map(|s| s.to_string())
-            .unwrap_or_default() // This will return an empty string if the Option is None
-    }
-
-    pub fn is_mainline(commit_hash: &str) -> bool {
-        // Execute the git command to check which branches contain the given commit.
-        let output = Command::new("git")
-            .args(["branch", "--contains", commit_hash])
-            .output()
-            .expect("Failed to execute git branch");
-
-        // If the command fails, it indicates that the commit is either detached
-        // or does not exist in any branches. Meaning, it is not part of mainline.
-        if !output.status.success() {
-            return false;
-        }
-
-        // Convert the command output to a string and check each line.
-        let branches = String::from_utf8_lossy(&output.stdout);
-        branches.lines().any(|branch| {
-            // Trim the branch name to remove any leading or trailing whitespace.
-            // The branch name could be prefixed with '*', indicating the current branch.
-            // We check for both "main" and "* main" to account for this possibility.
-            branch.trim() == "main" || branch.trim() == "* main"
-        })
+            .unwrap_or_default() // Return an empty string if the Option is None
     }
 }
 
@@ -137,16 +107,26 @@ mod tests {
     struct RawProfile {
         test_name: String,
         commit_hash: String,
+        commit_type: String,
     }
 
     impl RawProfile {
         fn new(test_name: &str) -> Self {
-            let commit_hash = git::get_current_commit_hash();
-            create_dir_all(format!("target/regression_artifacts/{commit_hash}")).unwrap();
+            // For filename readability, the first 7 digits of the commit hash are the typical standard to ensure a unique git SHA
+            let commit_hash = git::get_current_commit_hash()[..7].to_string();
+            let commit_type = env::var("COMMIT_TYPE")
+                .expect("COMMIT_TYPE environment variable must be set to 'baseline' or 'altered'");
+            assert!(
+                commit_type == "baseline" || commit_type == "altered",
+                "COMMIT_TYPE must be either 'baseline' or 'altered'"
+            );
+
+            create_dir_all(format!("target/regression_artifacts/{commit_type}")).unwrap();
 
             let raw_profile = Self {
                 test_name: test_name.to_owned(),
                 commit_hash,
+                commit_type,
             };
 
             let mut command = Command::new("valgrind");
@@ -169,8 +149,8 @@ mod tests {
 
         fn path(&self) -> String {
             format!(
-                "target/regression_artifacts/{}/{}.raw",
-                self.commit_hash, self.test_name
+                "target/regression_artifacts/{}/{}_{}.raw",
+                self.commit_type, self.commit_hash, self.test_name
             )
         }
 
@@ -184,54 +164,52 @@ mod tests {
         /// This method will panic if there are not two profiles.
         /// This method will also panic if both commits are on different logs (not mainline).
         fn query(test_name: &str) -> (RawProfile, RawProfile) {
-            let pattern = format!("target/regression_artifacts/**/*{}.raw", test_name);
-            let raw_files: Vec<String> = glob::glob(&pattern)
+            let baseline_pattern =
+                format!("target/regression_artifacts/baseline/*_{}.raw", test_name);
+            let altered_pattern =
+                format!("target/regression_artifacts/altered/*_{}.raw", test_name);
+
+            let baseline_file = glob::glob(&baseline_pattern)
                 .expect("Failed to read glob pattern")
                 .filter_map(Result::ok)
-                .map(|path| path.to_string_lossy().into_owned())
-                .collect();
-            assert_eq!(raw_files.len(), 2);
+                .next()
+                .expect("Baseline profile not found");
 
-            let profile1 = RawProfile {
+            let altered_file = glob::glob(&altered_pattern)
+                .expect("Failed to read glob pattern")
+                .filter_map(Result::ok)
+                .next()
+                .expect("Altered profile not found");
+
+            let baseline_profile = RawProfile {
                 test_name: test_name.to_string(),
-                commit_hash: git::extract_commit_hash(&raw_files[0]),
+                //file names stored an OsString could be invalid UTF-8 so to_string_lossy is used
+                commit_hash: git::extract_commit_hash(&baseline_file.to_string_lossy()),
+                commit_type: "baseline".to_string(),
             };
-            let profile2 = RawProfile {
+
+            let altered_profile = RawProfile {
                 test_name: test_name.to_string(),
-                commit_hash: git::extract_commit_hash(&raw_files[1]),
+                commit_hash: git::extract_commit_hash(&altered_file.to_string_lossy()),
+                commit_type: "altered".to_string(),
             };
 
-            // xor returns true if exactly one commit is mainline
-            if git::is_mainline(&profile1.commit_hash) ^ git::is_mainline(&profile2.commit_hash) {
-                // Return the mainline as first commit
-                if git::is_mainline(&profile1.commit_hash) {
-                    (profile1, profile2)
-                } else {
-                    (profile2, profile1)
-                }
-            } else {
-                // Neither or both profiles are on the mainline, so return the older one first
-                if git::is_older_commit(&profile1.commit_hash, &profile2.commit_hash) {
-                    (profile1, profile2)
-                } else if git::is_older_commit(&profile2.commit_hash, &profile1.commit_hash) {
-                    (profile2, profile1)
-                } else {
-                    panic!("The commits are not in the same log, are identical, or there are not two commits available");
-                }
-            }
+            (baseline_profile, altered_profile)
         }
     }
 
     struct AnnotatedProfile {
         test_name: String,
         commit_hash: String,
+        commit_type: String,
     }
 
     impl AnnotatedProfile {
         fn new(raw_profile: &RawProfile) -> Self {
             let annotated = Self {
                 test_name: raw_profile.test_name.clone(),
                 commit_hash: raw_profile.commit_hash.clone(),
+                commit_type: raw_profile.commit_type.clone(),
             };
 
             // annotate raw profile
@@ -251,8 +229,8 @@ mod tests {
 
         fn path(&self) -> String {
             format!(
-                "target/regression_artifacts/{}/{}.annotated",
-                self.commit_hash, self.test_name
+                "target/regression_artifacts/{}/{}_{}.annotated",
+                self.commit_type, self.commit_hash, self.test_name
             )
         }