From e77a5b586a85e00d0a652a04dd34905babcd8e73 Mon Sep 17 00:00:00 2001 From: cfranklin <579516+chris-franklin@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:38:26 +0100 Subject: [PATCH 1/5] Add support for test arguments to remote test commands This adds support for `BuildState.TestArgs` to remote builds. This includes support for `TestFields.ArgsPlaceholder` --- src/remote/action.go | 16 ++++++++++ src/remote/remote_test.go | 67 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/src/remote/action.go b/src/remote/action.go index 3b2f39782c..d61924fc03 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -160,6 +160,22 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg commandPrefix += `export TEST="$TEST_DIR/` + outs[0] + `" && ` } cmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state)) + if err == nil { + // If an args placeholder has been defined, then replace it with any provided test arguments... + if target.Test != nil && target.Test.ArgsPlaceholder != "" { + placeholder := target.Test.ArgsPlaceholder + if strings.Contains(cmd, placeholder) { + args := "" + if len(state.TestArgs) > 0 { + args = strings.Join(state.TestArgs, " ") + } + cmd = strings.ReplaceAll(cmd, placeholder, args) + } + } else if len(state.TestArgs) > 0 { + // ...otherwise, just append them to the command. + cmd += " " + strings.Join(state.TestArgs, " ") + } + } return &pb.Command{ Platform: &pb.Platform{ Properties: []*pb.Platform_Property{ diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index 05af2cd9ba..b6215d9747 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -353,3 +353,70 @@ func (c *Client) Store(target *core.BuildTarget) error { } return c.uploadLocalTarget(target) } + +func TestBuildTestCommand_WithPlaceholder(t *testing.T) { + c := newClientInstance("test") + state := c.state + state.TestArgs = []string{"--foo", "--bar"} + + target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target_placeholder"}) + target.AddOutput("remote_test") + target.Test = &core.TestFields{ + Timeout: time.Minute, + Command: "$TEST __TEST_ARGS__ 2>&1", + ArgsPlaceholder: "__TEST_ARGS__", + } + target.IsBinary = true + + cmd, err := c.buildTestCommand(state, target, 1) + assert.NoError(t, err) + + assert.True(t, + strings.HasSuffix(cmd.Arguments[len(cmd.Arguments)-1], "$TEST --foo --bar 2>&1"), + "incorrect suffix on %q", cmd.Arguments[len(cmd.Arguments)-1], + ) +} + +func TestBuildTestCommand_WithPlaceholderAndNoArguments(t *testing.T) { + c := newClientInstance("test") + state := c.state + + target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target_placeholder"}) + target.AddOutput("remote_test") + target.Test = &core.TestFields{ + Timeout: time.Minute, + Command: "$TEST __TEST_ARGS__ 2>&1", + ArgsPlaceholder: "__TEST_ARGS__", + } + target.IsBinary = true + + cmd, err := c.buildTestCommand(state, target, 1) + assert.NoError(t, err) + + assert.True(t, + strings.HasSuffix(cmd.Arguments[len(cmd.Arguments)-1], "$TEST 2>&1"), + "incorrect suffix on %q", cmd.Arguments[len(cmd.Arguments)-1], + ) +} + +func TestBuildTestCommand_WithoutPlaceholder(t *testing.T) { + c := newClientInstance("test") + state := c.state + state.TestArgs = []string{"--foo", "--bar"} + + target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target_no_placeholder"}) + target.AddOutput("remote_test") + target.Test = &core.TestFields{ + Timeout: time.Minute, + Command: "$TEST", + } + target.IsBinary = true + + cmd, err := c.buildTestCommand(state, target, 1) + assert.NoError(t, err) + + assert.True(t, + strings.HasSuffix(cmd.Arguments[len(cmd.Arguments)-1], "$TEST --foo --bar"), + "incorrect suffix on %q", cmd.Arguments[len(cmd.Arguments)-1], + ) +} From fe52a1976d602c9a12ab70bc3385fed40a6fc766 Mon Sep 17 00:00:00 2001 From: cfranklin <579516+chris-franklin@users.noreply.github.com> Date: Wed, 22 Jul 2026 18:02:08 +0100 Subject: [PATCH 2/5] Move args placeholder logic to core and make use of it in --shell output --- src/core/command_replacements.go | 21 +++++++++ src/core/command_replacements_test.go | 36 +++++++++++++++ src/output/shell_output.go | 8 ++-- src/remote/action.go | 18 +------- src/remote/remote_test.go | 46 +------------------ src/test/results_test.go | 65 ++++----------------------- src/test/test_step.go | 14 +----- 7 files changed, 74 insertions(+), 134 deletions(-) diff --git a/src/core/command_replacements.go b/src/core/command_replacements.go index c3be16df7e..a7267904d7 100644 --- a/src/core/command_replacements.go +++ b/src/core/command_replacements.go @@ -92,6 +92,27 @@ func ReplaceTestSequences(state *BuildState, target *BuildTarget, command string return replaceSequencesInternal(state, target, command, true) } +// TestCommand returns the command to run for a test target, with all sequences and test arguments processed. +func TestCommand(state *BuildState, target *BuildTarget) (string, error) { + cmd, err := ReplaceTestSequences(state, target, target.GetTestCommand(state)) + if err != nil { + return cmd, err + } + if target.Test != nil && target.Test.ArgsPlaceholder != "" { + placeholder := target.Test.ArgsPlaceholder + if strings.Contains(cmd, placeholder) { + args := "" + if len(state.TestArgs) > 0 { + args = strings.Join(state.TestArgs, " ") + } + cmd = strings.ReplaceAll(cmd, placeholder, args) + } + } else if len(state.TestArgs) > 0 { + cmd += " " + strings.Join(state.TestArgs, " ") + } + return cmd, nil +} + // TestWorkerCommand returns the worker & its arguments (if any) for a test, and the command to run for the test itself. func TestWorkerCommand(state *BuildState, target *BuildTarget) (string, string, string, error) { return workerAndArgs(state, target, target.GetTestCommand(state)) diff --git a/src/core/command_replacements_test.go b/src/core/command_replacements_test.go index 510aee77ee..bc2eb44eaa 100644 --- a/src/core/command_replacements_test.go +++ b/src/core/command_replacements_test.go @@ -297,3 +297,39 @@ func (h *testHasher) OutputHash(target *BuildTarget) ([]byte, error) { func (h *testHasher) SetHash(target *BuildTarget, hash []byte) { } + +func TestTestCommand(t *testing.T) { + // Let's create a target and state + target := makeTarget2("//path/to:target1", "python -m unittest", nil) + target.Test = &TestFields{ + Command: "python -m unittest", + } + + state := NewDefaultBuildState() + + // Case 1: No TestArgs + cmd, err := TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest", cmd) + + // Case 2: Appending TestArgs when ArgsPlaceholder is empty + state.TestArgs = []string{"foo", "bar"} + cmd, err = TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest foo bar", cmd) + + // Case 3: Replacing placeholder with TestArgs + target.Test.ArgsPlaceholder = "__TEST_ARGS__" + target.Test.Command = "python -m unittest __TEST_ARGS__" + target.Command = "python -m unittest __TEST_ARGS__" + cmd, err = TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest foo bar", cmd) + + // Case 4: Placeholder is specified but not present in the command + target.Test.Command = "python -m unittest" + target.Command = "python -m unittest" + cmd, err = TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest", cmd) +} diff --git a/src/output/shell_output.go b/src/output/shell_output.go index 972078c477..eaf82cdd85 100644 --- a/src/output/shell_output.go +++ b/src/output/shell_output.go @@ -421,20 +421,22 @@ func printTempDirs(state *core.BuildState, duration time.Duration, shell, shellR state = state.ForArch(state.TargetArch) for _, label := range state.ExpandVisibleOriginalTargets() { target := state.Graph.TargetOrDie(label) - cmd := target.GetCommand(state) + var cmd string dir := target.TmpDir() env := core.StampedBuildEnvironment(state, target, nil, filepath.Join(core.RepoRoot, target.TmpDir()), target.Stamp) shouldSandbox := target.Sandbox if state.NeedTests { - cmd = target.GetTestCommand(state) + cmd, _ = core.TestCommand(state, target) dir = filepath.Join(core.RepoRoot, target.TestDir(1)) env = core.TestEnvironment(state, target, dir, 1) shouldSandbox = target.Test.Sandbox if len(state.TestArgs) > 0 { env["TESTS"] = strings.Join(state.TestArgs, " ") } + } else { + cmd = target.GetCommand(state) + cmd, _ = core.ReplaceSequences(state, target, cmd) } - cmd, _ = core.ReplaceSequences(state, target, cmd) env["CMD"] = cmd fmt.Printf(" %s: %s\n", label, dir) fmt.Printf(" Command: %s\n", cmd) diff --git a/src/remote/action.go b/src/remote/action.go index d61924fc03..917a4641a7 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -159,23 +159,7 @@ func (c *Client) buildTestCommand(state *core.BuildState, target *core.BuildTarg if outs := target.Outputs(); len(outs) > 0 { commandPrefix += `export TEST="$TEST_DIR/` + outs[0] + `" && ` } - cmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state)) - if err == nil { - // If an args placeholder has been defined, then replace it with any provided test arguments... - if target.Test != nil && target.Test.ArgsPlaceholder != "" { - placeholder := target.Test.ArgsPlaceholder - if strings.Contains(cmd, placeholder) { - args := "" - if len(state.TestArgs) > 0 { - args = strings.Join(state.TestArgs, " ") - } - cmd = strings.ReplaceAll(cmd, placeholder, args) - } - } else if len(state.TestArgs) > 0 { - // ...otherwise, just append them to the command. - cmd += " " + strings.Join(state.TestArgs, " ") - } - } + cmd, err := core.TestCommand(state, target) return &pb.Command{ Platform: &pb.Platform{ Properties: []*pb.Platform_Property{ diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index b6215d9747..518cf16148 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -354,7 +354,7 @@ func (c *Client) Store(target *core.BuildTarget) error { return c.uploadLocalTarget(target) } -func TestBuildTestCommand_WithPlaceholder(t *testing.T) { +func TestBuildTestCommand(t *testing.T) { c := newClientInstance("test") state := c.state state.TestArgs = []string{"--foo", "--bar"} @@ -376,47 +376,3 @@ func TestBuildTestCommand_WithPlaceholder(t *testing.T) { "incorrect suffix on %q", cmd.Arguments[len(cmd.Arguments)-1], ) } - -func TestBuildTestCommand_WithPlaceholderAndNoArguments(t *testing.T) { - c := newClientInstance("test") - state := c.state - - target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target_placeholder"}) - target.AddOutput("remote_test") - target.Test = &core.TestFields{ - Timeout: time.Minute, - Command: "$TEST __TEST_ARGS__ 2>&1", - ArgsPlaceholder: "__TEST_ARGS__", - } - target.IsBinary = true - - cmd, err := c.buildTestCommand(state, target, 1) - assert.NoError(t, err) - - assert.True(t, - strings.HasSuffix(cmd.Arguments[len(cmd.Arguments)-1], "$TEST 2>&1"), - "incorrect suffix on %q", cmd.Arguments[len(cmd.Arguments)-1], - ) -} - -func TestBuildTestCommand_WithoutPlaceholder(t *testing.T) { - c := newClientInstance("test") - state := c.state - state.TestArgs = []string{"--foo", "--bar"} - - target := core.NewBuildTarget(core.BuildLabel{PackageName: "package", Name: "target_no_placeholder"}) - target.AddOutput("remote_test") - target.Test = &core.TestFields{ - Timeout: time.Minute, - Command: "$TEST", - } - target.IsBinary = true - - cmd, err := c.buildTestCommand(state, target, 1) - assert.NoError(t, err) - - assert.True(t, - strings.HasSuffix(cmd.Arguments[len(cmd.Arguments)-1], "$TEST --foo --bar"), - "incorrect suffix on %q", cmd.Arguments[len(cmd.Arguments)-1], - ) -} diff --git a/src/test/results_test.go b/src/test/results_test.go index ae4fc483de..cca0211997 100644 --- a/src/test/results_test.go +++ b/src/test/results_test.go @@ -185,60 +185,13 @@ func TestParseGoFileWithLogging(t *testing.T) { func TestTestCommandAndEnv(t *testing.T) { state := core.NewBuildState(core.DefaultConfiguration()) - t.Run("with configured __TEST_ARGS__ placeholder and test args", func(t *testing.T) { - target := core.NewBuildTarget(core.ParseBuildLabel("//src/test:placeholder_test", "")) - target.Test = &core.TestFields{ - Command: "./my_test_binary __TEST_ARGS__ 2>&1", - ArgsPlaceholder: "__TEST_ARGS__", - } - state.TestArgs = []string{"--flag1", "--flag2"} - cmd, _, err := testCommandAndEnv(state, target, 1) - assert.NoError(t, err) - assert.Equal(t, "./my_test_binary --flag1 --flag2 2>&1", cmd) - }) - - t.Run("with configured __TEST_ARGS__ placeholder and no test args", func(t *testing.T) { - target := core.NewBuildTarget(core.ParseBuildLabel("//src/test:placeholder_test", "")) - target.Test = &core.TestFields{ - Command: "./my_test_binary __TEST_ARGS__ 2>&1", - ArgsPlaceholder: "__TEST_ARGS__", - } - state.TestArgs = nil - cmd, _, err := testCommandAndEnv(state, target, 1) - assert.NoError(t, err) - assert.Equal(t, "./my_test_binary 2>&1", cmd) - }) - - t.Run("without placeholder but with __TEST_ARGS__ string present and test args", func(t *testing.T) { - target := core.NewBuildTarget(core.ParseBuildLabel("//src/test:placeholder_test", "")) - target.Test = &core.TestFields{ - Command: "./my_test_binary __TEST_ARGS__ 2>&1", - } - state.TestArgs = []string{"--flag1", "--flag2"} - cmd, _, err := testCommandAndEnv(state, target, 1) - assert.NoError(t, err) - assert.Equal(t, "./my_test_binary __TEST_ARGS__ 2>&1 --flag1 --flag2", cmd) - }) - - t.Run("without placeholder and test args", func(t *testing.T) { - target := core.NewBuildTarget(core.ParseBuildLabel("//src/test:placeholder_test", "")) - target.Test = &core.TestFields{ - Command: "./my_test_binary 2>&1", - } - state.TestArgs = []string{"--flag1", "--flag2"} - cmd, _, err := testCommandAndEnv(state, target, 1) - assert.NoError(t, err) - assert.Equal(t, "./my_test_binary 2>&1 --flag1 --flag2", cmd) - }) - - t.Run("without placeholder and no test args", func(t *testing.T) { - target := core.NewBuildTarget(core.ParseBuildLabel("//src/test:placeholder_test", "")) - target.Test = &core.TestFields{ - Command: "./my_test_binary 2>&1", - } - state.TestArgs = nil - cmd, _, err := testCommandAndEnv(state, target, 1) - assert.NoError(t, err) - assert.Equal(t, "./my_test_binary 2>&1", cmd) - }) + target := core.NewBuildTarget(core.ParseBuildLabel("//src/test:placeholder_test", "")) + target.Test = &core.TestFields{ + Command: "./my_test_binary __TEST_ARGS__ 2>&1", + ArgsPlaceholder: "__TEST_ARGS__", + } + state.TestArgs = []string{"--flag1", "--flag2"} + cmd, _, err := testCommandAndEnv(state, target, 1) + assert.NoError(t, err) + assert.Equal(t, "./my_test_binary --flag1 --flag2 2>&1", cmd) } diff --git a/src/test/test_step.go b/src/test/test_step.go index e1e24ed3f1..2aa7332f26 100644 --- a/src/test/test_step.go +++ b/src/test/test_step.go @@ -351,20 +351,8 @@ func pluralise(word string, quantity int) string { // testCommandAndEnv returns the test command & environment for a target. func testCommandAndEnv(state *core.BuildState, target *core.BuildTarget, run int) (string, core.BuildEnv, error) { - replacedCmd, err := core.ReplaceTestSequences(state, target, target.GetTestCommand(state)) + replacedCmd, err := core.TestCommand(state, target) env := core.TestEnvironment(state, target, filepath.Join(core.RepoRoot, target.TestDir(run)), run) - if target.Test != nil && target.Test.ArgsPlaceholder != "" { - placeholder := target.Test.ArgsPlaceholder - if strings.Contains(replacedCmd, placeholder) { - args := "" - if len(state.TestArgs) > 0 { - args = strings.Join(state.TestArgs, " ") - } - replacedCmd = strings.ReplaceAll(replacedCmd, placeholder, args) - } - } else if len(state.TestArgs) > 0 { - replacedCmd += " " + strings.Join(state.TestArgs, " ") - } return replacedCmd, env, err } From 95fb592a3fb6f193085aa64bad05a5467223ac5f Mon Sep 17 00:00:00 2001 From: cfranklin <579516+chris-franklin@users.noreply.github.com> Date: Wed, 22 Jul 2026 18:31:31 +0100 Subject: [PATCH 3/5] Improve the unit tests --- src/core/command_replacements_test.go | 112 +++++++++++++++++++------- 1 file changed, 81 insertions(+), 31 deletions(-) diff --git a/src/core/command_replacements_test.go b/src/core/command_replacements_test.go index bc2eb44eaa..65f76cf406 100644 --- a/src/core/command_replacements_test.go +++ b/src/core/command_replacements_test.go @@ -299,37 +299,87 @@ func (h *testHasher) SetHash(target *BuildTarget, hash []byte) { } func TestTestCommand(t *testing.T) { - // Let's create a target and state - target := makeTarget2("//path/to:target1", "python -m unittest", nil) - target.Test = &TestFields{ - Command: "python -m unittest", - } - state := NewDefaultBuildState() - // Case 1: No TestArgs - cmd, err := TestCommand(state, target) - assert.NoError(t, err) - assert.Equal(t, "python -m unittest", cmd) - - // Case 2: Appending TestArgs when ArgsPlaceholder is empty - state.TestArgs = []string{"foo", "bar"} - cmd, err = TestCommand(state, target) - assert.NoError(t, err) - assert.Equal(t, "python -m unittest foo bar", cmd) - - // Case 3: Replacing placeholder with TestArgs - target.Test.ArgsPlaceholder = "__TEST_ARGS__" - target.Test.Command = "python -m unittest __TEST_ARGS__" - target.Command = "python -m unittest __TEST_ARGS__" - cmd, err = TestCommand(state, target) - assert.NoError(t, err) - assert.Equal(t, "python -m unittest foo bar", cmd) - - // Case 4: Placeholder is specified but not present in the command - target.Test.Command = "python -m unittest" - target.Command = "python -m unittest" - cmd, err = TestCommand(state, target) - assert.NoError(t, err) - assert.Equal(t, "python -m unittest", cmd) + t.Run("Case 1: No TestArgs", func(t *testing.T) { + target := makeTarget2("//path/to:target1", "python -m unittest", nil) + target.Test = &TestFields{ + Command: "python -m unittest", + } + cmd, err := TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest", cmd) + }) + + t.Run("Case 2: Appending TestArgs when ArgsPlaceholder is empty", func(t *testing.T) { + target := makeTarget2("//path/to:target1", "python -m unittest", nil) + target.Test = &TestFields{ + Command: "python -m unittest", + } + state.TestArgs = []string{"foo", "bar"} + cmd, err := TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest foo bar", cmd) + }) + + t.Run("Case 3: Replacing placeholder with TestArgs in the middle of a command", func(t *testing.T) { + target := makeTarget2("//path/to:target1", "python -m unittest __TEST_ARGS__ 2>&1", nil) + target.Test = &TestFields{ + Command: "python -m unittest __TEST_ARGS__ 2>&1", + ArgsPlaceholder: "__TEST_ARGS__", + } + state.TestArgs = []string{"foo", "bar"} + cmd, err := TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest foo bar 2>&1", cmd) + }) + + t.Run("Case 4: Placeholder is specified but not present in the command", func(t *testing.T) { + target := makeTarget2("//path/to:target1", "python -m unittest", nil) + target.Test = &TestFields{ + Command: "python -m unittest", + ArgsPlaceholder: "__TEST_ARGS__", + } + state.TestArgs = []string{"foo", "bar"} + cmd, err := TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "python -m unittest", cmd) + }) + + t.Run("Case 5: Multiple occurrences of placeholder", func(t *testing.T) { + target := makeTarget2("//path/to:target1", "echo __TEST_ARGS__ and __TEST_ARGS__", nil) + target.Test = &TestFields{ + Command: "echo __TEST_ARGS__ and __TEST_ARGS__", + ArgsPlaceholder: "__TEST_ARGS__", + } + state.TestArgs = []string{"foo", "bar"} + cmd, err := TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "echo foo bar and foo bar", cmd) + }) + + t.Run("Case 6: Combined sequence and placeholder replacement", func(t *testing.T) { + target2 := makeTarget2("//path/to:target2", "", nil) + target1 := makeTarget2("//path/to:target1", "$(location //path/to:target2) __TEST_ARGS__", target2) + target1.Test = &TestFields{ + Command: "$(location //path/to:target2) __TEST_ARGS__", + ArgsPlaceholder: "__TEST_ARGS__", + } + state.TestArgs = []string{"--verbose"} + cmd, err := TestCommand(state, target1) + assert.NoError(t, err) + assert.Equal(t, "path/to/target2.py --verbose", cmd) + }) + + t.Run("Case 7: Empty test command fallback (defaults to target binary) and appending", func(t *testing.T) { + target := makeTarget2("//path/to:target1", "", nil) + target.IsBinary = true + target.Test = &TestFields{ + Command: "", + } + state.TestArgs = []string{"--foo", "--bar"} + cmd, err := TestCommand(state, target) + assert.NoError(t, err) + assert.Equal(t, "./target1.py --foo --bar", cmd) + }) } From 0d4111612c14157f75fd72a7cf1de0850af3a054 Mon Sep 17 00:00:00 2001 From: cfranklin <579516+chris-franklin@users.noreply.github.com> Date: Thu, 23 Jul 2026 11:35:29 +0100 Subject: [PATCH 4/5] Return error if placeholder specified but not used, and handle errors from `TestCommand` --- src/core/command_replacements.go | 4 +++- src/core/command_replacements_test.go | 18 +++++++++--------- src/output/shell_output.go | 10 +++++++--- src/remote/remote_test.go | 2 +- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/core/command_replacements.go b/src/core/command_replacements.go index a7267904d7..9210be8610 100644 --- a/src/core/command_replacements.go +++ b/src/core/command_replacements.go @@ -59,7 +59,7 @@ import ( "runtime/debug" "strings" - "github.com/peterebden/go-deferred-regex" + deferredregex "github.com/peterebden/go-deferred-regex" "github.com/thought-machine/please/src/fs" ) @@ -106,6 +106,8 @@ func TestCommand(state *BuildState, target *BuildTarget) (string, error) { args = strings.Join(state.TestArgs, " ") } cmd = strings.ReplaceAll(cmd, placeholder, args) + } else { + return "", fmt.Errorf("command %q does not contain expected arguments placeholder %q", cmd, target.Test.ArgsPlaceholder) } } else if len(state.TestArgs) > 0 { cmd += " " + strings.Join(state.TestArgs, " ") diff --git a/src/core/command_replacements_test.go b/src/core/command_replacements_test.go index 65f76cf406..4187278cdf 100644 --- a/src/core/command_replacements_test.go +++ b/src/core/command_replacements_test.go @@ -301,7 +301,7 @@ func (h *testHasher) SetHash(target *BuildTarget, hash []byte) { func TestTestCommand(t *testing.T) { state := NewDefaultBuildState() - t.Run("Case 1: No TestArgs", func(t *testing.T) { + t.Run("No TestArgs", func(t *testing.T) { target := makeTarget2("//path/to:target1", "python -m unittest", nil) target.Test = &TestFields{ Command: "python -m unittest", @@ -311,7 +311,7 @@ func TestTestCommand(t *testing.T) { assert.Equal(t, "python -m unittest", cmd) }) - t.Run("Case 2: Appending TestArgs when ArgsPlaceholder is empty", func(t *testing.T) { + t.Run("Appending TestArgs when ArgsPlaceholder is empty", func(t *testing.T) { target := makeTarget2("//path/to:target1", "python -m unittest", nil) target.Test = &TestFields{ Command: "python -m unittest", @@ -322,7 +322,7 @@ func TestTestCommand(t *testing.T) { assert.Equal(t, "python -m unittest foo bar", cmd) }) - t.Run("Case 3: Replacing placeholder with TestArgs in the middle of a command", func(t *testing.T) { + t.Run("Replacing placeholder with TestArgs in the middle of a command", func(t *testing.T) { target := makeTarget2("//path/to:target1", "python -m unittest __TEST_ARGS__ 2>&1", nil) target.Test = &TestFields{ Command: "python -m unittest __TEST_ARGS__ 2>&1", @@ -334,7 +334,7 @@ func TestTestCommand(t *testing.T) { assert.Equal(t, "python -m unittest foo bar 2>&1", cmd) }) - t.Run("Case 4: Placeholder is specified but not present in the command", func(t *testing.T) { + t.Run("Placeholder is specified but not present in the command", func(t *testing.T) { target := makeTarget2("//path/to:target1", "python -m unittest", nil) target.Test = &TestFields{ Command: "python -m unittest", @@ -342,11 +342,11 @@ func TestTestCommand(t *testing.T) { } state.TestArgs = []string{"foo", "bar"} cmd, err := TestCommand(state, target) - assert.NoError(t, err) - assert.Equal(t, "python -m unittest", cmd) + assert.EqualError(t, err, `command "python -m unittest" does not contain expected arguments placeholder "__TEST_ARGS__"`) + assert.Empty(t, cmd) }) - t.Run("Case 5: Multiple occurrences of placeholder", func(t *testing.T) { + t.Run("Multiple occurrences of placeholder", func(t *testing.T) { target := makeTarget2("//path/to:target1", "echo __TEST_ARGS__ and __TEST_ARGS__", nil) target.Test = &TestFields{ Command: "echo __TEST_ARGS__ and __TEST_ARGS__", @@ -358,7 +358,7 @@ func TestTestCommand(t *testing.T) { assert.Equal(t, "echo foo bar and foo bar", cmd) }) - t.Run("Case 6: Combined sequence and placeholder replacement", func(t *testing.T) { + t.Run("Combined sequence and placeholder replacement", func(t *testing.T) { target2 := makeTarget2("//path/to:target2", "", nil) target1 := makeTarget2("//path/to:target1", "$(location //path/to:target2) __TEST_ARGS__", target2) target1.Test = &TestFields{ @@ -371,7 +371,7 @@ func TestTestCommand(t *testing.T) { assert.Equal(t, "path/to/target2.py --verbose", cmd) }) - t.Run("Case 7: Empty test command fallback (defaults to target binary) and appending", func(t *testing.T) { + t.Run("Empty test command fallback (defaults to target binary) and appending", func(t *testing.T) { target := makeTarget2("//path/to:target1", "", nil) target.IsBinary = true target.Test = &TestFields{ diff --git a/src/output/shell_output.go b/src/output/shell_output.go index eaf82cdd85..be5c70a53f 100644 --- a/src/output/shell_output.go +++ b/src/output/shell_output.go @@ -13,7 +13,7 @@ import ( "strings" "time" - "github.com/peterebden/go-deferred-regex" + deferredregex "github.com/peterebden/go-deferred-regex" "github.com/thought-machine/please/src/cli" "github.com/thought-machine/please/src/core" @@ -422,11 +422,12 @@ func printTempDirs(state *core.BuildState, duration time.Duration, shell, shellR for _, label := range state.ExpandVisibleOriginalTargets() { target := state.Graph.TargetOrDie(label) var cmd string + var err error dir := target.TmpDir() env := core.StampedBuildEnvironment(state, target, nil, filepath.Join(core.RepoRoot, target.TmpDir()), target.Stamp) shouldSandbox := target.Sandbox if state.NeedTests { - cmd, _ = core.TestCommand(state, target) + cmd, err = core.TestCommand(state, target) dir = filepath.Join(core.RepoRoot, target.TestDir(1)) env = core.TestEnvironment(state, target, dir, 1) shouldSandbox = target.Test.Sandbox @@ -435,7 +436,10 @@ func printTempDirs(state *core.BuildState, duration time.Duration, shell, shellR } } else { cmd = target.GetCommand(state) - cmd, _ = core.ReplaceSequences(state, target, cmd) + cmd, err = core.ReplaceSequences(state, target, cmd) + } + if err != nil { + log.Errorf("Error pre-processing command: %s", err.Error()) } env["CMD"] = cmd fmt.Printf(" %s: %s\n", label, dir) diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index 518cf16148..21b703a3d1 100644 --- a/src/remote/remote_test.go +++ b/src/remote/remote_test.go @@ -373,6 +373,6 @@ func TestBuildTestCommand(t *testing.T) { assert.True(t, strings.HasSuffix(cmd.Arguments[len(cmd.Arguments)-1], "$TEST --foo --bar 2>&1"), - "incorrect suffix on %q", cmd.Arguments[len(cmd.Arguments)-1], + `expected suffix "$TEST --foo --bar 2>&1" on %q`, cmd.Arguments[len(cmd.Arguments)-1], ) } From 608005c9d0d0a9c50b597b3773419ad4520df394 Mon Sep 17 00:00:00 2001 From: cfranklin <579516+chris-franklin@users.noreply.github.com> Date: Thu, 23 Jul 2026 12:23:12 +0100 Subject: [PATCH 5/5] Invert and return early --- src/core/command_replacements.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/command_replacements.go b/src/core/command_replacements.go index 9210be8610..07f1b98e67 100644 --- a/src/core/command_replacements.go +++ b/src/core/command_replacements.go @@ -100,15 +100,14 @@ func TestCommand(state *BuildState, target *BuildTarget) (string, error) { } if target.Test != nil && target.Test.ArgsPlaceholder != "" { placeholder := target.Test.ArgsPlaceholder - if strings.Contains(cmd, placeholder) { - args := "" - if len(state.TestArgs) > 0 { - args = strings.Join(state.TestArgs, " ") - } - cmd = strings.ReplaceAll(cmd, placeholder, args) - } else { + if !strings.Contains(cmd, placeholder) { return "", fmt.Errorf("command %q does not contain expected arguments placeholder %q", cmd, target.Test.ArgsPlaceholder) } + args := "" + if len(state.TestArgs) > 0 { + args = strings.Join(state.TestArgs, " ") + } + cmd = strings.ReplaceAll(cmd, placeholder, args) } else if len(state.TestArgs) > 0 { cmd += " " + strings.Join(state.TestArgs, " ") }