From 88f8591decbaa5cf13910d31c60c148e21a405e6 Mon Sep 17 00:00:00 2001 From: Iris Scholten Date: Mon, 13 Jul 2026 16:28:18 -0700 Subject: [PATCH] feat(tests): derive splits from Depot CI matrix Use Depot CI matrix position defaults when shard flags are omitted, while preserving explicit overrides and unsplit runs. --- pkg/cmd/tests/run.go | 6 +- pkg/cmd/tests/run_test.go | 136 ++++++++++++++++++++++++++-- pkg/cmd/tests/selection.go | 72 +++++++++++++++ pkg/cmd/tests/selection_test.go | 151 ++++++++++++++++++++++++++++++++ pkg/cmd/tests/split.go | 4 + pkg/cmd/tests/split_test.go | 95 ++++++++++++++++++++ pkg/cmd/tests/tests_test.go | 18 ++++ 7 files changed, 475 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/tests/run.go b/pkg/cmd/tests/run.go index 28cf8b80..8c39d85d 100644 --- a/pkg/cmd/tests/run.go +++ b/pkg/cmd/tests/run.go @@ -101,6 +101,10 @@ func runTestsRun(cmd *cobra.Command, opts runOptions) error { key: opts.splitKey, output: splitOutputText, } + splitOpts, err := resolveSplitOptions(cmd, splitOpts) + if err != nil { + return cancellationErrorOr(err) + } splitRequested := runSplitRequested(splitOpts) mode, candidates, selectedCandidates, splitResponse, err := selectRunCandidates(cmd, splitOpts) if err != nil { @@ -193,7 +197,7 @@ func selectRunCandidates(cmd *cobra.Command, opts splitOptions) (splitMode, []st } func runSplitRequested(opts splitOptions) bool { - return opts.index >= 0 || opts.total != 0 + return opts.total > 1 } func uploadAndSummarizeTestReports(cmd *cobra.Command, reportPaths []string, key string, baseline reportFileBaseline) error { diff --git a/pkg/cmd/tests/run_test.go b/pkg/cmd/tests/run_test.go index ef2374e0..f90d392f 100644 --- a/pkg/cmd/tests/run_test.go +++ b/pkg/cmd/tests/run_test.go @@ -178,6 +178,92 @@ func TestRunUsesDefaultsForOmittedKeys(t *testing.T) { } } +func TestRunUsesMatrixEnvironmentDefaults(t *testing.T) { + resetTestHooks(t) + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "1") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "2") + workspace := t.TempDir() + t.Chdir(workspace) + writeTempFileAt(t, workspace, "reports/junit.xml", "") + + var splitReq *testresultsv1.SplitTestsRequest + resolveOIDCCredentialFunc = func(context.Context) (string, error) { return "oidc-token", nil } + splitTestsFunc = func(_ context.Context, _ string, req *testresultsv1.SplitTestsRequest) (*testresultsv1.SplitTestsResponse, error) { + splitReq = req + return &testresultsv1.SplitTestsResponse{ + Candidates: []string{"b.test.ts"}, + CandidatesWithTimings: 1, + }, nil + } + var commandCandidates []string + runShellCommandFunc = func(_ context.Context, _ string, candidates []string, _ io.Writer, _ io.Writer) (int, error) { + commandCandidates = append([]string(nil), candidates...) + writeRunReport(t, workspace) + return 0, nil + } + reportTestResultsFunc = func(context.Context, string, *testresultsv1.ReportTestResultsRequest) (*testresultsv1.ReportTestResultsResponse, error) { + return &testresultsv1.ReportTestResultsResponse{FilesProcessed: 1}, nil + } + + _, _, err := executeCommandWithInputOutput( + "a.test.ts\nb.test.ts\n", + "run", + "--command", "test-command", + "--report-path", "reports/junit.xml", + ) + if err != nil { + t.Fatal(err) + } + if splitReq == nil || splitReq.GetShardIndex() != 1 || splitReq.GetShardTotal() != 2 { + t.Fatalf("expected matrix shard 1/2, got %#v", splitReq) + } + if !equalStrings(commandCandidates, []string{"b.test.ts"}) { + t.Fatalf("expected selected candidates on command stdin, got %v", commandCandidates) + } +} + +func TestRunMatrixSingleShardPreservesUnsplitBehavior(t *testing.T) { + resetTestHooks(t) + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "0") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "1") + workspace := t.TempDir() + t.Chdir(workspace) + writeTempFileAt(t, workspace, "reports/junit.xml", "") + resolveOIDCCredentialFunc = func(context.Context) (string, error) { return "oidc-token", nil } + splitTestsFunc = func(context.Context, string, *testresultsv1.SplitTestsRequest) (*testresultsv1.SplitTestsResponse, error) { + t.Fatal("single-shard run should not call SplitTests") + return nil, nil + } + + var commandRan bool + runShellCommandFunc = func(_ context.Context, _ string, candidates []string, _ io.Writer, _ io.Writer) (int, error) { + if candidates != nil { + t.Fatalf("expected no command candidates, got %v", candidates) + } + commandRan = true + writeRunReport(t, workspace) + return 0, nil + } + reportTestResultsFunc = func(context.Context, string, *testresultsv1.ReportTestResultsRequest) (*testresultsv1.ReportTestResultsResponse, error) { + return &testresultsv1.ReportTestResultsResponse{FilesProcessed: 1}, nil + } + + _, stderr, err := executeCommandOutput( + "run", + "--command", "test-command", + "--report-path", "reports/junit.xml", + ) + if err != nil { + t.Fatal(err) + } + if !commandRan { + t.Fatal("expected test command to run") + } + if !strings.Contains(stderr, "Depot is running against a single shard.") { + t.Fatalf("expected single-shard summary, got %q", stderr) + } +} + func TestRunWithoutShardFlagsRunsAllCandidatesWithoutSplitting(t *testing.T) { resetTestHooks(t) workspace := t.TempDir() @@ -1034,22 +1120,60 @@ func TestRunRejectsUnsupportedTimingIdentityPair(t *testing.T) { } } -func TestRunRequiresCandidatesWhenStdinIsTerminal(t *testing.T) { +func TestRunTotalOneDoesNotRequireCandidatesWhenStdinIsTerminal(t *testing.T) { resetTestHooks(t) - runShellCommandFunc = func(context.Context, string, []string, io.Writer, io.Writer) (int, error) { - t.Fatal("command should not run") + workspace := t.TempDir() + t.Chdir(workspace) + writeTempFileAt(t, workspace, "reports/junit.xml", "") + splitTestsFunc = func(context.Context, string, *testresultsv1.SplitTestsRequest) (*testresultsv1.SplitTestsResponse, error) { + t.Fatal("single-shard run should not call SplitTests") + return nil, nil + } + resolveOIDCCredentialFunc = func(context.Context) (string, error) { return "oidc-token", nil } + + var commandRan bool + runShellCommandFunc = func(_ context.Context, _ string, candidates []string, _ io.Writer, _ io.Writer) (int, error) { + if candidates != nil { + t.Fatalf("expected no command candidates, got %v", candidates) + } + commandRan = true + writeRunReport(t, workspace) return 0, nil } + reportTestResultsFunc = func(context.Context, string, *testresultsv1.ReportTestResultsRequest) (*testresultsv1.ReportTestResultsResponse, error) { + return &testresultsv1.ReportTestResultsResponse{FilesProcessed: 1}, nil + } _, _, err := executeCommandOutput( "run", "--index", "0", "--total", "1", "--command", "test-command", - "--report-path", "junit.xml", + "--report-path", "reports/junit.xml", ) - if err == nil || !strings.Contains(err.Error(), "pipe newline-delimited candidates") || !strings.Contains(err.Error(), "--candidates-file") { - t.Fatalf("expected missing candidates error, got %v", err) + if err != nil { + t.Fatal(err) + } + if !commandRan { + t.Fatal("expected test command to run") + } +} + +func TestRunSplitRequested(t *testing.T) { + for _, tt := range []struct { + name string + opts splitOptions + want bool + }{ + {name: "no shard configuration", opts: splitOptions{index: -1}, want: false}, + {name: "single shard", opts: splitOptions{index: 0, total: 1}, want: false}, + {name: "multiple shards", opts: splitOptions{index: 0, total: 2}, want: true}, + } { + t.Run(tt.name, func(t *testing.T) { + if got := runSplitRequested(tt.opts); got != tt.want { + t.Fatalf("runSplitRequested(%+v) = %t, want %t", tt.opts, got, tt.want) + } + }) } } diff --git a/pkg/cmd/tests/selection.go b/pkg/cmd/tests/selection.go index 3e332bba..0d4d1715 100644 --- a/pkg/cmd/tests/selection.go +++ b/pkg/cmd/tests/selection.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strconv" "strings" "time" @@ -21,6 +22,77 @@ var ( const splitTestsRequestTimeout = 2 * time.Minute +const ( + matrixJobIndexEnv = "DEPOT_MATRIX_JOB_INDEX" + matrixJobTotalEnv = "DEPOT_MATRIX_JOB_TOTAL" +) + +func resolveSplitOptions(cmd *cobra.Command, opts splitOptions) (splitOptions, error) { + flags := cmd.Flags() + indexExplicit := flags.Changed("index") + totalExplicit := flags.Changed("total") + explicitTotalOne := totalExplicit && opts.total == 1 && !indexExplicit + shardConfigured := indexExplicit || totalExplicit + + indexFromEnv := false + if !indexExplicit && !explicitTotalOne { + index, set, err := matrixShardOption(matrixJobIndexEnv) + if err != nil { + return opts, err + } + if set { + opts.index = index + indexFromEnv = true + shardConfigured = true + } + } + + totalFromEnv := false + if !totalExplicit { + total, set, err := matrixShardOption(matrixJobTotalEnv) + if err != nil { + return opts, err + } + if set { + opts.total = total + totalFromEnv = true + shardConfigured = true + } + } + + if indexFromEnv && !totalExplicit && !totalFromEnv { + return opts, fmt.Errorf("%s must be set when %s is set", matrixJobTotalEnv, matrixJobIndexEnv) + } + if totalFromEnv && !indexExplicit && !indexFromEnv { + return opts, fmt.Errorf("%s must be set when %s is set", matrixJobIndexEnv, matrixJobTotalEnv) + } + + if explicitTotalOne { + opts.index = 0 + } + if !shardConfigured { + return opts, nil + } + if err := validateShard(opts.index, opts.total); err != nil { + return opts, err + } + + return opts, nil +} + +func matrixShardOption(name string) (int, bool, error) { + value, set := os.LookupEnv(name) + if !set { + return 0, false, nil + } + + parsed, err := strconv.Atoi(strings.TrimSpace(value)) + if err != nil { + return 0, true, fmt.Errorf("invalid %s value %q: must be an integer", name, value) + } + return parsed, true, nil +} + func selectTestCandidates(cmd *cobra.Command, opts splitOptions) (splitMode, []string, []string, *testresultsv1.SplitTestsResponse, error) { if err := validateShard(opts.index, opts.total); err != nil { return "", nil, nil, nil, err diff --git a/pkg/cmd/tests/selection_test.go b/pkg/cmd/tests/selection_test.go index 033ad071..7981e1d6 100644 --- a/pkg/cmd/tests/selection_test.go +++ b/pkg/cmd/tests/selection_test.go @@ -4,8 +4,159 @@ import ( "testing" testresultsv1 "github.com/depot/cli/pkg/proto/depot/testresults/v1" + "github.com/spf13/cobra" ) +func TestResolveSplitOptionsUsesMatrixEnvironmentDefaults(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "1") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "2") + + cmd, opts := newSplitOptionsCommand(t) + resolved, err := resolveSplitOptions(cmd, opts) + if err != nil { + t.Fatal(err) + } + if resolved.index != 1 || resolved.total != 2 { + t.Fatalf("expected matrix shard 1/2, got %d/%d", resolved.index, resolved.total) + } +} + +func TestResolveSplitOptionsPrefersExplicitFlags(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "1") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "4") + + cmd, opts := newSplitOptionsCommand(t, "--index", "0", "--total", "2") + resolved, err := resolveSplitOptions(cmd, opts) + if err != nil { + t.Fatal(err) + } + if resolved.index != 0 || resolved.total != 2 { + t.Fatalf("expected explicit shard 0/2, got %d/%d", resolved.index, resolved.total) + } +} + +func TestResolveSplitOptionsCombinesExplicitAndMatrixValues(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "1") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "4") + + cmd, opts := newSplitOptionsCommand(t, "--index", "0") + resolved, err := resolveSplitOptions(cmd, opts) + if err != nil { + t.Fatal(err) + } + if resolved.index != 0 || resolved.total != 4 { + t.Fatalf("expected explicit index with matrix total 0/4, got %d/%d", resolved.index, resolved.total) + } +} + +func TestResolveSplitOptionsUsesMatrixIndexWithExplicitTotal(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "1") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "4") + + cmd, opts := newSplitOptionsCommand(t, "--total", "2") + resolved, err := resolveSplitOptions(cmd, opts) + if err != nil { + t.Fatal(err) + } + if resolved.index != 1 || resolved.total != 2 { + t.Fatalf("expected matrix index with explicit total 1/2, got %d/%d", resolved.index, resolved.total) + } +} + +func TestResolveSplitOptionsTotalOneOverridesMatrixIndex(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "3") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "4") + + cmd, opts := newSplitOptionsCommand(t, "--total", "1") + resolved, err := resolveSplitOptions(cmd, opts) + if err != nil { + t.Fatal(err) + } + if resolved.index != 0 || resolved.total != 1 { + t.Fatalf("expected explicit total one to resolve to shard 0/1, got %d/%d", resolved.index, resolved.total) + } +} + +func TestResolveSplitOptionsPreservesExplicitInvalidFlags(t *testing.T) { + for _, tt := range []struct { + name string + args []string + want string + }{ + {name: "zero total", args: []string{"--index", "-1", "--total", "0"}, want: "--total must be greater than 0"}, + {name: "invalid index with total one", args: []string{"--index", "-1", "--total", "1"}, want: "--index must be greater than or equal to 0"}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "0") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "2") + + cmd, opts := newSplitOptionsCommand(t, tt.args...) + _, err := resolveSplitOptions(cmd, opts) + if err == nil || err.Error() != tt.want { + t.Fatalf("expected explicit invalid shard values to fail validation, got %v", err) + } + }) + } +} + +func TestResolveSplitOptionsAttributesMixedValidationErrorsToFlags(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "0") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "2") + + cmd, opts := newSplitOptionsCommand(t, "--total", "0") + _, err := resolveSplitOptions(cmd, opts) + if err == nil || err.Error() != "--total must be greater than 0" { + t.Fatalf("expected explicit total error without matrix attribution, got %v", err) + } +} + +func TestResolveSplitOptionsRejectsInvalidMatrixEnvironment(t *testing.T) { + t.Run("malformed index", func(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "one") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "2") + + cmd, opts := newSplitOptionsCommand(t) + _, err := resolveSplitOptions(cmd, opts) + if err == nil || err.Error() != `invalid DEPOT_MATRIX_JOB_INDEX value "one": must be an integer` { + t.Fatalf("expected matrix index parse error, got %v", err) + } + }) + + t.Run("incomplete pair", func(t *testing.T) { + unsetEnv(t, matrixJobTotalEnv) + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "0") + + cmd, opts := newSplitOptionsCommand(t) + _, err := resolveSplitOptions(cmd, opts) + if err == nil || err.Error() != "DEPOT_MATRIX_JOB_TOTAL must be set when DEPOT_MATRIX_JOB_INDEX is set" { + t.Fatalf("expected incomplete matrix environment error, got %v", err) + } + }) + + t.Run("index outside single shard", func(t *testing.T) { + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "3") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "1") + + cmd, opts := newSplitOptionsCommand(t) + _, err := resolveSplitOptions(cmd, opts) + if err == nil || err.Error() != "--index must be less than --total" { + t.Fatalf("expected invalid matrix shard values to fail validation, got %v", err) + } + }) +} + +func newSplitOptionsCommand(t *testing.T, args ...string) (*cobra.Command, splitOptions) { + t.Helper() + opts := splitOptions{index: -1} + cmd := &cobra.Command{Use: "test"} + cmd.Flags().IntVar(&opts.index, "index", -1, "") + cmd.Flags().IntVar(&opts.total, "total", 0, "") + if err := cmd.ParseFlags(args); err != nil { + t.Fatal(err) + } + return cmd, opts +} + func TestResolveCandidateIdentityInfersFilenamesFromSourceExtensions(t *testing.T) { identity, err := resolveCandidateIdentity("", []string{ "src/example.test.ts", diff --git a/pkg/cmd/tests/split.go b/pkg/cmd/tests/split.go index 7a1309e6..23deed0e 100644 --- a/pkg/cmd/tests/split.go +++ b/pkg/cmd/tests/split.go @@ -79,6 +79,10 @@ func runTestsSplit(cmd *cobra.Command, opts splitOptions) error { if err != nil { return err } + opts, err = resolveSplitOptions(cmd, opts) + if err != nil { + return err + } mode, candidates, selectedCandidates, splitResponse, err := selectTestCandidates(cmd, opts) if err != nil { diff --git a/pkg/cmd/tests/split_test.go b/pkg/cmd/tests/split_test.go index f583237b..83c66346 100644 --- a/pkg/cmd/tests/split_test.go +++ b/pkg/cmd/tests/split_test.go @@ -70,6 +70,101 @@ func TestSplitDefaultsToTimingSplit(t *testing.T) { } } +func TestSplitUsesMatrixEnvironmentDefaults(t *testing.T) { + resetTestHooks(t) + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "1") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "2") + resolveOIDCCredentialFunc = func(context.Context) (string, error) { return "oidc-token", nil } + + var splitReq *testresultsv1.SplitTestsRequest + splitTestsFunc = func(_ context.Context, _ string, req *testresultsv1.SplitTestsRequest) (*testresultsv1.SplitTestsResponse, error) { + splitReq = req + return &testresultsv1.SplitTestsResponse{ + Candidates: []string{"b.test.ts"}, + CandidatesRequested: 2, + CandidatesSelected: 1, + CandidatesWithTimings: 1, + }, nil + } + + stdout, stderr, err := executeCommandWithInputOutput( + "a.test.ts\nb.test.ts\n", + "split", + "--output", "json", + ) + if err != nil { + t.Fatal(err) + } + if stderr != "" { + t.Fatalf("expected no text summary for json output, got %q", stderr) + } + if splitReq == nil || splitReq.GetShardIndex() != 1 || splitReq.GetShardTotal() != 2 { + t.Fatalf("expected matrix shard 1/2, got %#v", splitReq) + } + + var out splitOutput + if err := json.Unmarshal([]byte(stdout), &out); err != nil { + t.Fatalf("expected valid json, got %q: %v", stdout, err) + } + if out.ShardIndex != 1 || out.ShardTotal != 2 { + t.Fatalf("expected matrix shard metadata 1/2, got %#v", out) + } +} + +func TestSplitExplicitFlagsOverrideMatrixEnvironment(t *testing.T) { + resetTestHooks(t) + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "1") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "4") + resolveOIDCCredentialFunc = func(context.Context) (string, error) { return "oidc-token", nil } + + var splitReq *testresultsv1.SplitTestsRequest + splitTestsFunc = func(_ context.Context, _ string, req *testresultsv1.SplitTestsRequest) (*testresultsv1.SplitTestsResponse, error) { + splitReq = req + return &testresultsv1.SplitTestsResponse{Candidates: []string{"a.test.ts"}, CandidatesWithTimings: 1}, nil + } + + _, _, err := executeCommandWithInputOutput( + "a.test.ts\n", + "split", + "--index", "0", + "--total", "2", + ) + if err != nil { + t.Fatal(err) + } + if splitReq == nil || splitReq.GetShardIndex() != 0 || splitReq.GetShardTotal() != 2 { + t.Fatalf("expected explicit shard 0/2, got %#v", splitReq) + } +} + +func TestSplitMatrixSingleShardPrintsAllCandidatesWithoutSplitting(t *testing.T) { + resetTestHooks(t) + t.Setenv("DEPOT_MATRIX_JOB_INDEX", "0") + t.Setenv("DEPOT_MATRIX_JOB_TOTAL", "1") + resolveOIDCCredentialFunc = func(context.Context) (string, error) { + t.Fatal("single-shard split should not need OIDC") + return "", nil + } + splitTestsFunc = func(context.Context, string, *testresultsv1.SplitTestsRequest) (*testresultsv1.SplitTestsResponse, error) { + t.Fatal("single-shard split should not call SplitTests") + return nil, nil + } + + stdout, stderr, err := executeCommandWithInputOutput( + "a.test.ts\nb.test.ts\n", + "split", + ) + if err != nil { + t.Fatal(err) + } + if stdout != "a.test.ts\nb.test.ts\n" { + t.Fatalf("expected all candidates on stdout, got %q", stdout) + } + if !strings.Contains(stderr, "selected all 2 candidate") { + t.Fatalf("expected single-shard summary, got %q", stderr) + } +} + func TestSplitSupportsJSONOutput(t *testing.T) { resetTestHooks(t) resolveOIDCCredentialFunc = func(context.Context) (string, error) { return "oidc-token", nil } diff --git a/pkg/cmd/tests/tests_test.go b/pkg/cmd/tests/tests_test.go index 2c7e88c5..54fe60aa 100644 --- a/pkg/cmd/tests/tests_test.go +++ b/pkg/cmd/tests/tests_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "io" + "os" "strings" "testing" "unicode/utf8" @@ -493,6 +494,8 @@ func requireContextDeadline(t *testing.T, ctx context.Context, name string) { func resetTestHooks(t *testing.T) { t.Helper() t.Setenv("GITHUB_WORKSPACE", "") + unsetEnv(t, matrixJobIndexEnv) + unsetEnv(t, matrixJobTotalEnv) resolveOrgAuthFunc = func(context.Context, string) (string, error) { return "token-1", nil @@ -551,6 +554,21 @@ func resetTestHooks(t *testing.T) { }) } +func unsetEnv(t *testing.T, key string) { + t.Helper() + value, set := os.LookupEnv(key) + if err := os.Unsetenv(key); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if set { + _ = os.Setenv(key, value) + return + } + _ = os.Unsetenv(key) + }) +} + func equalStatuses(left, right []testresultsv1.TestResultStatus) bool { if len(left) != len(right) { return false