From debbda8fc6af290feead0eea4f49ad4f91cd389a Mon Sep 17 00:00:00 2001 From: cfranklin <579516+chris-franklin@users.noreply.github.com> Date: Tue, 21 Jul 2026 10:44:44 +0100 Subject: [PATCH 1/2] Always move test outs even if there are test args or failures In the test step there is a `moveAndCacheOutputFiles` local function that's called if all test cases succeed. It exits early if any arguments were passed to the test, with the stated reason being that results shouldn't be cached if args were passed. The problem is that while it does prevent caching, it also prevents the files being copied to the output dir. This is unhelpful because those output files are not invalidated by the passing of args, indeed sometimes arguments are passed precisely //so// they are produced. Those files can also be helpful in diagnosing failures. This diff refactors the helpers to separate moving output files from the caching: files are always moved even when there are failures, but the original checks are still there and do prevent those files being cached. --- src/test/test_step.go | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/test/test_step.go b/src/test/test_step.go index 5da79676b..9c28e3804 100644 --- a/src/test/test_step.go +++ b/src/test/test_step.go @@ -94,27 +94,17 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge return &results } - moveAndCacheOutputFiles := func(results *core.TestSuite, coverage *core.TestCoverage) bool { - // Never cache test results when given arguments; the results may be incomplete. - if len(state.TestArgs) > 0 { - log.Debug("Not caching results for %s, we passed it arguments", label) - return true - } - // Never cache test results if there were failures (usually flaky tests). - if results.Failures() > 0 { - log.Debug("Not caching results for %s, test had failures", label) - return true - } + moveOutputFiles := func(results *core.TestSuite, coverage *core.TestCoverage) []string { outs := []string{filepath.Base(target.TestResultsFile())} if err := moveOutputFile(state, hash, outputFile, target.TestResultsFile(), dummyOutput); err != nil { state.LogTestResult(target, run, core.TargetTestFailed, results, coverage, err, "Failed to move test output file") - return false + return nil } if needCoverage || core.PathExists(coverageFile) { if err := moveOutputFile(state, hash, coverageFile, target.CoverageFile(), dummyCoverage); err != nil { state.LogTestResult(target, run, core.TargetTestFailed, results, coverage, err, "Failed to move test coverage file") - return false + return nil } outs = append(outs, filepath.Base(target.CoverageFile())) } @@ -123,10 +113,24 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge outFile := filepath.Join(target.OutDir(), output) if err := moveOutputFile(state, hash, tmpFile, outFile, ""); err != nil { state.LogTestResult(target, run, core.TargetTestFailed, results, coverage, err, "Failed to move test output file") - return false + return nil } outs = append(outs, output) } + return outs + } + + cacheOutputFiles := func(results *core.TestSuite, outs []string) bool { + // Never cache test results when given arguments; the results may be incomplete. + if len(state.TestArgs) > 0 { + log.Debug("Not caching results for %s, we passed it arguments", label) + return false + } + // Never cache test results if there were failures (usually flaky tests). + if results.Failures() > 0 { + log.Debug("Not caching results for %s, test had failures", label) + return false + } if state.Cache != nil && !runRemotely { state.Cache.Store(target, hash, outs) } @@ -189,9 +193,11 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge results, coverage = doFlakeRun(state, target, run, runRemotely) target.AddTestResults(results) + outs := moveOutputFiles(target.Test.Results, coverage) + if target.Test.Results.TestCases.AllSucceeded() { // Success, store in cache - moveAndCacheOutputFiles(target.Test.Results, coverage) + cacheOutputFiles(target.Test.Results, outs) } } else if state.TestSequentially { for run := 1; run <= int(state.NumTestRuns); run++ { From 506a420078d4ff793f0f5d05adf0e996668b6e20 Mon Sep 17 00:00:00 2001 From: cfranklin <579516+chris-franklin@users.noreply.github.com> Date: Tue, 21 Jul 2026 13:27:34 +0100 Subject: [PATCH 2/2] Add e2e tests and treat movement of the test results file as caching --- src/test/test_step.go | 20 ++++++++++++-------- test/BUILD | 30 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/src/test/test_step.go b/src/test/test_step.go index 9c28e3804..e1e24ed3f 100644 --- a/src/test/test_step.go +++ b/src/test/test_step.go @@ -94,12 +94,9 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge return &results } + // This moves all files OTHER THAN the result file to the plz-out/bin dir. moveOutputFiles := func(results *core.TestSuite, coverage *core.TestCoverage) []string { - outs := []string{filepath.Base(target.TestResultsFile())} - if err := moveOutputFile(state, hash, outputFile, target.TestResultsFile(), dummyOutput); err != nil { - state.LogTestResult(target, run, core.TargetTestFailed, results, coverage, err, "Failed to move test output file") - return nil - } + outs := []string{} if needCoverage || core.PathExists(coverageFile) { if err := moveOutputFile(state, hash, coverageFile, target.CoverageFile(), dummyCoverage); err != nil { @@ -120,7 +117,7 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge return outs } - cacheOutputFiles := func(results *core.TestSuite, outs []string) bool { + cacheOutputFiles := func(results *core.TestSuite, coverage *core.TestCoverage, outs []string) bool { // Never cache test results when given arguments; the results may be incomplete. if len(state.TestArgs) > 0 { log.Debug("Not caching results for %s, we passed it arguments", label) @@ -131,8 +128,15 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge log.Debug("Not caching results for %s, test had failures", label) return false } + + // The test result file is moved here rather than in moveOutputFiles because it is + // effectively a cache of the test result. + if err := moveOutputFile(state, hash, outputFile, target.TestResultsFile(), dummyOutput); err != nil { + state.LogTestResult(target, run, core.TargetTestFailed, results, coverage, err, "Failed to move test output file") + return false + } if state.Cache != nil && !runRemotely { - state.Cache.Store(target, hash, outs) + state.Cache.Store(target, hash, append(outs, filepath.Base(target.TestResultsFile()))) } return true } @@ -197,7 +201,7 @@ func test(state *core.BuildState, label core.BuildLabel, target *core.BuildTarge if target.Test.Results.TestCases.AllSucceeded() { // Success, store in cache - cacheOutputFiles(target.Test.Results, outs) + cacheOutputFiles(target.Test.Results, coverage, outs) } } else if state.TestSequentially { for run := 1; run <= int(state.NumTestRuns); run++ { diff --git a/test/BUILD b/test/BUILD index b66da012b..9993cb692 100644 --- a/test/BUILD +++ b/test/BUILD @@ -62,6 +62,22 @@ gentest( test_cmd = "cp $(location test_output_test_2.xml) test.results", ) +# Test that when test arguments are supplied, other output files are still moved to bin/. +plz_e2e_test( + name = "test_outputs_move_test", + cmd = "plz test //test:test_outputs_move_test_case -- some_test_arg", + expect_output_contains = "4 passed.", + expect_file_exists = "plz-out/bin/test/foo.txt", +) + +gentest( + name = "test_outputs_move_test_case", + data = ["test_output_test_1.txt"], + labels = ["manual"], + test_outputs = ["foo.txt"], + test_cmd = "cp $(location test_output_test_1.txt) test.results && echo 'hello' > foo.txt #", +) + # Test that on re-running a test it is cached. plz_e2e_test( name = "test_caching_test", @@ -69,6 +85,20 @@ plz_e2e_test( expect_output_contains = "Not re-running test //test:caching_test", ) +# Test that on running a test with args, it is NOT cached. +plz_e2e_test( + name = "test_nocache_with_args_test", + cmd = "plz test --rerun //test:nocache_test -- some_arg && plz test -v 4 //test:nocache_test", + expect_output_doesnt_contain = "Not re-running test //test:nocache_test", +) + +gentest( + name = "nocache_test", + labels = ["manual"], + no_test_output = True, + test_cmd = "true", +) + gentest( name = "caching_test", labels = ["manual"],