From fb2f9b7c86f8a21769a717525d83e1547756ef61 Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Sun, 26 Apr 2026 18:02:17 +0300 Subject: [PATCH 1/5] fix(test,action): allow flag-after-positional in gavel test and skip --show-passed for non-test subcommands The flanksource/gavel composite action appends --no-progress --no-color --show-passed --format ... after the user-supplied $GAVEL_ARGS. Two issues prevented this from working: 1. cmd/gavel/test.go and cmd/gavel/test_framework_subcmds.go set testCmd.Flags().SetInterspersed(false), so any --flag token after a positional package path was treated as another path. With the action appending flags after user paths, every workflow that passed paths (e.g. `gavel test ./tests/e2e`) failed parsing with "starting path ./--no-progress does not exist". 2. action.yml unconditionally appended --show-passed, which is only defined on `gavel test` / `gavel lint`. Wrapping `gavel fixtures` (or any other subcommand) failed at flag parsing with "unknown flag: --show-passed". Fix both: - Flip SetInterspersed to true on `gavel test` and its framework subcommands. Cobra's `--` separator still terminates parsing for the documented `gavel test ./pkg -- --focus X` pass-through idiom. - Detect the first non-flag word in $GAVEL_ARGS in the action's run step and only append --show-passed when it is `test` or `lint`. --- action.yml | 20 +++++++++++++++++--- cmd/gavel/test.go | 6 +++++- cmd/gavel/test_framework_subcmds.go | 2 +- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/action.yml b/action.yml index b6265b9fc..6f3364c2e 100644 --- a/action.yml +++ b/action.yml @@ -122,10 +122,24 @@ runs: set +e rm -f "${JSON_FILE}" "${GAVEL_LOG_FILE}" + # --show-passed is only defined on `gavel test` / `gavel lint`. Other + # subcommands (e.g. `fixtures`) reject unknown flags and fail at parse + # time. Detect the first non-flag word in $GAVEL_ARGS and only append + # the test-specific flag for those subcommands. + first_subcmd="" + for tok in $GAVEL_ARGS; do + case "$tok" in + -*) ;; + *) first_subcmd="$tok"; break ;; + esac + done + extra_flags=("--no-progress" "--no-color" "--format" "json=${JSON_FILE}") + case "$first_subcmd" in + test|lint) extra_flags=("--show-passed" "${extra_flags[@]}") ;; + esac + # shellcheck disable=SC2086 - gavel $GAVEL_ARGS \ - --no-progress --no-color --show-passed \ - --format "json=${JSON_FILE}" \ + gavel $GAVEL_ARGS "${extra_flags[@]}" \ 2> >(tee "${GAVEL_LOG_FILE}" >&2) code=$? set -e diff --git a/cmd/gavel/test.go b/cmd/gavel/test.go index f0a3d449e..9b453bbf9 100644 --- a/cmd/gavel/test.go +++ b/cmd/gavel/test.go @@ -544,7 +544,11 @@ var testCmd *cobra.Command func init() { testCmd = clicky.AddNamedCommand("test", rootCmd, testrunner.RunOptions{}, runTests) - testCmd.Flags().SetInterspersed(false) + // Allow flags and positional package paths to interleave so callers (e.g. the + // flanksource/gavel composite action) can append flags after user-supplied + // paths. Use `--` to terminate flag parsing when forwarding flags to the + // underlying runner via the `gavel test ./pkg -- --focus X` idiom. + testCmd.Flags().SetInterspersed(true) testCmd.Flags().BoolVar(&testDurationFlags.Detach, "detach", false, "With --ui, fork a detached UI server and exit. The child serves until --auto-stop (default 30m) or --idle-timeout (default 5m) fires.") testCmd.Flags().DurationVar(&testDurationFlags.AutoStop, "auto-stop", 0, diff --git a/cmd/gavel/test_framework_subcmds.go b/cmd/gavel/test_framework_subcmds.go index 61a9c7c4c..8c7494a2a 100644 --- a/cmd/gavel/test_framework_subcmds.go +++ b/cmd/gavel/test_framework_subcmds.go @@ -24,7 +24,7 @@ func registerTestFrameworkSubcommands() { return runTests(opts) }) sub.Short = fmt.Sprintf("Run only %s tests", fw) - sub.Flags().SetInterspersed(false) + sub.Flags().SetInterspersed(true) if err := sub.Flags().MarkHidden("framework"); err != nil { panic(fmt.Sprintf("hide --framework on %s subcommand: %v", name, err)) } From 270c072b4af71e474e00aef6b8e379ec4b2e7428 Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Mon, 27 Apr 2026 06:24:09 +0300 Subject: [PATCH 2/5] feat(test): add --ignore and --no-gitignore to gavel test, plus build-tag-aware discovery Lets callers run `gavel test ./... --ignore ./bench --ignore ./hack` instead of enumerating every package they want included. Bare directory patterns are recursive (no `/...` suffix needed); `/...` and `/**` suffixes are also accepted for Go-style compatibility. `.gitignore` is honored by default during test discovery (was already the case via WalkGitIgnoredBounded); pass `--no-gitignore` to opt out. Discovery now also evaluates build constraints via go/build.MatchFile so test files behind `//go:build never` (or any tag the current build context does not satisfy) no longer cause their package to be surfaced as runnable. The check is permissive on errors (missing files etc.) to preserve existing behavior on edge cases. Implementation notes: - `Ignore []string` and `NoGitignore bool` added to RunOptions; clicky's reflective flag binding picks them up from struct tags, no manual cobra wiring needed. - Filter is applied in TestOrchestrator.discoverPackagesInPaths after the per-runner discovery returns, so all frameworks benefit. - `--no-gitignore` is threaded through utils via a process-global toggle (utils.SetGitignoreDisabled) because the Runner interface does not pass per-call options into discovery; acceptable for a single-shot CLI. --- testrunner/path_filtering.go | 74 ++++++++++++++++++++ testrunner/path_filtering_unit_test.go | 94 ++++++++++++++++++++++++++ testrunner/runner.go | 17 ++++- testrunner/runners/buildtags.go | 27 ++++++++ testrunner/runners/buildtags_test.go | 59 ++++++++++++++++ testrunner/runners/ginkgo.go | 4 +- testrunner/runners/gotest.go | 13 +++- utils/walk.go | 29 +++++++- utils/walk_test.go | 63 +++++++++++++++++ 9 files changed, 373 insertions(+), 7 deletions(-) create mode 100644 testrunner/path_filtering.go create mode 100644 testrunner/path_filtering_unit_test.go create mode 100644 testrunner/runners/buildtags.go create mode 100644 testrunner/runners/buildtags_test.go diff --git a/testrunner/path_filtering.go b/testrunner/path_filtering.go new file mode 100644 index 000000000..16d689328 --- /dev/null +++ b/testrunner/path_filtering.go @@ -0,0 +1,74 @@ +package testrunner + +import ( + "path/filepath" + "strings" +) + +// applyIgnorePatterns returns the subset of pkgs that does not match any +// pattern in ignore. Patterns are repo-relative package paths. A bare +// directory pattern matches that directory and every package below it +// ("./bench" hides "./bench" and "./bench/sub"). Patterns may also use the +// Go-style "./bench/..." or "./bench/**" suffixes; both are treated as +// recursive directory matches. Empty ignore returns pkgs unchanged. +func applyIgnorePatterns(pkgs, ignore []string) []string { + if len(ignore) == 0 || len(pkgs) == 0 { + return pkgs + } + normalized := make([]string, 0, len(ignore)) + for _, p := range ignore { + if n := normalizeIgnorePattern(p); n != "" { + normalized = append(normalized, n) + } + } + if len(normalized) == 0 { + return pkgs + } + out := pkgs[:0:0] + for _, pkg := range pkgs { + if !ignoreMatches(pkg, normalized) { + out = append(out, pkg) + } + } + return out +} + +// normalizeIgnorePattern strips trailing /... and /** suffixes and collapses +// the path through filepath.Clean. The returned string is a directory +// prefix used as both an exact match and a path-prefix match. +func normalizeIgnorePattern(p string) string { + p = strings.TrimSpace(p) + if p == "" { + return "" + } + for _, suffix := range []string{"/...", "/**", "/*"} { + p = strings.TrimSuffix(p, suffix) + } + p = filepath.ToSlash(filepath.Clean(p)) + if p == "." { + return "" + } + if !strings.HasPrefix(p, "./") && !filepath.IsAbs(p) { + p = "./" + p + } + return p +} + +// ignoreMatches reports whether pkg matches any normalized ignore prefix. +// pkg is expected to look like "./foo/bar" (from getRelativePath). Match is +// either equality or a path-prefix check ("./bench" matches "./bench/sub"). +func ignoreMatches(pkg string, normalized []string) bool { + clean := filepath.ToSlash(filepath.Clean(pkg)) + if !strings.HasPrefix(clean, "./") && !filepath.IsAbs(clean) { + clean = "./" + clean + } + for _, pat := range normalized { + if clean == pat { + return true + } + if strings.HasPrefix(clean, pat+"/") { + return true + } + } + return false +} diff --git a/testrunner/path_filtering_unit_test.go b/testrunner/path_filtering_unit_test.go new file mode 100644 index 000000000..3b10cfc79 --- /dev/null +++ b/testrunner/path_filtering_unit_test.go @@ -0,0 +1,94 @@ +package testrunner + +import ( + "reflect" + "testing" +) + +func TestApplyIgnorePatterns(t *testing.T) { + tests := []struct { + name string + pkgs []string + ignore []string + want []string + }{ + { + name: "empty ignore returns input unchanged", + pkgs: []string{"./api", "./bench", "./bench/sub"}, + ignore: nil, + want: []string{"./api", "./bench", "./bench/sub"}, + }, + { + name: "bare directory ignore is recursive", + pkgs: []string{"./api", "./bench", "./bench/sub", "./bench/deep/inner"}, + ignore: []string{"./bench"}, + want: []string{"./api"}, + }, + { + name: "trailing /... is treated the same as bare dir", + pkgs: []string{"./api", "./hack", "./hack/foo"}, + ignore: []string{"./hack/..."}, + want: []string{"./api"}, + }, + { + name: "trailing /** is treated the same as bare dir", + pkgs: []string{"./api", "./hack", "./hack/foo"}, + ignore: []string{"./hack/**"}, + want: []string{"./api"}, + }, + { + name: "non-matching pattern is a no-op", + pkgs: []string{"./api", "./db"}, + ignore: []string{"./does-not-exist"}, + want: []string{"./api", "./db"}, + }, + { + name: "multiple patterns compose", + pkgs: []string{"./api", "./bench", "./hack/x", "./specs", "./tests/e2e", "./tests/unit"}, + ignore: []string{"./bench", "./hack", "./specs", "./tests/e2e"}, + want: []string{"./api", "./tests/unit"}, + }, + { + name: "exact-match without recursion does not over-match siblings", + pkgs: []string{"./bench", "./benchmarks"}, + ignore: []string{"./bench"}, + want: []string{"./benchmarks"}, + }, + { + name: "leading ./ is normalized when missing", + pkgs: []string{"./bench", "./api"}, + ignore: []string{"bench"}, + want: []string{"./api"}, + }, + { + name: "blank pattern is ignored", + pkgs: []string{"./api"}, + ignore: []string{" "}, + want: []string{"./api"}, + }, + { + name: "root pattern is rejected (would match everything)", + pkgs: []string{"./api", "./db"}, + ignore: []string{"."}, + want: []string{"./api", "./db"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := applyIgnorePatterns(tt.pkgs, tt.ignore) + // applyIgnorePatterns may return a nil slice for fully filtered input; + // normalize for comparison. + if got == nil { + got = []string{} + } + want := tt.want + if want == nil { + want = []string{} + } + if !reflect.DeepEqual(got, want) { + t.Errorf("applyIgnorePatterns(%v, %v) = %v, want %v", tt.pkgs, tt.ignore, got, want) + } + }) + } +} diff --git a/testrunner/runner.go b/testrunner/runner.go index d02fa46d9..1367dd418 100644 --- a/testrunner/runner.go +++ b/testrunner/runner.go @@ -105,6 +105,8 @@ type TestOrchestrator struct { type RunOptions struct { SyncTodos bool `json:"sync_todos,omitempty" flag:"sync-todos"` // Whether to sync test failures to TODOs StartingPaths []string `json:"starting_paths,omitempty" args:"true"` // Package paths to test (e.g., ["./pkg/testrunner"]). If empty, all packages are discovered. + Ignore []string `json:"ignore,omitempty" flag:"ignore"` // Package paths to exclude from discovery. Bare directories are recursive (e.g. ./bench excludes ./bench and ./bench/sub). Trailing /... is also accepted. + NoGitignore bool `json:"no_gitignore,omitempty" flag:"no-gitignore"` // Disable .gitignore-based pruning during package discovery. By default gitignored directories are skipped. ExtraArgs []string `json:"extra_args,omitempty" flag:"extra-args"` // Additional arguments to pass to test runners (e.g., ["--focus", "TestName"]) ShowPassed bool `json:"show_passed,omitempty" flag:"show-passed"` // Whether to show passed tests in output ShowStdout OutputMode `json:"show_stdout,omitempty" flag:"show-stdout" default:"OnFailure"` // When to show stdout: false|Never, OnFailure (default), true|Always @@ -147,6 +149,12 @@ func (opts RunOptions) Pretty() api.Text { if len(opts.StartingPaths) > 0 { text = text.Space().Append("StartingPaths: ", "text-muted").Append(clicky.CompactList(opts.StartingPaths), "text-blue-500") } + if len(opts.Ignore) > 0 { + text = text.Space().Append("Ignore: ", "text-muted").Append(clicky.CompactList(opts.Ignore), "text-blue-500") + } + if opts.NoGitignore { + text = text.Space().Append("NoGitignore: ", "text-muted").Append(icons.Check, "text-green-500") + } if len(opts.ExtraArgs) > 0 { text = text.Space().Append("ExtraArgs: ", "text-muted").Append(clicky.CompactList(opts.ExtraArgs), "text-blue-500") } @@ -381,6 +389,13 @@ func Run(opts RunOptions) (any, error) { opts.WorkDir, _ = os.Getwd() } + // --no-gitignore: short-circuit gitignore-aware walking process-wide. The + // flag is package-global because the Runner interface does not thread + // per-call options through to discovery. Acceptable for a single-shot + // CLI process. + prevDisable := utils.SetGitignoreDisabled(opts.NoGitignore) + defer utils.SetGitignoreDisabled(prevDisable) + // Split starting paths by execution root so each group runs with the // correct WorkDir. Nested Go modules get their own groups. groups, err := groupPathsByGitRoot(opts.WorkDir, opts.StartingPaths) @@ -1213,7 +1228,7 @@ func (o *TestOrchestrator) discoverPackagesInPaths(runner runners.Runner, starti } - return lo.Uniq(allPackages), nil + return applyIgnorePatterns(lo.Uniq(allPackages), o.Ignore), nil } // parseTestResults parses test results from either stdout (go test) or report file (Ginkgo). diff --git a/testrunner/runners/buildtags.go b/testrunner/runners/buildtags.go new file mode 100644 index 000000000..c4440655e --- /dev/null +++ b/testrunner/runners/buildtags.go @@ -0,0 +1,27 @@ +package runners + +import ( + "go/build" + "path/filepath" +) + +// matchesBuildContext reports whether file (a single _test.go path) is +// compiled under the default Go build context (current GOOS/GOARCH and the +// caller's build tags). Files excluded by //go:build constraints return +// false; matching, malformed, or unreadable files return true so we keep +// the previous permissive behavior on edge cases. +// +// Used by package discovery so directories whose only test files are +// excluded by build constraints (e.g. //go:build never) are not surfaced +// as runnable packages. +func matchesBuildContext(file string) bool { + dir, name := filepath.Split(file) + if dir == "" { + dir = "." + } + matched, err := build.Default.MatchFile(dir, name) + if err != nil { + return true + } + return matched +} diff --git a/testrunner/runners/buildtags_test.go b/testrunner/runners/buildtags_test.go new file mode 100644 index 000000000..b8601f92b --- /dev/null +++ b/testrunner/runners/buildtags_test.go @@ -0,0 +1,59 @@ +package runners + +import ( + "os" + "path/filepath" + "testing" +) + +func TestMatchesBuildContext(t *testing.T) { + dir := t.TempDir() + + cases := []struct { + name string + filename string + body string + want bool + }{ + { + name: "plain test file matches", + filename: "plain_test.go", + body: "package x\n", + want: true, + }, + { + name: "//go:build never excludes the file", + filename: "never_test.go", + body: "//go:build never\n\npackage x\n", + want: false, + }, + { + name: "//go:build linux on darwin runners would not match — use a tag we do not pass", + filename: "tagged_test.go", + body: "//go:build buildtag_that_definitely_isnt_set\n\npackage x\n", + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := filepath.Join(dir, tc.filename) + if err := os.WriteFile(path, []byte(tc.body), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + got := matchesBuildContext(path) + if got != tc.want { + t.Errorf("matchesBuildContext(%s) = %v, want %v", tc.filename, got, tc.want) + } + }) + } +} + +func TestMatchesBuildContextOnUnreadableReturnsTrue(t *testing.T) { + // Nonexistent file: MatchFile errors out. We treat error as "permissive" + // so the file isn't silently skipped on edge cases. + got := matchesBuildContext("/does/not/exist/foo_test.go") + if !got { + t.Errorf("matchesBuildContext on missing file = false, want true (permissive default)") + } +} diff --git a/testrunner/runners/ginkgo.go b/testrunner/runners/ginkgo.go index 8547abfe6..4c18a4b22 100644 --- a/testrunner/runners/ginkgo.go +++ b/testrunner/runners/ginkgo.go @@ -48,7 +48,7 @@ func (r *Ginkgo) Detect(workDir string) (bool, error) { if err != nil { return err } - if !d.IsDir() && strings.HasSuffix(d.Name(), "_test.go") && hasGinkgoImports(path) { + if !d.IsDir() && strings.HasSuffix(d.Name(), "_test.go") && matchesBuildContext(path) && hasGinkgoImports(path) { return errGinkgoDetected } return nil @@ -81,7 +81,7 @@ func (r *Ginkgo) DiscoverPackages(workDir string, recursive bool) ([]string, err return err } - if !d.IsDir() && strings.HasSuffix(d.Name(), "_test.go") { + if !d.IsDir() && strings.HasSuffix(d.Name(), "_test.go") && matchesBuildContext(path) { if hasGinkgoImports(path) { pkgDir := filepath.Dir(path) if !seen[pkgDir] { diff --git a/testrunner/runners/gotest.go b/testrunner/runners/gotest.go index 1e919c61e..4e623da27 100644 --- a/testrunner/runners/gotest.go +++ b/testrunner/runners/gotest.go @@ -56,7 +56,7 @@ func (r *GoTest) Detect(workDir string) (bool, error) { if d.IsDir() { return nil } - if strings.HasSuffix(d.Name(), "_test.go") { + if strings.HasSuffix(d.Name(), "_test.go") && matchesBuildContext(path) { return errGoTestDetected } return nil @@ -81,6 +81,9 @@ func (r *GoTest) packageHasNonGinkgoTests(pkgDir string) bool { for _, entry := range entries { if !entry.IsDir() && strings.HasSuffix(entry.Name(), "_test.go") { path := filepath.Join(pkgDir, entry.Name()) + if !matchesBuildContext(path) { + continue + } if !hasGinkgoImports(path) { return true } @@ -103,6 +106,9 @@ func (r *GoTest) inspectPackage(pkgDir string) (hasTests bool, hasBench bool) { continue } path := filepath.Join(pkgDir, entry.Name()) + if !matchesBuildContext(path) { + continue + } if hasGinkgoImports(path) { continue } @@ -162,6 +168,9 @@ func (r *GoTest) hasTestsBeyondTestMain(pkgDir string) bool { continue } path := filepath.Join(pkgDir, entry.Name()) + if !matchesBuildContext(path) { + continue + } if hasGinkgoImports(path) { continue } @@ -198,7 +207,7 @@ func (r *GoTest) DiscoverPackages(workDir string, recursive bool) ([]string, err return err } - if !d.IsDir() && strings.HasSuffix(d.Name(), "_test.go") { + if !d.IsDir() && strings.HasSuffix(d.Name(), "_test.go") && matchesBuildContext(path) { pkgDir := filepath.Dir(path) if !seen[pkgDir] { seen[pkgDir] = true diff --git a/utils/walk.go b/utils/walk.go index b8c4eebb3..b18b540cd 100644 --- a/utils/walk.go +++ b/utils/walk.go @@ -13,6 +13,27 @@ import ( type walkStopFn func(root, path string, d fs.DirEntry) (bool, error) +// disableGitignoreWalk, when true, makes WalkGitIgnored / WalkGitIgnoredBounded +// behave like a plain filepath.WalkDir (still skipping .git and respecting the +// stopAtNestedProjectRoot bound, but ignoring .gitignore patterns). +// +// This is set at most once per CLI invocation from the testrunner orchestrator +// when the user passes --no-gitignore. It is process-global because the Runner +// interface threads no per-call options through to discovery, and propagating +// one would ripple through every runner implementation. Acceptable trade-off +// for a CLI binary. +var disableGitignoreWalk bool + +// SetGitignoreDisabled toggles the global gitignore-aware walking flag. +// Pass true to make subsequent WalkGitIgnored / WalkGitIgnoredBounded calls +// skip .gitignore filtering. Returns the previous value so callers can +// restore it (useful in tests). +func SetGitignoreDisabled(v bool) bool { + prev := disableGitignoreWalk + disableGitignoreWalk = v + return prev +} + func FindGitRoot(dir string) string { dir, _ = filepath.Abs(dir) for { @@ -135,7 +156,7 @@ func WalkGitIgnoredBounded(root string, fn fs.WalkDirFunc, allowList ...string) func walkGitIgnored(root string, fn fs.WalkDirFunc, stop walkStopFn, allowList ...string) error { root, _ = filepath.Abs(root) gitRoot := FindGitRoot(root) - if gitRoot == "" { + if gitRoot == "" || disableGitignoreWalk { return filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { if err != nil { return fn(path, d, err) @@ -243,8 +264,12 @@ func stopAtNestedProjectRoot(root, path string, d fs.DirEntry) (bool, error) { // FilterGitIgnored returns the subset of absolute paths that are not matched // by .gitignore patterns. Uses go-git's ReadPatterns to recursively load all -// .gitignore files. If no git root is found, all paths are returned. +// .gitignore files. If no git root is found or the global disable flag is +// set (see SetGitignoreDisabled), all paths are returned. func FilterGitIgnored(paths []string, dir string) []string { + if disableGitignoreWalk { + return paths + } dir, _ = filepath.Abs(dir) gitRoot := FindGitRoot(dir) if gitRoot == "" { diff --git a/utils/walk_test.go b/utils/walk_test.go index c942d5e63..b504c33ab 100644 --- a/utils/walk_test.go +++ b/utils/walk_test.go @@ -381,3 +381,66 @@ var _ = Describe("FilterGitIgnored", func() { Expect(result).To(BeEmpty()) }) }) + +var _ = Describe("SetGitignoreDisabled", func() { + var root string + + BeforeEach(func() { + root = GinkgoT().TempDir() + }) + + AfterEach(func() { + // Always restore the default; test isolation depends on this. + SetGitignoreDisabled(false) + }) + + It("WalkGitIgnored honors gitignored paths when enabled (default)", func() { + setupGitRepo(root) + os.WriteFile(filepath.Join(root, ".gitignore"), []byte("ignored/\n"), 0644) + os.MkdirAll(filepath.Join(root, "ignored"), 0755) + os.WriteFile(filepath.Join(root, "ignored", "x.go"), nil, 0644) + os.WriteFile(filepath.Join(root, "main.go"), nil, 0644) + + paths, err := collectPaths(root) + Expect(err).NotTo(HaveOccurred()) + Expect(paths).To(ContainElement("main.go")) + Expect(paths).NotTo(ContainElement("ignored")) + }) + + It("WalkGitIgnored ignores .gitignore once disabled", func() { + setupGitRepo(root) + os.WriteFile(filepath.Join(root, ".gitignore"), []byte("ignored/\n"), 0644) + os.MkdirAll(filepath.Join(root, "ignored"), 0755) + os.WriteFile(filepath.Join(root, "ignored", "x.go"), nil, 0644) + os.WriteFile(filepath.Join(root, "main.go"), nil, 0644) + + SetGitignoreDisabled(true) + + paths, err := collectPaths(root) + Expect(err).NotTo(HaveOccurred()) + Expect(paths).To(ContainElement("main.go")) + Expect(paths).To(ContainElement("ignored")) + Expect(paths).To(ContainElement("ignored/x.go")) + }) + + It("FilterGitIgnored is bypassed once disabled", func() { + setupGitRepo(root) + os.WriteFile(filepath.Join(root, ".gitignore"), []byte("*.log\n"), 0644) + + paths := []string{ + filepath.Join(root, "main.go"), + filepath.Join(root, "debug.log"), + } + + SetGitignoreDisabled(true) + result := FilterGitIgnored(paths, root) + Expect(result).To(ConsistOf(paths)) + }) + + It("returns the previous value so callers can restore it", func() { + prev := SetGitignoreDisabled(true) + Expect(prev).To(BeFalse()) + prev2 := SetGitignoreDisabled(false) + Expect(prev2).To(BeTrue()) + }) +}) From 09a82c27f236f26fea46c723d0dc9eef760b7d4f Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Mon, 27 Apr 2026 06:30:30 +0300 Subject: [PATCH 3/5] feat(test): expand ./... and apply --ignore across nested-module groups Two follow-ups so duty's `./... --ignore ./hack` actually works: 1. Recognise Go-style recursive wildcards as starting paths. `./...`, `...`, `.`, and `./` now resolve to "discover from WorkDir" (the same code path used when no positional args are given), and any `.//...` is rewritten to `./` (DiscoverPackages already walks recursively). Previously gavel rejected `./...` with "starting path does not exist". 2. Apply --ignore to nested-module testGroups. expandNestedModuleGroups() spawns a separate group per child go.mod (e.g. ./hack/migrate, ./hack/generate-schemas), each running with its own WorkDir, so per-package filtering inside discoverPackagesInPaths never saw them. Drop ignored groups before runMultiRoot fans out. Also extends path_filtering_unit_test.go with TestExpandRecursiveWildcards and TestFilterIgnoredGroups. --- testrunner/path_filtering.go | 78 ++++++++++++++++++++++++ testrunner/path_filtering_unit_test.go | 82 ++++++++++++++++++++++++++ testrunner/runner.go | 7 +++ 3 files changed, 167 insertions(+) diff --git a/testrunner/path_filtering.go b/testrunner/path_filtering.go index 16d689328..a12c25e28 100644 --- a/testrunner/path_filtering.go +++ b/testrunner/path_filtering.go @@ -5,6 +5,84 @@ import ( "strings" ) +// filterIgnoredGroups drops testGroups whose workDir is matched by an +// ignore pattern relative to baseWorkDir. This is needed because nested-go-mod +// expansion spawns independent groups (each with its own WorkDir) before +// per-package discovery happens, so package-level filtering would never see +// them. baseWorkDir is the original (top-level) workdir; patterns are +// resolved against it. +func filterIgnoredGroups(baseWorkDir string, groups []testGroup, ignore []string) []testGroup { + if len(ignore) == 0 || len(groups) == 0 { + return groups + } + normalized := make([]string, 0, len(ignore)) + for _, p := range ignore { + if n := normalizeIgnorePattern(p); n != "" { + normalized = append(normalized, n) + } + } + if len(normalized) == 0 { + return groups + } + baseAbs, err := filepath.Abs(baseWorkDir) + if err != nil { + return groups + } + out := groups[:0:0] + for _, g := range groups { + gAbs, err := filepath.Abs(g.workDir) + if err != nil { + out = append(out, g) + continue + } + rel, err := filepath.Rel(baseAbs, gAbs) + if err != nil || strings.HasPrefix(rel, "..") { + // Group lives outside the base workdir — leave it alone. + out = append(out, g) + continue + } + if rel == "." { + out = append(out, g) + continue + } + pkgLike := "./" + filepath.ToSlash(rel) + if !ignoreMatches(pkgLike, normalized) { + out = append(out, g) + } + } + return out +} + +// expandRecursiveWildcards translates Go-style recursive wildcards +// ("./...", "...") into the empty starting-path convention, which downstream +// code already interprets as "discover from WorkDir recursively". Any path +// of the form ".//..." is rewritten to "./" so the directory is +// preserved but the wildcard suffix is dropped (DiscoverPackages walks +// recursively by default). +// +// Returns a new slice; input is not mutated. An input that contains only +// "./..." (or equivalents) becomes nil. +func expandRecursiveWildcards(paths []string) []string { + if len(paths) == 0 { + return paths + } + out := make([]string, 0, len(paths)) + for _, p := range paths { + switch p { + case "./...", "...", "./", ".": + // Equivalent to "no starting path" — let the orchestrator + // discover from WorkDir. + continue + } + p = strings.TrimSuffix(p, "/...") + out = append(out, p) + } + if len(out) == 0 { + return nil + } + return out +} + // applyIgnorePatterns returns the subset of pkgs that does not match any // pattern in ignore. Patterns are repo-relative package paths. A bare // directory pattern matches that directory and every package below it diff --git a/testrunner/path_filtering_unit_test.go b/testrunner/path_filtering_unit_test.go index 3b10cfc79..dbda92149 100644 --- a/testrunner/path_filtering_unit_test.go +++ b/testrunner/path_filtering_unit_test.go @@ -1,6 +1,7 @@ package testrunner import ( + "path/filepath" "reflect" "testing" ) @@ -92,3 +93,84 @@ func TestApplyIgnorePatterns(t *testing.T) { }) } } + +func TestExpandRecursiveWildcards(t *testing.T) { + tests := []struct { + name string + in []string + want []string + }{ + {name: "empty input", in: nil, want: nil}, + {name: "./... becomes empty (whole-workdir discovery)", in: []string{"./..."}, want: nil}, + {name: "... becomes empty", in: []string{"..."}, want: nil}, + {name: ". becomes empty", in: []string{"."}, want: nil}, + { + name: "./pkg/... is preserved without the suffix", + in: []string{"./pkg/..."}, + want: []string{"./pkg"}, + }, + { + name: "mixed wildcards and concrete paths", + in: []string{"./...", "./api", "./pkg/..."}, + want: []string{"./api", "./pkg"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := expandRecursiveWildcards(tt.in) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("expandRecursiveWildcards(%v) = %v, want %v", tt.in, got, tt.want) + } + }) + } +} + +func TestFilterIgnoredGroups(t *testing.T) { + base := filepath.FromSlash("/repo") + groups := []testGroup{ + {workDir: filepath.FromSlash("/repo")}, + {workDir: filepath.FromSlash("/repo/hack/migrate")}, + {workDir: filepath.FromSlash("/repo/hack/generate-schemas")}, + {workDir: filepath.FromSlash("/repo/sub")}, + } + + tests := []struct { + name string + ignore []string + wantDirs []string + }{ + { + name: "no ignore returns all", + ignore: nil, + wantDirs: []string{"/repo", "/repo/hack/migrate", "/repo/hack/generate-schemas", "/repo/sub"}, + }, + { + name: "bare dir ignore strips nested-module groups", + ignore: []string{"./hack"}, + wantDirs: []string{"/repo", "/repo/sub"}, + }, + { + name: "ignore that matches the base does not strip the base group", + ignore: []string{"./sub"}, + wantDirs: []string{"/repo", "/repo/hack/migrate", "/repo/hack/generate-schemas"}, + }, + { + name: "non-matching ignore is a no-op", + ignore: []string{"./does-not-exist"}, + wantDirs: []string{"/repo", "/repo/hack/migrate", "/repo/hack/generate-schemas", "/repo/sub"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := filterIgnoredGroups(base, groups, tt.ignore) + gotDirs := make([]string, 0, len(got)) + for _, g := range got { + gotDirs = append(gotDirs, filepath.ToSlash(g.workDir)) + } + if !reflect.DeepEqual(gotDirs, tt.wantDirs) { + t.Errorf("filterIgnoredGroups dirs = %v, want %v", gotDirs, tt.wantDirs) + } + }) + } +} diff --git a/testrunner/runner.go b/testrunner/runner.go index 1367dd418..fe84e6f12 100644 --- a/testrunner/runner.go +++ b/testrunner/runner.go @@ -253,6 +253,8 @@ type testGroup struct { func groupPathsByGitRoot(workDir string, startingPaths []string) ([]testGroup, error) { workDir, _ = filepath.Abs(workDir) + startingPaths = expandRecursiveWildcards(startingPaths) + if len(startingPaths) == 0 { return expandNestedModuleGroups([]testGroup{{workDir: workDir}}) } @@ -402,6 +404,10 @@ func Run(opts RunOptions) (any, error) { if err != nil { return nil, err } + groups = filterIgnoredGroups(opts.WorkDir, groups, opts.Ignore) + if len(groups) == 0 { + return parsers.TestSuiteResults{}, nil + } if len(groups) > 1 { return runMultiRoot(opts, groups) } @@ -677,6 +683,7 @@ func (o *TestOrchestrator) detectAndRun(frameworks []Framework, startingPaths [] packages, err = o.discoverPackagesInPaths(runner, startingPaths) } else { packages, err = runner.DiscoverPackages(o.WorkDir, o.Recursive) + packages = applyIgnorePatterns(packages, o.Ignore) } if err != nil { From c28dd9ab16e572e70695eaeec088712d3aa7e68e Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Mon, 27 Apr 2026 07:14:28 +0300 Subject: [PATCH 4/5] feat(gavel): support multiple gavel artifacts per PR and collapse all-passing sources Add discovery and merging of multiple gavel-* artifacts from different workflow runs or matrix shards. When a PR has multiple test jobs, each now appears as a separate entry in the results UI with per-job breakdowns, while aggregate counts roll up to the PR level. Also optimize the summary table to hide sources with 100% passing tests, collapsing them into a single "N more passing source(s)" row. This keeps PR comments short for the common "all green" case while still showing failing/skipped sources prominently. Key changes: - Add GavelArtifact struct and ListRunArtifacts/FindGavelArtifacts to discover artifacts via Actions API - Refactor DownloadArtifact to DownloadArtifactFiles for multi-file support - Add mergeGavelResults to concatenate test/lint/bench data from multiple payloads - Add GavelJobSummary for per-job breakdown in UI - Update summary table generation to filter and collapse all-passing sources - Add comprehensive tests for artifact discovery, merging, and table generation BREAKING CHANGE: DownloadArtifact now returns the first .json file only; use DownloadArtifactFiles for multi-file artifacts. --- cmd/gavel/summary.go | 28 ++++- cmd/gavel/summary_test.go | 104 +++++++++++++++- github/artifacts.go | 183 +++++++++++++++++++++++++-- github/artifacts_test.go | 158 ++++++++++++++++++++++++ pr/ui/gavel_results.go | 240 +++++++++++++++++++++++++++++++++--- pr/ui/gavel_results_test.go | 124 +++++++++++++++++++ pr/ui/handler.go | 47 +++++-- 7 files changed, 839 insertions(+), 45 deletions(-) diff --git a/cmd/gavel/summary.go b/cmd/gavel/summary.go index 5536ce53e..7fe66344d 100644 --- a/cmd/gavel/summary.go +++ b/cmd/gavel/summary.go @@ -231,9 +231,25 @@ func ensureSource(sources map[string]*sourceCounts, name string) *sourceCounts { return sc } +// writeCountsTable emits a markdown table containing only sources that have +// failures or skips. Sources with 100% passing tests are intentionally +// omitted — their pass count and duration are rolled into the totals row +// emitted by writeTotals so the PR comment stays short for the common +// "all green" case. When every source is clean, the table is omitted +// entirely and writeTotals carries the headline. func writeCountsTable(b *strings.Builder, sources map[string]*sourceCounts) { - rows := make([]*sourceCounts, 0, len(sources)) + var rows []*sourceCounts + var passingOnly, passingDuration int + var hiddenSources int + totalPassing := 0 for _, sc := range sources { + totalPassing += sc.passed + if sc.failed == 0 && sc.skipped == 0 { + passingOnly += sc.passed + passingDuration += int(sc.duration) + hiddenSources++ + continue + } rows = append(rows, sc) } sort.Slice(rows, func(i, j int) bool { @@ -245,12 +261,22 @@ func writeCountsTable(b *strings.Builder, sources map[string]*sourceCounts) { }) b.WriteString("## Gavel summary\n\n") + if len(rows) == 0 { + // Nothing to break out — totals row alone tells the story. + return + } b.WriteString("| Source | Pass | Fail | Skip | Duration |\n") b.WriteString("|---|---:|---:|---:|---:|\n") for _, sc := range rows { fmt.Fprintf(b, "| %s | %d | %d | %d | %s |\n", escapePipe(sc.name), sc.passed, sc.failed, sc.skipped, formatDuration(sc.duration)) } + if hiddenSources > 0 { + // One collapsed row covers every all-passing source so a reader + // can still see "we ran more than what's listed". + fmt.Fprintf(b, "| _%d more passing source(s)_ | %d | 0 | 0 | %s |\n", + hiddenSources, passingOnly, formatDuration(time.Duration(passingDuration))) + } b.WriteString("\n") } diff --git a/cmd/gavel/summary_test.go b/cmd/gavel/summary_test.go index 4996ec8c6..d921a22cd 100644 --- a/cmd/gavel/summary_test.go +++ b/cmd/gavel/summary_test.go @@ -84,16 +84,30 @@ func TestBuildCompactSummary(t *testing.T) { out := buildCompactSummary(input, compactSummaryBudget{maxFailures: 5, maxLinesPerFailure: 5, maxCharsPerLine: 200}) - // Counts table by source — expect one row per test package AND one per linter. + // Counts table by source — only sources that have failures or skips + // should appear. The serve package has failures, the verify package + // has a skipped test, and golangci-lint reports a violation, so all + // three keep their row. Sources that are 100% passing (e.g. the + // clean gofmt linter below) are folded into the passing-only summary + // row + Totals line. if !strings.Contains(out, "github.com/flanksource/gavel/serve") { t.Errorf("expected serve package row in counts table, got:\n%s", out) } if !strings.Contains(out, "github.com/flanksource/gavel/verify") { - t.Errorf("expected verify package row in counts table, got:\n%s", out) + t.Errorf("expected verify package row (has a skipped test) in counts table, got:\n%s", out) } if !strings.Contains(out, "golangci-lint") { t.Errorf("expected golangci-lint row in counts table, got:\n%s", out) } + // gofmt is a passing-only linter — must not appear as its own row. + if strings.Contains(out, "| lint: gofmt |") { + t.Errorf("passing-only linter must be folded into the passing summary row, got:\n%s", out) + } + // The collapsed-passing row should mention the gofmt linter's pass count + // (1) and the "more passing source(s)" text. + if !strings.Contains(out, "more passing source(s)") { + t.Errorf("expected collapsed passing-source row, got:\n%s", out) + } // Passing rows present, but no per-test listing for passing tests in the detail section. if strings.Contains(out, "initializes a bare git repo") { @@ -280,8 +294,11 @@ func TestBuildCompactSummaryPrefersRealResultsOverCrashField(t *testing.T) { Error: "partial error that should be ignored", } out := buildCompactSummary(input, compactSummaryBudget{maxFailures: 5, maxLinesPerFailure: 5, maxCharsPerLine: 200}) - if !strings.Contains(out, "pkg/x") { - t.Errorf("expected normal results to render, got:\n%s", out) + // pkg/x is 100% passing so its row is collapsed into the totals; we + // only need to confirm the normal counts path ran (Totals line) and + // that the crash-stub path did not render. + if !strings.Contains(out, "**Totals:** 1 passed") { + t.Errorf("expected normal Totals line for the single passing test, got:\n%s", out) } if strings.Contains(out, "Gavel crashed") { t.Errorf("normal results must not fall through to crash path, got:\n%s", out) @@ -329,6 +346,85 @@ func TestRunSummaryReadsJSONFile(t *testing.T) { } } +// TestBuildCompactSummaryCollapsesAllPassingSources exercises the +// "PR comment must stay short" behaviour: when every source is 100% +// passing, the per-source table disappears entirely and the Totals +// line carries the full headline (count + duration). Regressing to +// the old behaviour (one row per package) blows up PR comments on +// large monorepos. +func TestBuildCompactSummaryCollapsesAllPassingSources(t *testing.T) { + input := gavelResultJSON{ + Tests: []parsers.Test{ + {Package: "pkg/a", Children: parsers.Tests{ + {Package: "pkg/a", Name: "TestOne", Passed: true, Duration: 10 * time.Millisecond}, + {Package: "pkg/a", Name: "TestTwo", Passed: true, Duration: 20 * time.Millisecond}, + }}, + {Package: "pkg/b", Children: parsers.Tests{ + {Package: "pkg/b", Name: "TestThree", Passed: true, Duration: 30 * time.Millisecond}, + }}, + }, + } + out := buildCompactSummary(input, compactSummaryBudget{maxFailures: 5, maxLinesPerFailure: 5, maxCharsPerLine: 200}) + + // No per-package rows when every source is clean. + if strings.Contains(out, "| pkg/a |") { + t.Errorf("all-passing pkg/a must not appear as its own row, got:\n%s", out) + } + if strings.Contains(out, "| pkg/b |") { + t.Errorf("all-passing pkg/b must not appear as its own row, got:\n%s", out) + } + // No table headers either — the table itself collapses. + if strings.Contains(out, "| Source |") { + t.Errorf("counts table must collapse when every source is clean, got:\n%s", out) + } + // Totals line still carries the headline. + if !strings.Contains(out, "**Totals:** 3 passed · 0 failed · 0 skipped") { + t.Errorf("expected Totals line with aggregate counts, got:\n%s", out) + } +} + +// TestBuildCompactSummaryCollapsesPassingAlongsideFailing checks the +// mixed case: failing sources keep their own row, but every all-passing +// source is folded into a single trailing summary row whose pass count +// matches the sum of the hidden sources. +func TestBuildCompactSummaryCollapsesPassingAlongsideFailing(t *testing.T) { + input := gavelResultJSON{ + Tests: []parsers.Test{ + {Package: "pkg/fail", Children: parsers.Tests{ + {Package: "pkg/fail", Name: "TestBoom", Failed: true, Message: "boom"}, + }}, + {Package: "pkg/clean1", Children: parsers.Tests{ + {Package: "pkg/clean1", Name: "TestOne", Passed: true, Duration: 100 * time.Millisecond}, + {Package: "pkg/clean1", Name: "TestTwo", Passed: true, Duration: 100 * time.Millisecond}, + }}, + {Package: "pkg/clean2", Children: parsers.Tests{ + {Package: "pkg/clean2", Name: "TestThree", Passed: true, Duration: 50 * time.Millisecond}, + }}, + }, + } + out := buildCompactSummary(input, compactSummaryBudget{maxFailures: 5, maxLinesPerFailure: 5, maxCharsPerLine: 200}) + + // Failing source keeps its dedicated row. + if !strings.Contains(out, "| pkg/fail |") { + t.Errorf("failing pkg/fail row must be present, got:\n%s", out) + } + // Clean sources do not appear by name… + if strings.Contains(out, "pkg/clean1") { + t.Errorf("clean pkg/clean1 must not appear by name, got:\n%s", out) + } + if strings.Contains(out, "pkg/clean2") { + t.Errorf("clean pkg/clean2 must not appear by name, got:\n%s", out) + } + // …but a single collapsed row reports the count of hidden sources + // AND the sum of their pass counts (2 + 1 = 3). + if !strings.Contains(out, "_2 more passing source(s)_") { + t.Errorf("expected '2 more passing source(s)' collapse row, got:\n%s", out) + } + if !strings.Contains(out, "| _2 more passing source(s)_ | 3 | 0 | 0 |") { + t.Errorf("expected collapsed row with summed pass count, got:\n%s", out) + } +} + func longMultilineStderr() string { var sb strings.Builder // 8 lines of 250 chars each — must be truncated to 5 lines of ≤200 chars. diff --git a/github/artifacts.go b/github/artifacts.go index 3ff5c5a81..21c25878e 100644 --- a/github/artifacts.go +++ b/github/artifacts.go @@ -4,6 +4,7 @@ import ( "archive/zip" "bytes" "context" + "encoding/json" "fmt" "io" "regexp" @@ -37,6 +38,10 @@ var artifactLinkPattern = regexp.MustCompile( // FindGavelArtifact scans PR comments for the gavel sticky comment and // extracts the artifact ID and URL. It returns the most recent match. +// +// Prefer FindGavelArtifacts (plural) for surfaces that can show multiple +// gavel runs per PR; this single-result helper exists for legacy callers +// that only need the headline artifact. func FindGavelArtifact(comments []PRComment) (artifactID int64, artifactURL string, found bool) { for i := len(comments) - 1; i >= 0; i-- { body := comments[i].Body @@ -57,10 +62,164 @@ func FindGavelArtifact(comments []PRComment) (artifactID int64, artifactURL stri return 0, "", false } -// DownloadArtifact downloads a GitHub Actions artifact ZIP and extracts -// gavel-results.json from it. The opts.Repo field must be set to the -// owner/repo that owns the artifact. +// GavelArtifact identifies a single uploaded artifact whose name starts with +// the "gavel" prefix on a PR's workflow runs. +type GavelArtifact struct { + // Name is the upload-artifact name (e.g. "gavel-results", "gavel-bench"). + Name string `json:"name"` + // ID is the GitHub artifact ID; pass to DownloadArtifact / DownloadArtifactFiles. + ID int64 `json:"id"` + // RunID is the workflow run that produced the artifact. Used to group + // artifacts coming from the same job. + RunID int64 `json:"runId"` + // URL is the human-facing artifact URL embedded in the PR comment / job summary. + URL string `json:"url"` + // SizeBytes is the compressed size reported by the API; useful for surfacing + // "too large to fetch" hints in the UI. + SizeBytes int64 `json:"sizeBytes,omitempty"` + // Expired is true when GitHub has aged out the artifact and the bytes are no + // longer downloadable. + Expired bool `json:"expired,omitempty"` +} + +// HTMLURL returns the github.com URL for browsing the artifact in the Actions UI. +func (a GavelArtifact) HTMLURL(repo string) string { + return fmt.Sprintf("https://github.com/%s/actions/runs/%d/artifacts/%d", repo, a.RunID, a.ID) +} + +type restArtifact struct { + ID int64 `json:"id"` + Name string `json:"name"` + SizeInBytes int64 `json:"size_in_bytes"` + ArchiveDownloadURL string `json:"archive_download_url"` + Expired bool `json:"expired"` + WorkflowRun struct { + ID int64 `json:"id"` + } `json:"workflow_run"` +} + +type restArtifactsResponse struct { + TotalCount int64 `json:"total_count"` + Artifacts []restArtifact `json:"artifacts"` +} + +// ListRunArtifacts returns every artifact attached to a single workflow run. +// The result is unfiltered — callers (e.g. FindGavelArtifacts) decide which +// names matter. +func ListRunArtifacts(opts Options, runID int64) ([]GavelArtifact, error) { + token, err := opts.token() + if err != nil { + return nil, fmt.Errorf("cannot list artifacts: %w", err) + } + repo, err := opts.resolveRepo() + if err != nil { + return nil, fmt.Errorf("cannot resolve repo for artifacts: %w", err) + } + + path := fmt.Sprintf("/repos/%s/actions/runs/%d/artifacts?per_page=100", repo, runID) + resp, err := cachedGet(context.Background(), token, path, nil) + if err != nil { + return nil, fmt.Errorf("list artifacts for run %d: %w", runID, err) + } + + var payload restArtifactsResponse + if err := json.Unmarshal(resp.Body, &payload); err != nil { + return nil, fmt.Errorf("parse artifacts list for run %d: %w", runID, err) + } + + out := make([]GavelArtifact, 0, len(payload.Artifacts)) + for _, a := range payload.Artifacts { + ga := GavelArtifact{ + Name: a.Name, + ID: a.ID, + RunID: a.WorkflowRun.ID, + SizeBytes: a.SizeInBytes, + Expired: a.Expired, + } + if ga.RunID == 0 { + ga.RunID = runID + } + ga.URL = ga.HTMLURL(repo) + out = append(out, ga) + } + return out, nil +} + +// FindGavelArtifacts walks every workflow run on the PR and returns artifacts +// whose name starts with the "gavel" prefix (case-insensitive). The result is +// de-duplicated by artifact ID. +// +// Discovery uses the Actions API rather than the sticky comment, so a PR with +// N parallel test jobs (matrix builds, multiple OSes, separate bench job…) +// surfaces N artifacts instead of just the most recently posted one. +func FindGavelArtifacts(opts Options, pr *PRInfo) ([]GavelArtifact, error) { + if pr == nil { + return nil, fmt.Errorf("nil PR") + } + + seenRun := make(map[int64]bool) + var runIDs []int64 + for _, check := range pr.StatusCheckRollup { + runID, err := ExtractRunID(check.DetailsURL) + if err != nil || seenRun[runID] { + continue + } + seenRun[runID] = true + runIDs = append(runIDs, runID) + } + + seenArtifact := make(map[int64]bool) + var out []GavelArtifact + for _, runID := range runIDs { + artifacts, err := ListRunArtifacts(opts, runID) + if err != nil { + logger.Warnf("list artifacts for run %d: %v", runID, err) + continue + } + for _, a := range artifacts { + if !hasGavelPrefix(a.Name) { + continue + } + if seenArtifact[a.ID] { + continue + } + seenArtifact[a.ID] = true + out = append(out, a) + } + } + return out, nil +} + +func hasGavelPrefix(name string) bool { + return strings.HasPrefix(strings.ToLower(name), "gavel") +} + +// DownloadArtifact downloads a GitHub Actions artifact ZIP and returns the +// first .json file found inside. Use DownloadArtifactFiles when an artifact +// can legitimately contain multiple JSON payloads that should be merged. func DownloadArtifact(opts Options, artifactID int64) ([]byte, error) { + files, err := DownloadArtifactFiles(opts, artifactID) + if err != nil { + return nil, err + } + for _, f := range files { + if strings.HasSuffix(strings.ToLower(f.Name), ".json") { + return f.Body, nil + } + } + return nil, fmt.Errorf("no .json file found in artifact zip") +} + +// ArtifactFile is one entry pulled from an artifact zip. +type ArtifactFile struct { + Name string + Body []byte +} + +// DownloadArtifactFiles downloads an artifact ZIP and returns every .json +// entry inside it. Useful when a single uploaded artifact bundles results +// from multiple test packages or jobs that should be merged. +func DownloadArtifactFiles(opts Options, artifactID int64) ([]ArtifactFile, error) { token, err := opts.token() if err != nil { return nil, fmt.Errorf("cannot download artifact: %w", err) @@ -76,16 +235,17 @@ func DownloadArtifact(opts Options, artifactID int64) ([]byte, error) { return nil, fmt.Errorf("download artifact %d: %w", artifactID, err) } - return extractJSONFromZip(result.Body) + return extractJSONFilesFromZip(result.Body) } -func extractJSONFromZip(data []byte) ([]byte, error) { +func extractJSONFilesFromZip(data []byte) ([]ArtifactFile, error) { r, err := zip.NewReader(bytes.NewReader(data), int64(len(data))) if err != nil { return nil, fmt.Errorf("open artifact zip: %w", err) } + var out []ArtifactFile for _, f := range r.File { - if !strings.HasSuffix(f.Name, ".json") { + if !strings.HasSuffix(strings.ToLower(f.Name), ".json") { continue } rc, err := f.Open() @@ -93,12 +253,15 @@ func extractJSONFromZip(data []byte) ([]byte, error) { logger.Warnf("skip zip entry %s: %v", f.Name, err) continue } - defer rc.Close() - content, err := io.ReadAll(io.LimitReader(rc, 50<<20)) // 50 MB cap + content, err := io.ReadAll(io.LimitReader(rc, 50<<20)) // 50 MB cap per file + rc.Close() if err != nil { return nil, fmt.Errorf("read zip entry %s: %w", f.Name, err) } - return content, nil + out = append(out, ArtifactFile{Name: f.Name, Body: content}) } - return nil, fmt.Errorf("no .json file found in artifact zip") + if len(out) == 0 { + return nil, fmt.Errorf("no .json file found in artifact zip") + } + return out, nil } diff --git a/github/artifacts_test.go b/github/artifacts_test.go index bcb6e860e..4b115217e 100644 --- a/github/artifacts_test.go +++ b/github/artifacts_test.go @@ -1,6 +1,10 @@ package github import ( + "archive/zip" + "bytes" + "encoding/json" + "strings" "testing" ) @@ -154,3 +158,157 @@ func TestFindGavelArtifact(t *testing.T) { }) } } + +func buildZipFixture(t *testing.T, files map[string]string) []byte { + t.Helper() + var buf bytes.Buffer + w := zip.NewWriter(&buf) + for name, body := range files { + f, err := w.Create(name) + if err != nil { + t.Fatalf("zip create %s: %v", name, err) + } + if _, err := f.Write([]byte(body)); err != nil { + t.Fatalf("zip write %s: %v", name, err) + } + } + if err := w.Close(); err != nil { + t.Fatalf("zip close: %v", err) + } + return buf.Bytes() +} + +// TestExtractJSONFilesFromZipReturnsAllJSON proves the multi-file extractor +// keeps every .json entry — single-file artifacts that bundle multiple test +// shards (e.g. `gavel-results.json`, `gavel-bench.json`) need all of them +// merged downstream, not just the first one alphabetically. +func TestExtractJSONFilesFromZipReturnsAllJSON(t *testing.T) { + zipData := buildZipFixture(t, map[string]string{ + "gavel-results.json": `{"tests":[]}`, + "gavel-bench.json": `{"bench":{}}`, + "gavel.log": "log line\n", + "README.txt": "ignored", + }) + files, err := extractJSONFilesFromZip(zipData) + if err != nil { + t.Fatalf("extract: %v", err) + } + if len(files) != 2 { + t.Fatalf("expected 2 .json files, got %d (%v)", len(files), names(files)) + } + got := map[string]string{} + for _, f := range files { + got[f.Name] = string(f.Body) + } + if got["gavel-results.json"] != `{"tests":[]}` { + t.Errorf("gavel-results.json body = %q", got["gavel-results.json"]) + } + if got["gavel-bench.json"] != `{"bench":{}}` { + t.Errorf("gavel-bench.json body = %q", got["gavel-bench.json"]) + } +} + +func TestExtractJSONFilesFromZipNoJSON(t *testing.T) { + zipData := buildZipFixture(t, map[string]string{ + "gavel.log": "no json here", + "README.txt": "still nope", + }) + if _, err := extractJSONFilesFromZip(zipData); err == nil { + t.Fatal("expected error when zip has no .json file") + } +} + +func names(files []ArtifactFile) []string { + out := make([]string, len(files)) + for i, f := range files { + out[i] = f.Name + } + return out +} + +// TestHasGavelPrefixAcceptsExpectedNames pins down the set of artifact names +// that should match. Anything starting with "gavel" (case-insensitive) is in; +// names that contain "gavel" elsewhere or use other prefixes are out. +func TestHasGavelPrefixAcceptsExpectedNames(t *testing.T) { + for _, in := range []string{ + "gavel-results", "gavel-bench", "Gavel-Results", + "gavel", "gavel-self-test", "gavel-results-linux", + } { + if !hasGavelPrefix(in) { + t.Errorf("expected match for %q", in) + } + } + for _, out := range []string{ + "results", "test-results-gavel", "junit-gavel-results", "", + } { + if hasGavelPrefix(out) { + t.Errorf("unexpected match for %q", out) + } + } +} + +// TestRestArtifactsResponseUnmarshal sanity-checks the wire shape we parse +// out of /repos/{owner}/{name}/actions/runs/{id}/artifacts so a future GitHub +// payload tweak (snake_case key rename, nested workflow_run shape change) +// fails this test instead of silently returning empty results in production. +func TestRestArtifactsResponseUnmarshal(t *testing.T) { + const payload = `{ + "total_count": 2, + "artifacts": [ + { + "id": 111, + "name": "gavel-results", + "size_in_bytes": 4096, + "archive_download_url": "https://api.github.com/repos/o/r/actions/artifacts/111/zip", + "expired": false, + "workflow_run": {"id": 99} + }, + { + "id": 222, + "name": "gavel-bench", + "size_in_bytes": 2048, + "expired": true, + "workflow_run": {"id": 99} + } + ] + }` + var got restArtifactsResponse + if err := json.Unmarshal([]byte(payload), &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if got.TotalCount != 2 { + t.Errorf("TotalCount = %d, want 2", got.TotalCount) + } + if len(got.Artifacts) != 2 { + t.Fatalf("artifacts = %d, want 2", len(got.Artifacts)) + } + if got.Artifacts[0].Name != "gavel-results" || got.Artifacts[0].WorkflowRun.ID != 99 { + t.Errorf("artifact 0 = %+v", got.Artifacts[0]) + } + if !got.Artifacts[1].Expired { + t.Errorf("artifact 1 should be Expired") + } +} + +// TestGavelArtifactHTMLURL pins down the artifact URL we hand the UI so +// in-app "open in Actions" links don't silently break if HTMLURL is +// refactored. +func TestGavelArtifactHTMLURL(t *testing.T) { + a := GavelArtifact{ID: 555, RunID: 999} + want := "https://github.com/owner/name/actions/runs/999/artifacts/555" + if got := a.HTMLURL("owner/name"); got != want { + t.Errorf("HTMLURL = %q, want %q", got, want) + } + // Confirm the URL is also recognised by the inverse parser — round-trip + // guards against a regex/fmt drift between the two helpers. + if !strings.Contains(want, "/runs/999/artifacts/555") { + t.Fatalf("test URL fixture is malformed: %q", want) + } + repo, runID, artID, err := ParseArtifactURL(want) + if err != nil { + t.Fatalf("ParseArtifactURL: %v", err) + } + if repo != "owner/name" || runID != 999 || artID != 555 { + t.Errorf("round-trip mismatch: repo=%q run=%d art=%d", repo, runID, artID) + } +} diff --git a/pr/ui/gavel_results.go b/pr/ui/gavel_results.go index d62ad7836..19a31d122 100644 --- a/pr/ui/gavel_results.go +++ b/pr/ui/gavel_results.go @@ -36,6 +36,37 @@ type GavelResultsSummary struct { TopFailures []TestFailure `json:"topFailures,omitempty"` // TopLintViolations lists the first 5 lint findings across all linters. TopLintViolations []LintViolation `json:"topLintViolations,omitempty"` + // Jobs is the per-job breakdown when the PR has more than one gavel + // artifact. Empty when only a single artifact was discovered. The + // outer counts/TopFailures fields are the sum across Jobs so PR + // sidebar badges and existing single-summary consumers keep working. + Jobs []GavelJobSummary `json:"jobs,omitempty"` +} + +// GavelJobSummary holds the merged results from one workflow run / job. +// When a single job uploads N gavel-* artifacts (matrix shards, separate +// bench json, etc.), all of them are downloaded and merged into one entry. +type GavelJobSummary struct { + // JobName is a human label — typically the artifact name (or a comma list + // when several artifacts were merged into the same run). + JobName string `json:"jobName"` + // RunID is the workflow run that produced these artifacts. + RunID int64 `json:"runId,omitempty"` + // ArtifactIDs lists every artifact ID merged into this entry. The first + // ID is treated as the canonical "open in UI" target. + ArtifactIDs []int64 `json:"artifactIds"` + ArtifactURL string `json:"artifactUrl,omitempty"` + TestsPassed int `json:"testsPassed"` + TestsFailed int `json:"testsFailed"` + TestsSkipped int `json:"testsSkipped"` + TestsTotal int `json:"testsTotal"` + LintViolations int `json:"lintViolations"` + LintLinters int `json:"lintLinters"` + HasBench bool `json:"hasBench"` + BenchRegressions int `json:"benchRegressions,omitempty"` + Error string `json:"error,omitempty"` + TopFailures []TestFailure `json:"topFailures,omitempty"` + TopLintViolations []LintViolation `json:"topLintViolations,omitempty"` } type TestFailure struct { @@ -82,6 +113,21 @@ func (g *gavelResultJSON) UnmarshalJSON(data []byte) error { return nil } +// mergeGavelResults concatenates tests/lint/bench from multiple JSON payloads +// belonging to the same job. The first non-nil bench wins (bench comparisons +// are aggregate-level; we don't try to merge two of them). +func mergeGavelResults(payloads ...gavelResultJSON) gavelResultJSON { + var merged gavelResultJSON + for _, p := range payloads { + merged.Tests = append(merged.Tests, p.Tests...) + merged.Lint = append(merged.Lint, p.Lint...) + if merged.Bench == nil { + merged.Bench = p.Bench + } + } + return merged +} + func computeGavelSummary(jsonBytes []byte, artifactID int64, artifactURL string) *GavelResultsSummary { var data gavelResultJSON if err := json.Unmarshal(jsonBytes, &data); err != nil { @@ -96,7 +142,14 @@ func computeGavelSummary(jsonBytes []byte, artifactID int64, artifactURL string) ArtifactID: artifactID, ArtifactURL: artifactURL, } + applyResultsToSummary(data, summary) + return summary +} +// applyResultsToSummary populates the count/top fields of a GavelResultsSummary +// from a (possibly-merged) gavelResultJSON payload. Used both by the +// single-artifact path and by the multi-artifact merge path. +func applyResultsToSummary(data gavelResultJSON, summary *GavelResultsSummary) { for _, root := range data.Tests { walkTestCounts(root, summary) } @@ -129,8 +182,149 @@ func computeGavelSummary(jsonBytes []byte, artifactID int64, artifactURL string) } } } +} - return summary +// computeGavelJobSummary downloads every artifact in `arts`, merges all of +// their .json payloads into one logical result set, and returns a single +// GavelJobSummary representing the job. +// +// `arts` MUST share a workflow run — typically the slice produced by grouping +// FindGavelArtifacts() output by RunID. Download errors on individual +// artifacts are recorded in the Error field rather than aborting the merge, +// so a partially-broken job still surfaces its successful files. +func computeGavelJobSummary(opts github.Options, arts []github.GavelArtifact) GavelJobSummary { + if len(arts) == 0 { + return GavelJobSummary{Error: "no artifacts"} + } + + job := GavelJobSummary{ + RunID: arts[0].RunID, + JobName: jobNameFromArtifacts(arts), + ArtifactURL: arts[0].URL, + } + job.ArtifactIDs = make([]int64, 0, len(arts)) + + var payloads []gavelResultJSON + var partialErrors []string + for _, a := range arts { + job.ArtifactIDs = append(job.ArtifactIDs, a.ID) + if a.Expired { + partialErrors = append(partialErrors, fmt.Sprintf("artifact %s (%d) expired", a.Name, a.ID)) + continue + } + files, err := github.DownloadArtifactFiles(opts, a.ID) + if err != nil { + partialErrors = append(partialErrors, fmt.Sprintf("download %s (%d): %v", a.Name, a.ID, err)) + continue + } + for _, f := range files { + var p gavelResultJSON + if err := json.Unmarshal(f.Body, &p); err != nil { + partialErrors = append(partialErrors, fmt.Sprintf("parse %s/%s: %v", a.Name, f.Name, err)) + continue + } + payloads = append(payloads, p) + } + } + + merged := mergeGavelResults(payloads...) + + // Reuse the summary-population logic via a transient summary, then copy + // the count fields onto the job struct. Keeps a single source of truth + // for "how do we count tests". + tmp := &GavelResultsSummary{} + applyResultsToSummary(merged, tmp) + + job.TestsPassed = tmp.TestsPassed + job.TestsFailed = tmp.TestsFailed + job.TestsSkipped = tmp.TestsSkipped + job.TestsTotal = tmp.TestsTotal + job.LintViolations = tmp.LintViolations + job.LintLinters = tmp.LintLinters + job.HasBench = tmp.HasBench + job.BenchRegressions = tmp.BenchRegressions + job.TopFailures = tmp.TopFailures + job.TopLintViolations = tmp.TopLintViolations + + if len(partialErrors) > 0 && len(payloads) == 0 { + // Total failure: surface the first error verbatim so the UI shows + // it instead of a misleading "0 tests" panel. + job.Error = partialErrors[0] + } else if len(partialErrors) > 0 { + job.Error = fmt.Sprintf("%d of %d artifact(s) failed to load: %s", + len(partialErrors), len(arts), strings.Join(partialErrors, "; ")) + } + return job +} + +// jobNameFromArtifacts produces a human label for a job entry. When all +// artifacts share a name we use that; when names differ we join them so the +// UI can show "gavel-results, gavel-bench". +func jobNameFromArtifacts(arts []github.GavelArtifact) string { + if len(arts) == 0 { + return "" + } + seen := make(map[string]bool, len(arts)) + var names []string + for _, a := range arts { + if seen[a.Name] { + continue + } + seen[a.Name] = true + names = append(names, a.Name) + } + return strings.Join(names, ", ") +} + +// summarizeGavelArtifacts groups the discovered artifacts by workflow run, +// downloads + merges each group, then folds the per-job results into a +// single PR-level summary. The PR-level summary's Jobs field carries the +// per-job breakdown for surfaces that want detail. +func summarizeGavelArtifacts(opts github.Options, arts []github.GavelArtifact) *GavelResultsSummary { + if len(arts) == 0 { + return nil + } + + byRun := make(map[int64][]github.GavelArtifact) + var runOrder []int64 + for _, a := range arts { + if _, ok := byRun[a.RunID]; !ok { + runOrder = append(runOrder, a.RunID) + } + byRun[a.RunID] = append(byRun[a.RunID], a) + } + + pr := &GavelResultsSummary{ + ArtifactID: arts[0].ID, + ArtifactURL: arts[0].URL, + } + for _, runID := range runOrder { + job := computeGavelJobSummary(opts, byRun[runID]) + pr.Jobs = append(pr.Jobs, job) + pr.TestsPassed += job.TestsPassed + pr.TestsFailed += job.TestsFailed + pr.TestsSkipped += job.TestsSkipped + pr.TestsTotal += job.TestsTotal + pr.LintViolations += job.LintViolations + pr.LintLinters += job.LintLinters + if job.HasBench { + pr.HasBench = true + } + pr.BenchRegressions += job.BenchRegressions + for _, f := range job.TopFailures { + if len(pr.TopFailures) >= 5 { + break + } + pr.TopFailures = append(pr.TopFailures, f) + } + for _, v := range job.TopLintViolations { + if len(pr.TopLintViolations) >= 5 { + break + } + pr.TopLintViolations = append(pr.TopLintViolations, v) + } + } + return pr } func walkTestCounts(t parsers.Test, s *GavelResultsSummary) { @@ -231,29 +425,39 @@ func (s *Server) getOrCreateArtifact(artifactID int64, repo string) (*artifactEn opts := s.ghOpts opts.Repo = repo - jsonBytes, err := github.DownloadArtifact(opts, artifactID) + files, err := github.DownloadArtifactFiles(opts, artifactID) if err != nil { return nil, err } - summary := computeGavelSummary(jsonBytes, artifactID, "") + // Merge every JSON entry in the zip into a single payload before + // computing summary / loading the snapshot. Single-file artifacts hit + // the same path with one payload, so behaviour is unchanged for them. + var payloads []gavelResultJSON + for _, f := range files { + var p gavelResultJSON + if err := json.Unmarshal(f.Body, &p); err != nil { + logger.Warnf("artifact %d: parse %s: %v", artifactID, f.Name, err) + continue + } + payloads = append(payloads, p) + } + if len(payloads) == 0 { + return nil, fmt.Errorf("artifact %d: no parseable .json file in zip", artifactID) + } + merged := mergeGavelResults(payloads...) + + summary := &GavelResultsSummary{ArtifactID: artifactID} + applyResultsToSummary(merged, summary) srv := testui.NewServer() - var snap testui.Snapshot - if err := json.Unmarshal(jsonBytes, &snap); err != nil { - logger.Warnf("artifact %d: unmarshal as snapshot: %v, trying legacy format", artifactID, err) - var data gavelResultJSON - if err := json.Unmarshal(jsonBytes, &data); err != nil { - return nil, fmt.Errorf("parse artifact %d: %w", artifactID, err) - } - snap = testui.Snapshot{ - Tests: data.Tests, - Lint: data.Lint, - Bench: data.Bench, - Status: testui.SnapshotStatus{ - LintRun: len(data.Lint) > 0, - }, - } + snap := testui.Snapshot{ + Tests: merged.Tests, + Lint: merged.Lint, + Bench: merged.Bench, + Status: testui.SnapshotStatus{ + LintRun: len(merged.Lint) > 0, + }, } srv.LoadSnapshot(snap) srv.MarkDone() diff --git a/pr/ui/gavel_results_test.go b/pr/ui/gavel_results_test.go index 80b669835..e0556705c 100644 --- a/pr/ui/gavel_results_test.go +++ b/pr/ui/gavel_results_test.go @@ -228,3 +228,127 @@ func TestSnapshotIncludesGavelResults(t *testing.T) { t.Errorf("marshaled snapshot missing gavelResults field: %s", b) } } + +// TestMergeGavelResultsConcatenates exercises the per-job merge: when one job +// uploads multiple gavel-* artifacts (e.g. matrix shards or a separate bench +// JSON), all of their tests/lint/bench data must aggregate into one logical +// payload. Pass + Fail + Skip counts must equal the sum across inputs. +func TestMergeGavelResultsConcatenates(t *testing.T) { + a := gavelResultJSON{} + if err := json.Unmarshal([]byte(`{"tests":[{"name":"A1","passed":true},{"name":"A2","failed":true}]}`), &a); err != nil { + t.Fatal(err) + } + b := gavelResultJSON{} + if err := json.Unmarshal([]byte(`{"tests":[{"name":"B1","passed":true}],"lint":[{"linter":"lint1","success":false,"violations":[{"file":"x.go","line":1,"message":"x"}]}]}`), &b); err != nil { + t.Fatal(err) + } + + merged := mergeGavelResults(a, b) + if len(merged.Tests) != 3 { + t.Errorf("merged tests = %d, want 3", len(merged.Tests)) + } + if len(merged.Lint) != 1 { + t.Errorf("merged lint = %d, want 1", len(merged.Lint)) + } + + summary := &GavelResultsSummary{} + applyResultsToSummary(merged, summary) + if summary.TestsPassed != 2 || summary.TestsFailed != 1 || summary.TestsTotal != 3 { + t.Errorf("summary counts wrong: passed=%d failed=%d total=%d", + summary.TestsPassed, summary.TestsFailed, summary.TestsTotal) + } + if summary.LintViolations != 1 { + t.Errorf("LintViolations = %d, want 1", summary.LintViolations) + } +} + +// TestMergeGavelResultsKeepsFirstBench documents that bench comparisons are +// not summed across artifacts — the first non-nil one wins. Two artifacts +// each carrying their own bench struct is rare, but if it happens we want a +// deterministic answer (no double-counting regressions, no panic). +func TestMergeGavelResultsKeepsFirstBench(t *testing.T) { + a := gavelResultJSON{} + b := gavelResultJSON{} + if err := json.Unmarshal([]byte(`{"bench":{"threshold":1.05}}`), &a); err != nil { + t.Fatal(err) + } + if err := json.Unmarshal([]byte(`{"bench":{"threshold":2.0}}`), &b); err != nil { + t.Fatal(err) + } + merged := mergeGavelResults(a, b) + if merged.Bench == nil || merged.Bench.Threshold != 1.05 { + t.Errorf("expected first bench to win (threshold=1.05), got %+v", merged.Bench) + } +} + +// TestJobNameFromArtifactsDistinct asserts that distinct artifact names from +// the same job are joined into a comma-separated label so the UI can show +// "gavel-results, gavel-bench" instead of just "gavel-results". +func TestJobNameFromArtifactsDistinct(t *testing.T) { + got := jobNameFromArtifacts([]github.GavelArtifact{ + {Name: "gavel-results"}, + {Name: "gavel-bench"}, + {Name: "gavel-results"}, // duplicate must be deduped + }) + if got != "gavel-results, gavel-bench" { + t.Errorf("jobNameFromArtifacts = %q, want %q", got, "gavel-results, gavel-bench") + } +} + +// TestSummarizeGavelArtifactsGroupsByRun verifies that the PR-level rollup +// groups artifacts from the same workflow run into a single GavelJobSummary +// entry. We can't exercise the network-fetch path here (DownloadArtifactFiles +// would call out to GitHub), so we use an empty artifact set per run — the +// grouping/RunID handling is what matters. +func TestSummarizeGavelArtifactsGroupsByRun(t *testing.T) { + // Fake out the github layer by passing only metadata: the function will + // try to download and fail, recording an error on each job. That's + // enough to assert the grouping shape. + arts := []github.GavelArtifact{ + {Name: "gavel-results", ID: 1, RunID: 100, URL: "u1"}, + {Name: "gavel-bench", ID: 2, RunID: 100, URL: "u2"}, + {Name: "gavel-results", ID: 3, RunID: 200, URL: "u3"}, + } + // Without a token, DownloadArtifactFiles errors immediately at + // opts.token() — no network call. The merge logic still has to walk + // every group and produce the right number of Job entries. + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + pr := summarizeGavelArtifacts(github.Options{Repo: "o/r"}, arts) + if pr == nil { + t.Fatal("summarizeGavelArtifacts returned nil") + } + if len(pr.Jobs) != 2 { + t.Fatalf("Jobs = %d, want 2 (one per RunID)", len(pr.Jobs)) + } + // Job for RunID 100 must merge both artifact IDs. + var run100 *GavelJobSummary + for i := range pr.Jobs { + if pr.Jobs[i].RunID == 100 { + run100 = &pr.Jobs[i] + } + } + if run100 == nil { + t.Fatalf("missing RunID 100 in Jobs: %+v", pr.Jobs) + } + if len(run100.ArtifactIDs) != 2 { + t.Errorf("RunID 100 ArtifactIDs = %v, want 2 entries", run100.ArtifactIDs) + } + if run100.JobName != "gavel-results, gavel-bench" { + t.Errorf("RunID 100 JobName = %q", run100.JobName) + } + // Each job recorded an error (no token); the PR-level counts should + // therefore be zero rather than misleading partials. + if pr.TestsTotal != 0 { + t.Errorf("expected 0 tests (all downloads failed), got %d", pr.TestsTotal) + } +} + +// TestSummarizeGavelArtifactsEmpty asserts that an empty input returns nil +// so the caller can short-circuit emit("gavel", ...) and not show an empty +// "Gavel results" section in the UI. +func TestSummarizeGavelArtifactsEmpty(t *testing.T) { + if got := summarizeGavelArtifacts(github.Options{}, nil); got != nil { + t.Errorf("expected nil for empty input, got %+v", got) + } +} diff --git a/pr/ui/handler.go b/pr/ui/handler.go index 654d44abf..0441891a3 100644 --- a/pr/ui/handler.go +++ b/pr/ui/handler.go @@ -917,26 +917,42 @@ func (s *Server) handleDetail(w http.ResponseWriter, r *http.Request) { runIDs = append(runIDs, runID) } - // Start gavel results fetch in parallel + // Start gavel results fetch in parallel. Discovery first walks every + // workflow run on the PR for artifacts whose name starts with "gavel" + // (matrix shards, separate bench jobs, etc. all show up). Sticky + // comment is a fallback for cases where the runs API turns up empty — + // e.g. an artifact uploaded by a workflow whose run isn't in + // StatusCheckRollup. type gavelResult struct { summary *GavelResultsSummary } gavelCh := make(chan gavelResult, 1) allComments := append(pr.Comments, pr.ReviewThreads...) - artifactID, artifactURL, hasArtifact := github.FindGavelArtifact(allComments) - if hasArtifact { + artifacts, _ := github.FindGavelArtifacts(opts, pr) + hasArtifact := len(artifacts) > 0 + var stickyID int64 + var stickyURL string + var hasSticky bool + if !hasArtifact { + stickyID, stickyURL, hasSticky = github.FindGavelArtifact(allComments) + } + if hasArtifact || hasSticky { go func() { - jsonBytes, err := github.DownloadArtifact(opts, artifactID) + if hasArtifact { + gavelCh <- gavelResult{summarizeGavelArtifacts(opts, artifacts)} + return + } + jsonBytes, err := github.DownloadArtifact(opts, stickyID) if err != nil { - logger.Warnf("artifact %d download failed: %v", artifactID, err) + logger.Warnf("artifact %d download failed: %v", stickyID, err) gavelCh <- gavelResult{&GavelResultsSummary{ - ArtifactID: artifactID, - ArtifactURL: artifactURL, + ArtifactID: stickyID, + ArtifactURL: stickyURL, Error: err.Error(), }} - } else { - gavelCh <- gavelResult{computeGavelSummary(jsonBytes, artifactID, artifactURL)} + return } + gavelCh <- gavelResult{computeGavelSummary(jsonBytes, stickyID, stickyURL)} }() } @@ -967,7 +983,7 @@ func (s *Server) handleDetail(w http.ResponseWriter, r *http.Request) { // Wait for gavel results var gavelSummary *GavelResultsSummary - if hasArtifact { + if hasArtifact || hasSticky { gr := <-gavelCh gavelSummary = gr.summary emit("gavel", map[string]any{"gavelResults": gavelSummary}) @@ -1019,8 +1035,15 @@ func (s *Server) fetchPRDetail(repo string, number int) prDetail { result.Runs = runs result.Comments = prwatch.MergeAndFilter(pr.Comments, pr.ReviewThreads) - // Scan all comments (including general issue comments, not just review - // threads) for a gavel sticky comment with an artifact link. + // Discover gavel artifacts via the workflow runs API first, then fall + // back to the sticky-comment URL if nothing turned up. The runs API + // catches matrix shards / multiple gavel-* uploads that the single + // sticky-comment URL can't represent. + if artifacts, _ := github.FindGavelArtifacts(opts, pr); len(artifacts) > 0 { + result.GavelResults = summarizeGavelArtifacts(opts, artifacts) + s.setGavelSummary(repo, number, result.GavelResults) + return result + } allComments := append(pr.Comments, pr.ReviewThreads...) if artifactID, artifactURL, found := github.FindGavelArtifact(allComments); found { jsonBytes, err := github.DownloadArtifact(opts, artifactID) From 1714de95302232f7a81ab2b6075eac6817ed6460 Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Mon, 27 Apr 2026 08:39:48 +0300 Subject: [PATCH 5/5] feat(ui): add snapshot index and replay mode to gavel ui serve Implement a new index view that lists previously-saved test snapshots from the .gavel/ directory, allowing users to browse and replay historical test runs without re-running tests. Key changes: - Add /api/runs endpoint to list snapshots with metadata (test counts, lint violations, duration, git SHA) - Add /api/runs/{name} endpoint to load individual snapshots, with pointer resolution for aliased runs (last, main, master) - Add resolveGavelRoot() to discover .gavel/ directory when gavel ui serve is run with no arguments - Implement RunIndex frontend component with sortable snapshot table showing test results and lint counts - Add route parsing for /run/{name} URLs to support deep-linking into specific snapshots - Maintain backward compatibility with existing /tests, /lint, etc. routes Users can now run `gavel ui serve` in a git repo to browse all saved snapshots, or continue using `gavel ui serve ` to view a specific snapshot. Pointer files (last.json, main.json) are automatically resolved to their target snapshots. --- .gavel.yaml | 41 +++- cmd/gavel/ui_serve.go | 31 +++ testrunner/ui/handler.go | 3 + testrunner/ui/routes.go | 17 ++ testrunner/ui/runs_index.go | 263 ++++++++++++++++++++++ testrunner/ui/runs_index_test.go | 177 +++++++++++++++ testrunner/ui/src/App.tsx | 105 ++++++++- testrunner/ui/src/components/RunIndex.tsx | 180 +++++++++++++++ testrunner/ui/src/routes.test.ts | 64 +++++- testrunner/ui/src/routes.ts | 37 ++- testrunner/ui/src/types.ts | 21 ++ 11 files changed, 926 insertions(+), 13 deletions(-) create mode 100644 testrunner/ui/runs_index.go create mode 100644 testrunner/ui/runs_index_test.go create mode 100644 testrunner/ui/src/components/RunIndex.tsx diff --git a/.gavel.yaml b/.gavel.yaml index 5f83f6df9..2f0d3a948 100644 --- a/.gavel.yaml +++ b/.gavel.yaml @@ -1,9 +1,46 @@ -commit: {} +commit: + gitignore: + - .bin/ + - .cache + - .claude + - .container-sandbox.yaml + - .DS_Store + - .DS_Store? + - .env + - .env.local + - .gavel/ + - .gingko/ + - .idea/ + - .jscpd.json + - .playwright-mcp/ + - .shell/ + - .Spotlight-V100 + - .task + - .tmp/ + - .Trashes + - .vitest/ + - .vscode/ + - '*.log' + - '*.test' + - node_modules/ + - dist/ + - '*.tmp' + - .metadata_never_index + - .tmp + - .agents/ + - .sandbox.yaml + - nohup.out + - skills-lock.json + linkedDeps: {} fixtures: {} lint: ignore: - file: .shell/checkout/8d512cac74b933d1f8d97629ab79966828d2e0789c66a463d8a78e7d40ccc42f/** -secrets: {} + - file: linters/betterleaks/testdata/betterleaks-output.json + source: betterleaks +secrets: + configs: + - .betterleaks.toml ssh: {} verify: checks: diff --git a/cmd/gavel/ui_serve.go b/cmd/gavel/ui_serve.go index e041af2ee..15d38c81a 100644 --- a/cmd/gavel/ui_serve.go +++ b/cmd/gavel/ui_serve.go @@ -6,8 +6,10 @@ import ( "net" "net/http" "os" + "os/exec" "path/filepath" "strconv" + "strings" "time" "github.com/flanksource/clicky" @@ -94,7 +96,14 @@ func runUIServe(opts UIServeOptions) (any, error) { return nil, fmt.Errorf("load results %v: %w", opts.ResultsFiles, err) } } else { + root, err := resolveGavelRoot() + if err != nil { + listener.Close() //nolint:errcheck + return nil, err + } + srv.SetGavelDir(root) srv.MarkDone() + logger.Infof("Indexing snapshots from %s", filepath.Join(root, ".gavel")) } addr := listener.Addr().(*net.TCPAddr) @@ -315,6 +324,28 @@ func firstNonLoopbackIPv4() string { return "" } +// resolveGavelRoot returns the git root of the current working directory and +// validates that .gavel/ exists inside it. Used by `gavel ui serve` (no args) +// to discover the snapshot directory to index. +func resolveGavelRoot() (string, error) { + out, err := exec.Command("git", "rev-parse", "--show-toplevel").Output() + if err != nil { + return "", fmt.Errorf("not inside a git repository: %w", err) + } + root := strings.TrimSpace(string(out)) + if root == "" { + return "", fmt.Errorf("git rev-parse --show-toplevel returned empty") + } + gavelDir := filepath.Join(root, ".gavel") + if _, err := os.Stat(gavelDir); err != nil { + if os.IsNotExist(err) { + return "", fmt.Errorf("no %s directory found in %s — run `gavel test` to populate it, or pass a snapshot path explicitly", ".gavel", root) + } + return "", err + } + return root, nil +} + // writeURLFile writes url to path atomically (write to tempfile in same dir, // fsync, rename). Atomicity matters because a wrapping shell script may be // polling the file concurrently. diff --git a/testrunner/ui/handler.go b/testrunner/ui/handler.go index 8652d4246..7a4b164c5 100644 --- a/testrunner/ui/handler.go +++ b/testrunner/ui/handler.go @@ -26,6 +26,7 @@ type Server struct { embeddedDiagnostics *DiagnosticsSnapshot updated chan struct{} gitRoot string + gavelDir string diag *DiagnosticsManager rerunMu sync.Mutex @@ -206,6 +207,8 @@ func (s *Server) Handler() http.Handler { mux.HandleFunc("/api/rerun/stream", s.handleRerunStream) mux.HandleFunc("/api/lint/ignore", s.handleLintIgnore) mux.HandleFunc("/api/benchmarks", s.handleBenchJSON) + mux.HandleFunc("/api/runs", s.handleRunsIndex) + mux.HandleFunc("/api/runs/", s.handleRunSnapshot) return mux } diff --git a/testrunner/ui/routes.go b/testrunner/ui/routes.go index 5c43fd761..bc18ba533 100644 --- a/testrunner/ui/routes.go +++ b/testrunner/ui/routes.go @@ -216,6 +216,23 @@ func parseRouteRequest(r *http.Request) (routeRequest, bool) { } segments := strings.Split(path, "/") + // `/run/` is a frontend-only prefix that selects which saved + // snapshot to load. Strip it so the rest of the route parses the same + // way as the bare `//...` form. + if segments[0] == "run" { + if len(segments) < 2 { + // `/run` with no name behaves like the index page. + req.TestFilters = parseTestFilters(r) + req.LintFilters = parseLintFilters(r) + return req, true + } + segments = segments[2:] + if len(segments) == 0 { + req.TestFilters = parseTestFilters(r) + req.LintFilters = parseLintFilters(r) + return req, true + } + } tabSeg := segments[0] pathFormat := "" if base, format := stripKnownFormat(tabSeg); format != "" { diff --git a/testrunner/ui/runs_index.go b/testrunner/ui/runs_index.go new file mode 100644 index 000000000..ed5ad9434 --- /dev/null +++ b/testrunner/ui/runs_index.go @@ -0,0 +1,263 @@ +package testui + +import ( + "encoding/json" + "fmt" + "net/http" + "os" + "path/filepath" + "sort" + "strings" + "time" + + "github.com/flanksource/gavel/testrunner/parsers" +) + +// gavelDir is the conventional snapshot directory written by snapshots.Save. +// Duplicated here (rather than imported from the snapshots package) because +// snapshots already imports this package. +const gavelDir = ".gavel" + +// pointerJSON mirrors snapshots.Pointer's wire shape. Re-declared locally to +// avoid importing the snapshots package, which would create an import cycle. +type pointerJSON struct { + Path string `json:"path"` + SHA string `json:"sha"` + Uncommitted string `json:"uncommitted,omitempty"` +} + +type RunCounts struct { + Total int `json:"total"` + Passed int `json:"passed"` + Failed int `json:"failed"` + Skipped int `json:"skipped"` + Pending int `json:"pending"` +} + +type RunIndexEntry struct { + Name string `json:"name"` + Path string `json:"path"` + Pointer string `json:"pointer,omitempty"` + Modified time.Time `json:"modified"` + SHA string `json:"sha,omitempty"` + Started *time.Time `json:"started,omitempty"` + Ended *time.Time `json:"ended,omitempty"` + Counts *RunCounts `json:"counts,omitempty"` + Lint int `json:"lint,omitempty"` + Error string `json:"error,omitempty"` +} + +// SetGavelDir tells the server which directory holds previously-saved snapshots. +// When set, /api/runs scans this directory and /api/runs/{name} loads a single +// snapshot from it. Empty string disables both endpoints. +func (s *Server) SetGavelDir(root string) { + s.mu.Lock() + defer s.mu.Unlock() + if root == "" { + s.gavelDir = "" + return + } + s.gavelDir = filepath.Join(root, gavelDir) +} + +// GavelDir returns the directory backing /api/runs, or "" if no-arg index mode +// is disabled. +func (s *Server) GavelDir() string { + s.mu.RLock() + defer s.mu.RUnlock() + return s.gavelDir +} + +func (s *Server) handleRunsIndex(w http.ResponseWriter, _ *http.Request) { + dir := s.GavelDir() + if dir == "" { + http.NotFound(w, nil) + return + } + entries, err := buildRunIndex(dir) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(entries) //nolint:errcheck +} + +func (s *Server) handleRunSnapshot(w http.ResponseWriter, r *http.Request) { + dir := s.GavelDir() + if dir == "" { + http.NotFound(w, r) + return + } + name := strings.TrimPrefix(r.URL.Path, "/api/runs/") + if name == "" || strings.ContainsRune(name, '/') || strings.Contains(name, "..") { + http.Error(w, "invalid run name", http.StatusBadRequest) + return + } + if !strings.HasSuffix(name, ".json") { + name += ".json" + } + path := filepath.Join(dir, name) + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + http.NotFound(w, r) + return + } + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + // If this is a pointer, dereference once. + if resolved, ok := tryResolvePointer(dir, data); ok { + data = resolved + } + w.Header().Set("Content-Type", "application/json") + w.Write(data) //nolint:errcheck +} + +// tryResolvePointer detects pointer files (the small {path,sha} shape written +// by snapshots.Save) and returns the bytes of the snapshot they reference. +// Returns (nil, false) when raw is not a pointer or the pointed file cannot be +// read — caller falls back to the original bytes. +func tryResolvePointer(dir string, raw []byte) ([]byte, bool) { + var p pointerJSON + if err := json.Unmarshal(raw, &p); err != nil { + return nil, false + } + if p.Path == "" || p.SHA == "" { + return nil, false + } + target := p.Path + if !filepath.IsAbs(target) { + target = filepath.Join(filepath.Dir(dir), target) + } + data, err := os.ReadFile(target) + if err != nil { + return nil, false + } + return data, true +} + +// buildRunIndex scans dir, returns one entry per .json file with summary +// metadata. Pointers are resolved (stats come from the underlying snapshot) +// and rendered as labeled rows pointing to the resolved snapshot. +func buildRunIndex(dir string) ([]RunIndexEntry, error) { + infos, err := os.ReadDir(dir) + if err != nil { + return nil, fmt.Errorf("read %s: %w", dir, err) + } + + pointers := make([]RunIndexEntry, 0) + snapshots := make([]RunIndexEntry, 0) + + for _, fi := range infos { + if fi.IsDir() { + continue + } + name := fi.Name() + if !strings.HasSuffix(name, ".json") { + continue + } + full := filepath.Join(dir, name) + stat, err := fi.Info() + if err != nil { + continue + } + entry := RunIndexEntry{ + Name: strings.TrimSuffix(name, ".json"), + Path: filepath.Join(gavelDir, name), + Modified: stat.ModTime(), + } + raw, err := os.ReadFile(full) + if err != nil { + entry.Error = err.Error() + snapshots = append(snapshots, entry) + continue + } + + if resolved, ok := tryResolvePointer(dir, raw); ok { + var ptr pointerJSON + _ = json.Unmarshal(raw, &ptr) + entry.Pointer = entry.Name + entry.SHA = shortSHA(ptr.SHA) + // Path points at the resolved snapshot so click-through deep links + // to the same URL as a direct snapshot row. + entry.Path = ptr.Path + fillFromSnapshot(&entry, resolved) + pointers = append(pointers, entry) + continue + } + + fillFromSnapshot(&entry, raw) + snapshots = append(snapshots, entry) + } + + sort.SliceStable(pointers, func(i, j int) bool { + return pointerOrder(pointers[i].Pointer) < pointerOrder(pointers[j].Pointer) || + (pointerOrder(pointers[i].Pointer) == pointerOrder(pointers[j].Pointer) && + pointers[i].Pointer < pointers[j].Pointer) + }) + sort.SliceStable(snapshots, func(i, j int) bool { + return snapshots[i].Modified.After(snapshots[j].Modified) + }) + + out := make([]RunIndexEntry, 0, len(pointers)+len(snapshots)) + out = append(out, pointers...) + out = append(out, snapshots...) + return out, nil +} + +func pointerOrder(name string) int { + switch name { + case "last": + return 0 + case "main", "master": + return 2 + default: + return 1 + } +} + +func fillFromSnapshot(entry *RunIndexEntry, raw []byte) { + var snap Snapshot + if err := json.Unmarshal(raw, &snap); err != nil { + entry.Error = err.Error() + return + } + if snap.Git != nil && entry.SHA == "" { + entry.SHA = shortSHA(snap.Git.SHA) + } + if snap.Metadata != nil { + if !snap.Metadata.Started.IsZero() { + t := snap.Metadata.Started + entry.Started = &t + } + if !snap.Metadata.Ended.IsZero() { + t := snap.Metadata.Ended + entry.Ended = &t + } + } + sum := parsers.Tests(snap.Tests).Sum() + if sum.Total > 0 || sum.Failed > 0 || sum.Skipped > 0 || sum.Pending > 0 { + entry.Counts = &RunCounts{ + Total: sum.Total, + Passed: sum.Passed, + Failed: sum.Failed, + Skipped: sum.Skipped, + Pending: sum.Pending, + } + } + for _, lr := range snap.Lint { + if lr == nil { + continue + } + entry.Lint += len(lr.Violations) + } +} + +func shortSHA(sha string) string { + if len(sha) > 7 { + return sha[:7] + } + return sha +} diff --git a/testrunner/ui/runs_index_test.go b/testrunner/ui/runs_index_test.go new file mode 100644 index 000000000..803ce6a77 --- /dev/null +++ b/testrunner/ui/runs_index_test.go @@ -0,0 +1,177 @@ +package testui_test + +import ( + "encoding/json" + "net/http" + "os" + "path/filepath" + "testing" + "time" + + "github.com/flanksource/gavel/testrunner/parsers" + testui "github.com/flanksource/gavel/testrunner/ui" +) + +func writeTestSnapshot(t *testing.T, path string, snap testui.Snapshot, mtime time.Time) { + t.Helper() + data, err := json.MarshalIndent(snap, "", " ") + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } + if err := os.Chtimes(path, mtime, mtime); err != nil { + t.Fatalf("chtimes %s: %v", path, err) + } +} + +func writeTestPointer(t *testing.T, path string, pointerPath, sha string) { + t.Helper() + body := map[string]string{"path": pointerPath, "sha": sha} + data, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatalf("write pointer %s: %v", path, err) + } +} + +func TestRunsIndexEndpointListsSnapshotsAndPointers(t *testing.T) { + root := t.TempDir() + gavelDir := filepath.Join(root, ".gavel") + if err := os.MkdirAll(gavelDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + older := time.Now().Add(-2 * time.Hour) + newer := time.Now().Add(-30 * time.Minute) + + snapOld := testui.Snapshot{ + Metadata: &testui.SnapshotMetadata{Started: older, Ended: older.Add(time.Minute)}, + Git: &testui.SnapshotGit{SHA: "abcdef1234567890"}, + Tests: []parsers.Test{ + {Name: "TestA", Passed: true}, + {Name: "TestB", Failed: true}, + }, + } + snapNew := testui.Snapshot{ + Metadata: &testui.SnapshotMetadata{Started: newer, Ended: newer.Add(2 * time.Minute)}, + Git: &testui.SnapshotGit{SHA: "1111111deadbeef"}, + Tests: []parsers.Test{ + {Name: "TestC", Passed: true}, + {Name: "TestD", Skipped: true}, + {Name: "TestE", Passed: true}, + }, + } + + oldPath := filepath.Join(gavelDir, "sha-abcdef1234567890.json") + newPath := filepath.Join(gavelDir, "sha-1111111deadbeef.json") + writeTestSnapshot(t, oldPath, snapOld, older) + writeTestSnapshot(t, newPath, snapNew, newer) + // Pointer that resolves to the new snapshot. + writeTestPointer(t, filepath.Join(gavelDir, "last.json"), + filepath.Join(".gavel", "sha-1111111deadbeef.json"), "1111111deadbeef") + + srv := testui.NewServer() + srv.SetGavelDir(root) + handler := srv.Handler() + + resp := doRequest(t, handler, http.MethodGet, "/api/runs", nil) + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", resp.Code, resp.Body.String()) + } + + var entries []testui.RunIndexEntry + if err := json.NewDecoder(resp.Body).Decode(&entries); err != nil { + t.Fatalf("decode: %v", err) + } + + if len(entries) != 3 { + t.Fatalf("got %d entries, want 3: %+v", len(entries), entries) + } + + // Pointer first. + if entries[0].Pointer != "last" { + t.Fatalf("entries[0].Pointer = %q, want last", entries[0].Pointer) + } + if entries[0].SHA != "1111111" { + t.Fatalf("entries[0].SHA = %q, want 1111111", entries[0].SHA) + } + if entries[0].Counts == nil || entries[0].Counts.Total != 3 { + t.Fatalf("entries[0].Counts = %+v, want Total=3", entries[0].Counts) + } + + // Snapshots after, newest first by mtime. + if entries[1].Pointer != "" { + t.Fatalf("entries[1].Pointer = %q, want empty", entries[1].Pointer) + } + if entries[1].Name != "sha-1111111deadbeef" { + t.Fatalf("entries[1].Name = %q, want sha-1111111deadbeef", entries[1].Name) + } + if entries[2].Name != "sha-abcdef1234567890" { + t.Fatalf("entries[2].Name = %q, want sha-abcdef1234567890", entries[2].Name) + } + if entries[2].Counts == nil || entries[2].Counts.Failed != 1 || entries[2].Counts.Passed != 1 { + t.Fatalf("entries[2].Counts = %+v, want Failed=1 Passed=1", entries[2].Counts) + } +} + +func TestRunsIndexEndpoint404WhenDirNotSet(t *testing.T) { + srv := testui.NewServer() + handler := srv.Handler() + resp := doRequest(t, handler, http.MethodGet, "/api/runs", nil) + if resp.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", resp.Code) + } +} + +func TestRunSnapshotEndpointResolvesPointer(t *testing.T) { + root := t.TempDir() + gavelDir := filepath.Join(root, ".gavel") + if err := os.MkdirAll(gavelDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + snap := testui.Snapshot{ + Tests: []parsers.Test{{Name: "TestX", Passed: true}}, + } + target := filepath.Join(gavelDir, "sha-deadbeef.json") + writeTestSnapshot(t, target, snap, time.Now()) + writeTestPointer(t, filepath.Join(gavelDir, "last.json"), + filepath.Join(".gavel", "sha-deadbeef.json"), "deadbeef") + + srv := testui.NewServer() + srv.SetGavelDir(root) + handler := srv.Handler() + + resp := doRequest(t, handler, http.MethodGet, "/api/runs/last.json", nil) + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", resp.Code, resp.Body.String()) + } + var got testui.Snapshot + if err := json.NewDecoder(resp.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got.Tests) != 1 || got.Tests[0].Name != "TestX" { + t.Fatalf("got tests %+v, want [TestX]", got.Tests) + } +} + +func TestRunSnapshotEndpointRejectsTraversal(t *testing.T) { + root := t.TempDir() + gavelDir := filepath.Join(root, ".gavel") + if err := os.MkdirAll(gavelDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + srv := testui.NewServer() + srv.SetGavelDir(root) + handler := srv.Handler() + + for _, name := range []string{"..", "../etc/passwd", "foo/bar.json"} { + resp := doRequest(t, handler, http.MethodGet, "/api/runs/"+name, nil) + if resp.Code == http.StatusOK { + t.Fatalf("traversal %q returned 200; body=%s", name, resp.Body.String()) + } + } +} diff --git a/testrunner/ui/src/App.tsx b/testrunner/ui/src/App.tsx index 2ce375747..f249e3c6b 100644 --- a/testrunner/ui/src/App.tsx +++ b/testrunner/ui/src/App.tsx @@ -1,5 +1,5 @@ import { useState, useEffect, useRef, useMemo, useCallback } from 'preact/hooks'; -import type { Test, Snapshot, LinterResult, BenchComparison, DiagnosticsSnapshot, ProcessNode, ProcessDetails, RunMeta } from './types'; +import type { Test, Snapshot, LinterResult, BenchComparison, DiagnosticsSnapshot, ProcessNode, ProcessDetails, RunMeta, RunIndexEntry } from './types'; import { Summary } from './components/Summary'; import { TestNode } from './components/TestNode'; import { DetailPanel, type IgnoreRequest } from './components/DetailPanel'; @@ -10,6 +10,7 @@ import { LintFilterBar, type LintGrouping, type LintFilters } from './components import { LintView } from './components/LintView'; import { BenchView } from './components/BenchView'; import { RerunDialog } from './components/RerunDialog'; +import { RunIndex } from './components/RunIndex'; import { SplitPane } from './components/SplitPane'; import { copyCurrentViewForAgent, downloadCurrentView } from './export'; import { @@ -80,13 +81,15 @@ function applySnapshot( } function currentRouteState( + view: 'index' | 'run', + runName: string, tab: TabKey, selectedPath: string, filters: Filters, lintGrouping: LintGrouping, lintFilters: LintFilters, ): RouteState { - return { tab, selectedPath, filters, lintGrouping, lintFilters }; + return { view, runName, tab, selectedPath, filters, lintGrouping, lintFilters }; } function mergeProcessDetails(root: ProcessNode | undefined, details: ProcessDetails): ProcessNode | undefined { @@ -105,7 +108,9 @@ function mergeProcessDetails(root: ProcessNode | undefined, details: ProcessDeta } export function App() { - const initialRoute = typeof window !== 'undefined' ? parseRoute(window.location) : { + const initialRoute: RouteState = typeof window !== 'undefined' ? parseRoute(window.location) : { + view: 'run', + runName: '', tab: 'tests' as TabKey, selectedPath: '', filters: { status: defaultStatusFilter(), framework: new Map() }, @@ -124,6 +129,8 @@ export function App() { const [status, setStatus] = useState('Loading...'); const [expandAll, setExpandAll] = useState(null); const [filters, setFilters] = useState(initialRoute.filters); + const [activeView, setActiveView] = useState<'index' | 'run'>(initialRoute.view); + const [runName, setRunName] = useState(initialRoute.runName); const [activeTab, setActiveTab] = useState(initialRoute.tab); const [lintGrouping, setLintGrouping] = useState(initialRoute.lintGrouping); const [lintFilters, setLintFilters] = useState(initialRoute.lintFilters); @@ -142,11 +149,13 @@ export function App() { const copyResetTimer = useRef(null); const routeState = useMemo( - () => currentRouteState(activeTab, selectedPath, filters, lintGrouping, lintFilters), - [activeTab, selectedPath, filters, lintGrouping, lintFilters], + () => currentRouteState(activeView, runName, activeTab, selectedPath, filters, lintGrouping, lintFilters), + [activeView, runName, activeTab, selectedPath, filters, lintGrouping, lintFilters], ); const commitRoute = useCallback((next: RouteState, mode: 'push' | 'replace' = 'push') => { + setActiveView(next.view); + setRunName(next.runName); setActiveTab(next.tab); setSelectedPath(next.selectedPath); setFilters(next.filters); @@ -163,6 +172,8 @@ export function App() { useEffect(() => { const onPopState = () => { const next = parseRoute(window.location); + setActiveView(next.view); + setRunName(next.runName); setActiveTab(next.tab); setSelectedPath(next.selectedPath); setFilters(next.filters); @@ -183,6 +194,27 @@ export function App() { }, []); useEffect(() => { + if (activeView === 'index') return; + + // Static replay mode: fetch the named snapshot once, no SSE. + if (runName) { + fetch(apiUrl(`/api/runs/${encodeURIComponent(runName)}`)) + .then(async r => { + if (!r.ok) throw new Error(`HTTP ${r.status}: ${(await r.text()).trim()}`); + return r.json() as Promise; + }) + .then(snap => { + applySnapshot(snap, startTime, endTime, doneRef, setTests, setLint, setLintRun, setBench, setDiagnosticsAvailable, setDiagnostics, setRunMeta, setDone, setStatus); + doneRef.current = true; + setDone(true); + setStatus('Snapshot loaded'); + }) + .catch(e => { + setStatus(`Failed to load snapshot: ${e?.message || e}`); + }); + return; + } + if (streamToken === 0) { fetch(apiUrl('/api/tests')) .then(r => r.json()) @@ -217,7 +249,7 @@ export function App() { }, 1000); return () => { es.close(); clearInterval(timer); }; - }, [streamToken]); + }, [streamToken, activeView, runName]); const fetchDiagnostics = useCallback(async () => { const res = await fetch(apiUrl('/api/diagnostics')); @@ -323,6 +355,41 @@ export function App() { }); }, [routeState, commitRoute]); + const onIndexSelect = useCallback((entry: RunIndexEntry) => { + // Reset transient run-state so the new snapshot loads cleanly. + startTime.current = null; + endTime.current = null; + doneRef.current = false; + setTests([]); + setLint(undefined); + setLintRun(false); + setBench(undefined); + setDiagnosticsAvailable(false); + setDiagnostics(undefined); + setStatus('Loading snapshot...'); + commitRoute({ + view: 'run', + runName: entry.name, + tab: 'tests', + selectedPath: '', + filters: { status: defaultStatusFilter(), framework: new Map() }, + lintGrouping: 'linter-rule-file', + lintFilters: { severity: new Map(), linter: new Map() }, + }); + }, [commitRoute]); + + const onBackToIndex = useCallback(() => { + commitRoute({ + view: 'index', + runName: '', + tab: 'tests', + selectedPath: '', + filters: { status: defaultStatusFilter(), framework: new Map() }, + lintGrouping: 'linter-rule-file', + lintFilters: { severity: new Map(), linter: new Map() }, + }); + }, [commitRoute]); + const onSelect = useCallback((test: Test) => { const nextPath = test.route_path === selectedPath ? '' : (test.route_path || ''); commitRoute({ @@ -612,6 +679,22 @@ export function App() { const backTo = typeof window !== 'undefined' ? (window as any).__gavelBackTo as string | undefined : undefined; + if (activeView === 'index') { + return ( +
+ {backTo && ( +
+ + + Back to PR + +
+ )} + +
+ ); + } + return (
{backTo && ( @@ -622,6 +705,16 @@ export function App() {
)} + {runName && ( +
+ + / + {runName} +
+ )}
diff --git a/testrunner/ui/src/components/RunIndex.tsx b/testrunner/ui/src/components/RunIndex.tsx new file mode 100644 index 000000000..22344e783 --- /dev/null +++ b/testrunner/ui/src/components/RunIndex.tsx @@ -0,0 +1,180 @@ +import { useEffect, useState } from 'preact/hooks'; +import type { RunIndexEntry } from '../types'; +import { apiUrl } from '../config'; + +interface Props { + onSelect: (entry: RunIndexEntry) => void; +} + +function relativeTime(iso: string): string { + const t = Date.parse(iso); + if (Number.isNaN(t)) return ''; + const diff = Date.now() - t; + if (diff < 60_000) return 'just now'; + if (diff < 3_600_000) return `${Math.floor(diff / 60_000)} min ago`; + if (diff < 86_400_000) return `${Math.floor(diff / 3_600_000)} h ago`; + if (diff < 7 * 86_400_000) return `${Math.floor(diff / 86_400_000)} d ago`; + return new Date(t).toISOString().slice(0, 16).replace('T', ' '); +} + +function formatDuration(startedISO: string | undefined, endedISO: string | undefined): string { + if (!startedISO || !endedISO) return ''; + const start = Date.parse(startedISO); + const end = Date.parse(endedISO); + if (Number.isNaN(start) || Number.isNaN(end) || end < start) return ''; + const ms = end - start; + if (ms < 1000) return `${ms} ms`; + if (ms < 60_000) return `${(ms / 1000).toFixed(1)} s`; + const m = Math.floor(ms / 60_000); + const s = Math.floor((ms % 60_000) / 1000); + return `${m}m ${s}s`; +} + +function entryKey(entry: RunIndexEntry): string { + return entry.pointer ? `pointer:${entry.pointer}` : `name:${entry.name}`; +} + +export function RunIndex({ onSelect }: Props) { + const [entries, setEntries] = useState(null); + const [error, setError] = useState(''); + + useEffect(() => { + let cancelled = false; + fetch(apiUrl('/api/runs')) + .then(async r => { + if (!r.ok) throw new Error(`HTTP ${r.status}: ${(await r.text()).trim()}`); + return r.json() as Promise; + }) + .then(data => { if (!cancelled) setEntries(data); }) + .catch(e => { if (!cancelled) setError(e?.message || String(e)); }); + return () => { cancelled = true; }; + }, []); + + return ( +
+
+
+

+ + Saved Runs +

+ {entries && ( + + {entries.length} {entries.length === 1 ? 'snapshot' : 'snapshots'} in .gavel/ + + )} +
+ + {error && ( +
+ Failed to load run index: {error} +
+ )} + + {!entries && !error && ( +
+ +

Loading runs...

+
+ )} + + {entries && entries.length === 0 && ( +
+ No snapshots found in .gavel/. Run gavel test to populate it. +
+ )} + + {entries && entries.length > 0 && ( +
+ + + + + + + + + + + + {entries.map(entry => ( + + ))} + +
RunModifiedTestsLintDuration
+
+ )} +
+
+ ); +} + +interface RowProps { + entry: RunIndexEntry; + onSelect: (entry: RunIndexEntry) => void; +} + +function RunIndexRow({ entry, onSelect }: RowProps) { + const counts = entry.counts; + const failed = counts?.failed ?? 0; + const passed = counts?.passed ?? 0; + const skipped = counts?.skipped ?? 0; + const pending = counts?.pending ?? 0; + const total = counts?.total ?? 0; + const lint = entry.lint ?? 0; + + return ( + onSelect(entry)} + > + +
+ {entry.pointer ? ( + + + {entry.pointer} + + ) : ( + + )} + + {entry.pointer ? entry.name : entry.name} + + {entry.sha && ( + @{entry.sha} + )} +
+ {entry.error && ( +
{entry.error}
+ )} + + + {relativeTime(entry.modified)} + + + {total > 0 ? ( + + {failed > 0 && {failed} failed} + {failed > 0 && passed > 0 && ·} + {passed > 0 && {passed} passed} + {skipped > 0 && <>·{skipped} skipped} + {pending > 0 && <>·{pending} pending} + + ) : ( + — + )} + + + {lint > 0 ? ( + {lint} + ) : ( + — + )} + + + {formatDuration(entry.started, entry.ended) || —} + + + ); +} diff --git a/testrunner/ui/src/routes.test.ts b/testrunner/ui/src/routes.test.ts index 701423b6e..9844145a4 100644 --- a/testrunner/ui/src/routes.test.ts +++ b/testrunner/ui/src/routes.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from 'vitest'; -import { buildRoute, defaultStatusFilter, parseRoute } from './routes'; + +// vitest runs in node by default; routes.ts -> config.ts touches `window` +// at import time. Stub it before importing. +(globalThis as any).window = (globalThis as any).window || {}; + +const { buildRoute, defaultStatusFilter, parseRoute } = await import('./routes'); describe('lint route grouping', () => { it('defaults lint routes to linter-rule-file grouping', () => { @@ -17,6 +22,8 @@ describe('lint route grouping', () => { it('omits the default grouping from lint URLs and keeps legacy grouping params', () => { const baseState = { + view: 'run' as const, + runName: '', tab: 'lint' as const, selectedPath: '', filters: { status: defaultStatusFilter(), framework: new Map() }, @@ -34,3 +41,58 @@ describe('lint route grouping', () => { })).toBe('/lint?grouping=linter-file'); }); }); + +describe('run index routes', () => { + it('parses / as the index view', () => { + const route = parseRoute(new URL('http://example.test/') as unknown as Location); + expect(route.view).toBe('index'); + expect(route.runName).toBe(''); + }); + + it('parses /run/ as the run view with default tab', () => { + const route = parseRoute(new URL('http://example.test/run/sha-abc123') as unknown as Location); + expect(route.view).toBe('run'); + expect(route.runName).toBe('sha-abc123'); + expect(route.tab).toBe('tests'); + }); + + it('parses /run//lint/?status=failed', () => { + const route = parseRoute(new URL('http://example.test/run/last/lint/golangci?status=failed') as unknown as Location); + expect(route.view).toBe('run'); + expect(route.runName).toBe('last'); + expect(route.tab).toBe('lint'); + expect(route.selectedPath).toBe('golangci'); + }); + + it('builds /run// for run-detail URLs', () => { + expect(buildRoute({ + view: 'run', + runName: 'sha-abc123', + tab: 'tests', + selectedPath: '', + filters: { status: defaultStatusFilter(), framework: new Map() }, + lintGrouping: 'linter-rule-file', + lintFilters: { severity: new Map(), linter: new Map() }, + })).toBe('/run/sha-abc123/tests'); + }); + + it('builds / for the index view regardless of other state', () => { + expect(buildRoute({ + view: 'index', + runName: '', + tab: 'tests', + selectedPath: '', + filters: { status: defaultStatusFilter(), framework: new Map() }, + lintGrouping: 'linter-rule-file', + lintFilters: { severity: new Map(), linter: new Map() }, + })).toBe('/'); + }); + + it('keeps backward-compat: /tests still parses as a run view with empty runName', () => { + const route = parseRoute(new URL('http://example.test/tests/foo') as unknown as Location); + expect(route.view).toBe('run'); + expect(route.runName).toBe(''); + expect(route.tab).toBe('tests'); + expect(route.selectedPath).toBe('foo'); + }); +}); diff --git a/testrunner/ui/src/routes.ts b/testrunner/ui/src/routes.ts index 216191b6f..051cba4c5 100644 --- a/testrunner/ui/src/routes.ts +++ b/testrunner/ui/src/routes.ts @@ -22,8 +22,14 @@ function isDefaultStatusFilter(state: FilterState): boolean { } export type TabKey = 'tests' | 'lint' | 'bench' | 'diagnostics'; +export type ViewKey = 'index' | 'run'; export interface RouteState { + view: ViewKey; + // runName identifies which .gavel/.json the detail view should fetch. + // Empty string means "use the in-memory snapshot" — the case when `gavel ui + // serve ` was launched with an explicit path. + runName: string; tab: TabKey; selectedPath: string; filters: Filters; @@ -45,12 +51,27 @@ export function parseRoute(location: Location): RouteState { } const trimmed = pathname.replace(/^\/+|\/+$/g, ''); const segments = trimmed ? trimmed.split('/').map(decodeURIComponent) : []; + let view: ViewKey = 'run'; + let runName = ''; let tab: TabKey = 'tests'; let selectedPath = ''; + let tabSegments = segments; + + if (segments.length === 0) { + view = 'index'; + tabSegments = []; + } else if (segments[0] === 'run') { + runName = segments[1] || ''; + if (!runName) { + // /run with no name behaves like the index — defensive against half-typed URLs. + view = 'index'; + } + tabSegments = segments.slice(2); + } - if (segments[0] === 'tests' || segments[0] === 'lint' || segments[0] === 'bench' || segments[0] === 'diagnostics') { - tab = segments[0]; - selectedPath = segments.slice(1).join('/'); + if (tabSegments[0] === 'tests' || tabSegments[0] === 'lint' || tabSegments[0] === 'bench' || tabSegments[0] === 'diagnostics') { + tab = tabSegments[0]; + selectedPath = tabSegments.slice(1).join('/'); } const params = new URLSearchParams(location.search); @@ -64,6 +85,8 @@ export function parseRoute(location: Location): RouteState { status = decodeFilterState(splitCSV(rawStatus)); } return { + view, + runName, tab, selectedPath, filters: { @@ -79,7 +102,13 @@ export function parseRoute(location: Location): RouteState { } export function buildRoute(state: RouteState): string { - const segments: string[] = [state.tab]; + if (state.view === 'index') { + return `${basePath}/`; + } + + const segments: string[] = []; + if (state.runName) segments.push('run', encodeURIComponent(state.runName)); + segments.push(state.tab); if (state.selectedPath) segments.push(...state.selectedPath.split('/').map(encodeURIComponent)); const params = new URLSearchParams(); diff --git a/testrunner/ui/src/types.ts b/testrunner/ui/src/types.ts index 2ed6cb33e..458794631 100644 --- a/testrunner/ui/src/types.ts +++ b/testrunner/ui/src/types.ts @@ -188,3 +188,24 @@ export interface FixtureContext { expected?: any; actual?: any; } + +export interface RunCounts { + total: number; + passed: number; + failed: number; + skipped: number; + pending: number; +} + +export interface RunIndexEntry { + name: string; + path: string; + pointer?: string; + modified: string; + sha?: string; + started?: string; + ended?: string; + counts?: RunCounts; + lint?: number; + error?: string; +}