diff --git a/src/core/command_replacements.go b/src/core/command_replacements.go index c3be16df7e..07f1b98e67 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" ) @@ -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)) diff --git a/src/core/command_replacements_test.go b/src/core/command_replacements_test.go index 510aee77ee..4187278cdf 100644 --- a/src/core/command_replacements_test.go +++ b/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) + }) +} diff --git a/src/output/shell_output.go b/src/output/shell_output.go index 972078c477..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" @@ -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) diff --git a/src/remote/action.go b/src/remote/action.go index 3b2f39782c..917a4641a7 100644 --- a/src/remote/action.go +++ b/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{ diff --git a/src/remote/remote_test.go b/src/remote/remote_test.go index 05af2cd9ba..21b703a3d1 100644 --- a/src/remote/remote_test.go +++ b/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], + ) +} 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 }