From 65e9e895912e2ef54a5bbda10df983fef500b793 Mon Sep 17 00:00:00 2001 From: Brendan Jackman Date: Sat, 20 Sep 2025 19:03:00 +0200 Subject: [PATCH] feat: Add by_commit_with_notes caching option --- README.md | 44 ++++++ limmat.schema.json | 4 +- src/config.rs | 1 + src/git.rs | 68 +++++++++- src/test.rs | 273 +++++++++++++++++++++++++++++++++++++- src/ui.rs | 1 + tests/integration_test.rs | 68 ++++++++++ 7 files changed, 453 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 85d1336..f4dbc1c 100644 --- a/README.md +++ b/README.md @@ -151,6 +151,42 @@ That means Limmat won't re-run tests unless the actual repository contents change - for example changes to the commit message won't invalidate cache results. +For fine-grained cache control, you can use `cache = "by_commit_with_notes"`. +This creates cache keys based on both the commit hash and git notes attached +to the commit under the `refs/notes/limmat` ref. This allows you to +invalidate the cache for specific commits by adding or modifying notes: + +```bash +# This will invalidate the cache for this commit +git notes --ref=limmat add -m "force rebuild" abc1234 + +# Different note content = different cache key +git notes --ref=limmat add -m "test with flag X" abc1234 --force +``` + +Your test scripts can access the exact notes content that was used for the cache key: + +```bash +#!/bin/bash +# Example test script using git notes for configuration + +if [[ -n "$LIMMAT_NOTES_OBJECT" ]]; then + # Get the exact notes content used for cache key generation + notes_content=$(git cat-file -p "$LIMMAT_NOTES_OBJECT") + echo "Test configuration from notes: $notes_content" + + # Parse test parameters from notes + if echo "$notes_content" | grep -q "rebuild"; then + echo "Force rebuild requested" + make clean + fi +else + echo "No notes attached to this commit, using defaults" +fi + +make test +``` + If the test is terminated by a signal, it isn't considered to have produced a result: instead of "success" or "failure" it's an "error". Errors aren't cached. @@ -323,6 +359,7 @@ These environment variables are passed to your job. | `LIMMAT_ORIGIN` | Path of the main repository worktree (i.e. `--repo`). | | `LIMMAT_COMMIT` | Hash of the commit to be tested. | | `LIMMAT_CONFIG` | Path of the config file. | +| `LIMMAT_NOTES_OBJECT` | Git object hash of the notes content used for cache key generation (when using `cache = "by_commit_with_notes"`). Empty if no notes exist. | | `LIMMAT_RESOURCE__` | Values for [resources](#resources) used by the test. | | `LIMMAT_RESOURCE_` | If the test only uses one of a resource, shorthand for `LIMMAT_RESOURCE__0` | | `LIMMAT_ARTIFACTS_` | If the test depends on `job_name`, this directory contains that job's [artifacts](#artifacts). | @@ -376,6 +413,13 @@ make -j defconfig make -j16 vmlinux O=$LIMMAT_ARTIFACTS """ +# Run stress tests that can be configured via git notes +[[tests]] +name = "stress_test" +cache = "by_commit_with_notes" +command = "stress_test.sh" +depends_on = ["kbuild"] + # Check the locally-built kernel boots in a QEMU VM. [[tests]] name = "boot_qemu" diff --git a/limmat.schema.json b/limmat.schema.json index 3bd8d33..d8b3616 100644 --- a/limmat.schema.json +++ b/limmat.schema.json @@ -32,7 +32,8 @@ "enum": [ "no_caching", "by_commit", - "by_tree" + "by_tree", + "by_commit_with_notes" ] }, "Command": { @@ -103,6 +104,7 @@ ], "properties": { "cache": { + "description": "Controls when test results are cached. \"by_commit\" (default) caches by commit hash. \"by_tree\" caches by tree hash (ignores commit message changes). \"by_commit_with_notes\" caches by commit hash plus git notes under refs/notes/limmat. \"no_caching\" disables caching.", "default": "by_commit", "allOf": [ { diff --git a/src/config.rs b/src/config.rs index 52ac959..bd65c46 100644 --- a/src/config.rs +++ b/src/config.rs @@ -94,6 +94,7 @@ pub struct Test { /// willing to wait when you terminate this program. shutdown_grace_period_s: u64, #[serde(default = "default_cache_policy")] + /// Controls when test results are cached. "by_commit" (default) caches by commit hash. "by_tree" caches by tree hash (ignores commit message changes). "by_commit_with_notes" caches by commit hash plus git notes under refs/notes/limmat. "no_caching" disables caching. cache: CachePolicy, #[serde(default)] depends_on: Vec, diff --git a/src/git.rs b/src/git.rs index b30b27e..b26fa27 100644 --- a/src/git.rs +++ b/src/git.rs @@ -172,6 +172,7 @@ impl Worktree for PersistentWorktree { pub struct Commit { pub hash: CommitHash, pub tree: TreeHash, + pub limmat_notes_object: Option, } impl Commit { @@ -180,6 +181,7 @@ impl Commit { Self { hash: CommitHash::new("080b8ecbad3e34e55c5a035af80100f73b742a8d"), tree: TreeHash::new("6366d790125291272542a6b40f6fd3400e080821"), + limmat_notes_object: None, } } } @@ -457,6 +459,65 @@ pub trait Worktree: Debug + Sync { }) } + async fn get_notes_object_hash( + &self, + commit_hash: &CommitHash, + ) -> anyhow::Result> { + debug!( + "Looking for git notes for commit {}", + commit_hash.as_ref() as &str + ); + let output = self + .git(["notes", "--ref=limmat", "list"]) + .await + .arg(commit_hash.as_ref() as &str) + .output() + .await + .context("failed to run 'git notes list'")?; + + debug!("git notes list exit code: {:?}", output.status.code()); + debug!( + "git notes list stdout: {:?}", + String::from_utf8_lossy(&output.stdout) + ); + debug!( + "git notes list stderr: {:?}", + String::from_utf8_lossy(&output.stderr) + ); + + // Exit code 1 means no notes found, which is fine + if output.status.code() == Some(1) { + debug!("No notes found for commit {}", commit_hash.as_ref() as &str); + return Ok(None); + } + + if !output.status.success() { + // Other error - treat as no notes to be safe + debug!( + "git notes list failed with code {:?}, treating as no notes", + output.status.code() + ); + return Ok(None); + } + + let notes_output = String::from_utf8_lossy(&output.stdout); + let notes_object_hash = notes_output.trim(); + if notes_object_hash.is_empty() { + debug!( + "Empty notes output for commit {}", + commit_hash.as_ref() as &str + ); + return Ok(None); + } + + debug!( + "Found notes object hash {} for commit {}", + notes_object_hash, + commit_hash.as_ref() as &str + ); + Ok(Some(Hash::new(notes_object_hash.to_string()))) + } + // None means we successfully looked it up but it didn't exist. async fn rev_parse(&self, rev_spec: S) -> anyhow::Result> where @@ -486,9 +547,14 @@ pub trait Worktree: Debug + Sync { String::from_utf8_lossy(&output.stderr) ); } + + let commit_hash = CommitHash::new(parts[0]); + let limmat_notes_object = self.get_notes_object_hash(&commit_hash).await?; + Ok(Some(Commit { - hash: CommitHash::new(parts[0]), + hash: commit_hash, tree: TreeHash::new(parts[1]), + limmat_notes_object, })) } } diff --git a/src/test.rs b/src/test.rs index 4757638..db8ebb1 100644 --- a/src/test.rs +++ b/src/test.rs @@ -44,6 +44,7 @@ pub enum CachePolicy { NoCaching, ByCommit, ByTree, + ByCommitWithNotes, } impl CachePolicy { @@ -54,6 +55,14 @@ impl CachePolicy { CachePolicy::NoCaching => None::, CachePolicy::ByCommit => Some(commit.hash.clone().into()), CachePolicy::ByTree => Some(commit.tree.clone().into()), + CachePolicy::ByCommitWithNotes => { + let notes_part = commit + .limmat_notes_object + .as_ref() + .map(|h| h.as_ref()) + .unwrap_or("no-notes"); + Some(Hash::new(format!("{}:{}", commit.hash, notes_part))) + } } } } @@ -741,6 +750,9 @@ impl<'a> TestJob { dep_db_entries: &DepDatabaseEntries, ) { cmd.env("LIMMAT_COMMIT", &self.test_case.commit_hash); + if let Some(notes_object) = &self.test_case.notes_object_hash { + cmd.env("LIMMAT_NOTES_OBJECT", notes_object.as_ref() as &str); + } cmd.env("LIMMAT_ARTIFACTS", artifacts_dir); for (k, v) in self.base_env.iter() { cmd.env(k, v); @@ -865,8 +877,19 @@ impl<'a> TestJob { pub struct TestCaseId(String); impl TestCaseId { - fn new(commit_hash: &CommitHash, test_name: &TestName) -> Self { - Self(format!("{}:{}", commit_hash, test_name)) + fn new( + commit_hash: &CommitHash, + test_name: &TestName, + notes_object_hash: &Option, + ) -> Self { + Self(format!( + "{}:{}:{}", + commit_hash, + test_name, + notes_object_hash + .as_ref() + .map_or("".to_string(), |h| h.to_string()), + )) } } @@ -878,6 +901,8 @@ pub struct TestCase { // Hash that will be used to identify the test result, if caching is // enabled. Might be a tree hash, otherwise it matches the commit hash. pub cache_hash: Option, + // Git object hash of notes content (for LIMMAT_NOTES_OBJECT env var) + pub notes_object_hash: Option, pub test: Arc, } @@ -896,6 +921,7 @@ impl TestCase { pub fn new(commit: Commit, test: Arc) -> Self { Self { cache_hash: test.cache_policy.cache_hash(&commit), + notes_object_hash: commit.limmat_notes_object.clone(), test, commit_hash: commit.hash, } @@ -912,7 +938,7 @@ impl TestCase { // doesn't really need to be. fn id(&self) -> TestCaseId { // The hash_cache is redundant information here so we don't need to include it. - TestCaseId::new(&self.commit_hash, &self.test.name) + TestCaseId::new(&self.commit_hash, &self.test.name, &self.notes_object_hash) } } @@ -927,7 +953,7 @@ impl GraphNode for TestCase { self.test .depends_on .iter() - .map(|test_name| TestCaseId::new(&self.commit_hash, test_name)) + .map(|test_name| TestCaseId::new(&self.commit_hash, test_name, &self.notes_object_hash)) .collect() } } @@ -1843,6 +1869,66 @@ mod tests { assert_eq!(f.scripts[2].num_runs(&orig_commit.hash), 1); } + #[tokio::test] + async fn should_cache_by_commit_with_notes() { + let f = TestScriptFixture::builder() + .cache_policies([CachePolicy::ByCommitWithNotes]) + .build() + .await; + + // Create a commit + let commit = f + .repo + .commit("test commit for notes caching") + .await + .expect("couldn't create test commit"); + + // Run test on the commit - should run the first time + f.manager.set_revisions(vec![commit.clone()]).await.unwrap(); + f.manager.settled().await; + assert_eq!(f.scripts[0].num_runs(&commit.hash), 1); + + // Run again on same commit - should be cached (no notes = consistent cache key) + f.manager.set_revisions(vec![commit.clone()]).await.unwrap(); + f.manager.settled().await; + assert_eq!(f.scripts[0].num_runs(&commit.hash), 1); // Still 1, was cached + + // Add a note to the commit using git CLI + let notes_content = "test note content"; + tokio::process::Command::new("git") + .current_dir(f.repo.path()) + .args(["notes", "--ref=limmat", "add", "-m", notes_content]) + .arg(&commit.hash) + .status() + .await + .expect("Failed to execute git command"); + + // Create a new commit object by re-parsing the same commit hash + // This should pick up the newly added notes + let commit_with_notes = f + .repo + .rev_parse(&commit.hash) + .await + .expect("Failed to re-parse commit") + .expect("Commit should exist"); + + // Run again - should re-run because notes changed the cache key + f.manager + .set_revisions(vec![commit_with_notes.clone()]) + .await + .unwrap(); + f.manager.settled().await; + assert_eq!(f.scripts[0].num_runs(&commit.hash), 2); // Should be 2 now + + // Run again with same note - should be cached again + f.manager + .set_revisions(vec![commit_with_notes.clone()]) + .await + .unwrap(); + f.manager.settled().await; + assert_eq!(f.scripts[0].num_runs(&commit.hash), 2); // Still 2, was cached + } + #[test_case(1, 1 ; "single worktree, one test")] #[test_case(4, 1 ; "multiple worktrees, one test")] #[test_case(4, 4 ; "multiple worktrees, multiple tests")] @@ -2099,6 +2185,185 @@ mod tests { assert_eq!(env.get("LIMMAT_CONFIG"), Some("/fake/config/path").as_ref()); } + #[tokio::test] + async fn test_limmat_notes_object_env() { + let temp_dir = TempDir::new().unwrap(); + let repo = Arc::new(TempRepo::new().await.unwrap()); + + // Create a commit + let commit = repo + .commit("test commit") + .await + .expect("couldn't create test commit"); + + // Add a git note to the commit + let notes_content = "test note for env var"; + tokio::process::Command::new("git") + .current_dir(repo.path()) + .args(["notes", "--ref=limmat", "add", "-m", notes_content]) + .arg(&commit.hash) + .status() + .await + .expect("Failed to add git note"); + + // Get the notes object hash that should be set + let notes_output = tokio::process::Command::new("git") + .current_dir(repo.path()) + .args(["notes", "--ref=limmat", "list"]) + .arg(&commit.hash) + .output() + .await + .expect("Failed to get notes list"); + let expected_notes_object = String::from_utf8_lossy(¬es_output.stdout) + .trim() + .to_string(); + + let db_dir = TempDir::new().expect("couldn't make temp dir for result DB"); + + // Set up test that dumps environment + let tests = Dag::new([Arc::new( + TestBuilder::new( + "env_test", + "bash", + [ + "-c".into(), + OsString::from(format!( + "env >> {0:?}/${{LIMMAT_COMMIT}}_env.txt", + temp_dir.path() + )), + ], + ) + .cache_policy(CachePolicy::ByCommitWithNotes) + .needs_resources([(ResourceKey::Worktree, 1)]) + .build(), + )]) + .expect("couldn't build test DAG"); + + let resource_pools = + Pools::new([(ResourceKey::Worktree, worktree_resources(&repo, 1).await)]); + let m = Manager::new( + repo.clone(), + "/fake/config/path", + Arc::new(Database::create_or_open(db_dir.path()).expect("couldn't setup result DB")), + Arc::new(resource_pools), + tests, + ); + + m.set_revisions([commit.clone()]) + .await + .expect("set_revisions failed"); + m.settled().await; + + // Read and parse the environment dump + let env_path = temp_dir.path().join(format!("{}_env.txt", commit.hash)); + let env_dump = fs::read_to_string(&env_path).unwrap_or_else(|_| { + panic!( + "couldn't read env dumped from test script at {}", + env_path.display() + ) + }); + let env: HashMap<&str, &str> = env_dump + .trim() + .split("\n") + .filter_map(|line| { + let parts: Vec<_> = line.splitn(2, "=").collect(); + if parts.len() != 2 { + return None; + } + Some((parts[0], parts[1])) + }) + .collect(); + + // Verify LIMMAT_NOTES_OBJECT is set correctly + assert_eq!( + env.get("LIMMAT_NOTES_OBJECT"), + Some(expected_notes_object.as_str()).as_ref() + ); + + // Also verify LIMMAT_COMMIT is still set + assert_eq!( + env.get("LIMMAT_COMMIT").map(|t| CommitHash::new(*t)), + Some(&commit.hash).cloned() + ); + } + + #[tokio::test] + async fn test_limmat_notes_object_env_no_notes() { + let temp_dir = TempDir::new().unwrap(); + let repo = Arc::new(TempRepo::new().await.unwrap()); + + // Create a commit WITHOUT notes + let commit = repo + .commit("test commit without notes") + .await + .expect("couldn't create test commit"); + + let db_dir = TempDir::new().expect("couldn't make temp dir for result DB"); + + // Set up test that dumps environment + let tests = Dag::new([Arc::new( + TestBuilder::new( + "env_test", + "bash", + [ + "-c".into(), + OsString::from(format!( + "env >> {0:?}/${{LIMMAT_COMMIT}}_env.txt", + temp_dir.path() + )), + ], + ) + .cache_policy(CachePolicy::ByCommitWithNotes) + .needs_resources([(ResourceKey::Worktree, 1)]) + .build(), + )]) + .expect("couldn't build test DAG"); + + let resource_pools = + Pools::new([(ResourceKey::Worktree, worktree_resources(&repo, 1).await)]); + let m = Manager::new( + repo.clone(), + "/fake/config/path", + Arc::new(Database::create_or_open(db_dir.path()).expect("couldn't setup result DB")), + Arc::new(resource_pools), + tests, + ); + + m.set_revisions([commit.clone()]) + .await + .expect("set_revisions failed"); + m.settled().await; + + // Read and parse the environment dump + let env_path = temp_dir.path().join(format!("{}_env.txt", commit.hash)); + let env_dump = fs::read_to_string(&env_path).unwrap_or_else(|_| { + panic!( + "couldn't read env dumped from test script at {}", + env_path.display() + ) + }); + let env: HashMap<&str, &str> = env_dump + .trim() + .split("\n") + .filter_map(|line| { + let parts: Vec<_> = line.splitn(2, "=").collect(); + if parts.len() != 2 { + return None; + } + Some((parts[0], parts[1])) + }) + .collect(); + + // Verify LIMMAT_NOTES_OBJECT is NOT set when there are no notes + assert_eq!(env.get("LIMMAT_NOTES_OBJECT"), None); + + // Verify LIMMAT_COMMIT is still set + assert_eq!( + env.get("LIMMAT_COMMIT").map(|t| CommitHash::new(*t)), + Some(&commit.hash).cloned() + ); + } + #[tokio::test] async fn should_not_start_canceled() { let f = TestScriptFixture::builder() diff --git a/src/ui.rs b/src/ui.rs index 6153764..a831e66 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -488,6 +488,7 @@ mod tests { test_case: TestCase { commit_hash: commit_hash.clone(), cache_hash: Some(commit_hash.clone().into()), + notes_object_hash: None, // This is just a fake test case test: test.clone(), }, status, diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 6a43321..277557f 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -971,3 +971,71 @@ async fn skip_test() { // will return an inner error). assert_that!(result, err(anything())); } + +#[tokio::test] +async fn should_provide_notes_object_env_var() { + let temp_dir = TempDir::new().unwrap(); + let notes_output_file = temp_dir.path().join("notes_content.txt"); + + let config = format!( + r##" + num_worktrees = 1 + [[tests]] + name = "notes_test" + cache = "by_commit_with_notes" + command = ["bash", "-c", "if [[ -n \"$LIMMAT_NOTES_OBJECT\" ]]; then git cat-file -p \"$LIMMAT_NOTES_OBJECT\" > {}; else echo 'NO_NOTES' > {}; fi"] + shutdown_grace_period_s = 2 + "##, + notes_output_file.display(), + notes_output_file.display(), + ); + + let builder = LimmatChildBuilder::new(&config).await.unwrap(); + + // Create a new commit first + let git_status = Command::new("git") + .current_dir(&builder.repo_dir) + .args(["commit", "--allow-empty", "-m", "test commit for notes"]) + .status() + .await + .expect("Failed to create test commit"); + + assert!(git_status.success(), "Failed to create test commit"); + + // Add a git note to the new HEAD commit + let notes_content = "integration test note content"; + let git_status = Command::new("git") + .current_dir(&builder.repo_dir) + .args(["notes", "--ref=limmat", "add", "-m", notes_content, "HEAD"]) + .status() + .await + .expect("Failed to execute git notes command"); + + assert!(git_status.success(), "Failed to add git note"); + + let mut limmat = builder.start(["watch", "HEAD^"]).await.unwrap(); + + // Wait for the test to complete + timeout( + Duration::from_secs(10), + limmat.result_exists("notes_test", "HEAD"), + ) + .await + .expect("Test result not found within timeout") + .expect("Failed to check for test result"); + + limmat + .terminate() + .await + .expect("Failed to terminate limmat"); + + // Verify the notes content was correctly accessed via LIMMAT_NOTES_OBJECT + let output_content = + fs::read_to_string(¬es_output_file).expect("Failed to read notes output file"); + + assert_eq!( + output_content.trim(), + notes_content, + "Expected notes content to be accessible via LIMMAT_NOTES_OBJECT" + ); +}