Add support for test arguments to remote test commands

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"
 )
@@ -92,6 +92,28 @@ 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) {
+			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, " ")
+	}
+	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))

src/core/command_replacements_test.go

@@ -297,3 +297,89 @@ func (h *testHasher) OutputHash(target *BuildTarget) ([]byte, error) {
 
 func (h *testHasher) SetHash(target *BuildTarget, hash []byte) {
 }
+
+func TestTestCommand(t *testing.T) {
+	state := NewDefaultBuildState()
+
+	t.Run("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("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("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("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.EqualError(t, err, `command "python -m unittest" does not contain expected arguments placeholder "__TEST_ARGS__"`)
+		assert.Empty(t, cmd)
+	})
+
+	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__",
+			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("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("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)
+	})
+}

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"
@@ -421,20 +421,26 @@ 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
+		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 = target.GetTestCommand(state)
+			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
 			if len(state.TestArgs) > 0 {
 				env["TESTS"] = strings.Join(state.TestArgs, " ")
 			}
+		} else {
+			cmd = target.GetCommand(state)
+			cmd, err = core.ReplaceSequences(state, target, cmd)
+		}
+		if err != nil {
+			log.Errorf("Error pre-processing command: %s", err.Error())
 		}
-		cmd, _ = core.ReplaceSequences(state, target, cmd)
 		env["CMD"] = cmd
 		fmt.Printf("  %s: %s\n", label, dir)
 		fmt.Printf("    Command: %s\n", cmd)

src/remote/action.go

@@ -159,7 +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))
+	cmd, err := core.TestCommand(state, target)
 	return &pb.Command{
 		Platform: &pb.Platform{
 			Properties: []*pb.Platform_Property{

src/remote/remote_test.go

@@ -353,3 +353,26 @@ func (c *Client) Store(target *core.BuildTarget) error {
 	}
 	return c.uploadLocalTarget(target)
 }
+
+func TestBuildTestCommand(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"),
+		`expected suffix "$TEST --foo --bar 2>&1" on %q`, cmd.Arguments[len(cmd.Arguments)-1],
+	)
+}

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)
 }

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
 }