diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index fc4d54c..0515917 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -3,8 +3,6 @@ name: CodeQL on: push: branches: [main] - pull_request: - branches: [main] schedule: - cron: '31 7 * * 3' diff --git a/internal/commands/commands_test.go b/internal/commands/commands_test.go index 786d3fb..f547c98 100644 --- a/internal/commands/commands_test.go +++ b/internal/commands/commands_test.go @@ -218,6 +218,7 @@ func TestNewLintCommand_Flags(t *testing.T) { {"deadcode-exclude", "string"}, {"no-gofumpt", "bool"}, {"no-golangci", "bool"}, + {"new-from-rev", "string"}, // Shared linter config flags {"max-line-width", "int"}, {"tab-width", "int"}, @@ -247,6 +248,7 @@ func TestNewLintCommand_FlagDefaults(t *testing.T) { stringDefaults := map[string]string{ "paths": "", "deadcode-exclude": "", + "new-from-rev": "", "exclude": "", "rule": "", } diff --git a/internal/commands/lint.go b/internal/commands/lint.go index 0b87715..969530d 100644 --- a/internal/commands/lint.go +++ b/internal/commands/lint.go @@ -51,6 +51,10 @@ func bindLintFlags(cmd *cobra.Command, opts *lint.LintOpts) { ) fl.BoolVar(&opts.NoGofumpt, "no-gofumpt", false, "skip gofumpt") fl.BoolVar(&opts.NoGolangci, "no-golangci", false, "skip golangci-lint") + fl.StringVar( + &opts.NewFromRev, "new-from-rev", "", + "only report new golangci-lint issues since this git revision", + ) } // bindLinterConfigFlags binds the shared linter configuration diff --git a/internal/services/covgate/covgate.go b/internal/services/covgate/covgate.go index 6e3519f..2896feb 100644 --- a/internal/services/covgate/covgate.go +++ b/internal/services/covgate/covgate.go @@ -7,6 +7,7 @@ import ( "runtime" "strings" "sync" + "time" "github.com/mirurobotics/gotools/internal/services/gocover" ) @@ -99,14 +100,16 @@ func (r *runner) runPackages( func (r *runner) printResults(w io.Writer, results []checkResult) error { hasFailures := false + var total time.Duration for _, res := range results { _, _ = fmt.Fprint(w, res.output) + total += res.duration if !res.passed { hasFailures = true } } - _, _ = fmt.Fprintln(w) + _, _ = fmt.Fprintf(w, "\nTotal time: %s\n", fmtDuration(total)) if hasFailures { _, _ = fmt.Fprintln( w, "ERROR: One or more packages failed "+ @@ -121,19 +124,20 @@ func (r *runner) printResults(w io.Writer, results []checkResult) error { func printHeader(w io.Writer) { _, _ = fmt.Fprintf( - w, "%-6s %8s %8s %s\n", - "STATUS", "COVERAGE", "REQUIRED", "PACKAGE", + w, "%-6s %8s %8s %8s %s\n", + "STATUS", "COVERAGE", "REQUIRED", "TIME", "PACKAGE", ) _, _ = fmt.Fprintf( - w, "%-6s %8s %8s %s\n", - "------", "--------", "--------", "-------", + w, "%-6s %8s %8s %8s %s\n", + "------", "--------", "--------", "--------", "-------", ) } // checkResult holds the output and pass/fail status for a single package check. type checkResult struct { - output string // formatted line(s) to print - passed bool + output string // formatted line(s) to print + passed bool + duration time.Duration } // checkPackageCtx holds the per-run constants passed to checkPackage. @@ -151,18 +155,21 @@ func (r *runner) checkPackage(pkg string, ctx checkPackageCtx) checkResult { testPaths := gocover.BuildTestPaths(pkg, relPkg, ctx.srcPrefix, ctx.testDir) - var b strings.Builder + start := time.Now() coverage, output, testErr := r.measure(pkg, testPaths) + elapsed := time.Since(start) + + var b strings.Builder if testErr != nil { _, _ = fmt.Fprintf( - &b, "%-6s %8s %8s %s\n", - "FAIL", "---", "---", + &b, "%-6s %8s %8s %8s %s\n", + "FAIL", "---", "---", fmtDuration(elapsed), relPkg+" (tests failed)", ) _, _ = fmt.Fprintln(&b) _, _ = fmt.Fprint(&b, string(output)) _, _ = fmt.Fprintln(&b) - return checkResult{output: b.String(), passed: false} + return checkResult{b.String(), false, elapsed} } status := "PASS" @@ -170,8 +177,18 @@ func (r *runner) checkPackage(pkg string, ctx checkPackageCtx) checkResult { status = "FAIL" } _, _ = fmt.Fprintf( - &b, "%-6s %7.1f%% %7.1f%% %s\n", - status, coverage, threshold, relPkg, + &b, "%-6s %7.1f%% %7.1f%% %8s %s\n", + status, coverage, threshold, fmtDuration(elapsed), relPkg, ) - return checkResult{output: b.String(), passed: coverage >= threshold} + return checkResult{b.String(), coverage >= threshold, elapsed} +} + +func fmtDuration(d time.Duration) string { + d = d.Round(100 * time.Millisecond) + if d < time.Minute { + return fmt.Sprintf("%.1fs", d.Seconds()) + } + m := int(d.Minutes()) + s := d - time.Duration(m)*time.Minute + return fmt.Sprintf("%dm%02.0fs", m, s.Seconds()) } diff --git a/internal/services/covgate/covgate_test.go b/internal/services/covgate/covgate_test.go index 4a79e7f..2c72b89 100644 --- a/internal/services/covgate/covgate_test.go +++ b/internal/services/covgate/covgate_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/mirurobotics/gotools/internal/services/gocover" "github.com/mirurobotics/gotools/internal/testutil" @@ -17,7 +18,7 @@ func TestPrintHeader(t *testing.T) { printHeader(&buf) out := buf.String() - cols := []string{"STATUS", "COVERAGE", "REQUIRED", "PACKAGE"} + cols := []string{"STATUS", "COVERAGE", "REQUIRED", "TIME", "PACKAGE"} for _, col := range cols { if !strings.Contains(out, col) { t.Errorf("output missing column %q", col) @@ -373,3 +374,64 @@ func TestRun_GoListError(t *testing.T) { t.Errorf("unexpected error: %v", err) } } + +func TestFmtDuration(t *testing.T) { + tests := []struct { + d time.Duration + want string + }{ + {0, "0.0s"}, + {500 * time.Millisecond, "0.5s"}, + {3200 * time.Millisecond, "3.2s"}, + {60 * time.Second, "1m00s"}, + {7*time.Minute + 45*time.Second, "7m45s"}, + } + for _, tt := range tests { + if got := fmtDuration(tt.d); got != tt.want { + t.Errorf("fmtDuration(%v) = %q, want %q", tt.d, got, tt.want) + } + } +} + +func TestRun_OutputContainsTiming(t *testing.T) { + testutil.MakePkgDir(t, "pkg/a") + + var buf bytes.Buffer + //nolint:exhaustruct // test uses partial initialization + r := runner{ + goModule: func() (string, error) { return modName, nil }, + goListPackages: func(string) ([]string, error) { + return []string{"example.com/mod/pkg/a"}, nil + }, + measure: fakeMeasure(90.0), + } + + //nolint:exhaustruct // test uses partial initialization + err := r.run(Opts{Out: &buf, DefaultThreshold: 80.0}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + s := buf.String() + if !strings.Contains(s, "TIME") { + t.Errorf("output missing TIME column header: %s", s) + } + if !strings.Contains(s, "Total time:") { + t.Errorf("output missing total time: %s", s) + } +} + +func TestCheckPackage_OutputContainsTime(t *testing.T) { + testutil.MakePkgDir(t, pkgRel) + + //nolint:exhaustruct // test uses partial initialization + r := runner{measure: fakeMeasure(85.0)} + + //nolint:exhaustruct // test uses partial initialization + res := r.checkPackage(pkgName, checkPackageCtx{module: modName, threshold: 80.0}) + if !strings.Contains(res.output, "0.0s") { + t.Errorf("output missing duration: %s", res.output) + } + if res.duration < 0 { + t.Errorf("expected non-negative duration, got %v", res.duration) + } +} diff --git a/internal/services/lint/lint.go b/internal/services/lint/lint.go index 745b840..2430bfb 100644 --- a/internal/services/lint/lint.go +++ b/internal/services/lint/lint.go @@ -7,11 +7,17 @@ import ( "os" "os/exec" "strings" + "time" "github.com/mirurobotics/gotools/internal/services/cmdutil" "github.com/mirurobotics/gotools/internal/services/lint/linter" ) +type stepTiming struct { + name string + duration time.Duration +} + // LintOpts holds the options for the lint orchestrator. type LintOpts struct { Paths string @@ -21,6 +27,7 @@ type LintOpts struct { DeadcodeExclude string NoGofumpt bool NoGolangci bool + NewFromRev string Out io.Writer Err io.Writer } @@ -35,12 +42,30 @@ func RunLint(opts LintOpts) error { opts.Err = os.Stderr } - var failures []string + totalStart := time.Now() + failures, timings, fatal := runLintSteps(opts) + printTimings(opts.Out, timings, time.Since(totalStart)) + if fatal != nil { + return fatal + } + if len(failures) > 0 { + return fmt.Errorf("lint failed: %s", strings.Join(failures, ", ")) + } + + _, _ = fmt.Fprintln(opts.Out, "Lint complete") + return nil +} + +func runLintSteps( + opts LintOpts, +) (failures []string, timings []stepTiming, fatal error) { if opts.Paths != "" { + start := time.Now() hadIssues, err := runCustomLinter(opts) + timings = append(timings, stepTiming{"custom linter", time.Since(start)}) if err != nil { - return err + return failures, timings, err } if hadIssues { failures = append(failures, "custom linter") @@ -48,29 +73,53 @@ func RunLint(opts LintOpts) error { } if !opts.NoGofumpt { - if err := RunGofumpt(opts.Out, opts.Err, opts.DoFix); err != nil { - return fmt.Errorf("gofumpt: %w", err) + start := time.Now() + err := RunGofumpt(opts.Out, opts.Err, opts.DoFix) + timings = append(timings, stepTiming{"gofumpt", time.Since(start)}) + if err != nil { + return failures, timings, fmt.Errorf("gofumpt: %w", err) } } if !opts.NoGolangci { - if err := RunGolangci(opts.Out, opts.Err); err != nil { + start := time.Now() + err := RunGolangci(opts.Out, opts.Err, opts.NewFromRev) + timings = append(timings, stepTiming{"golangci-lint", time.Since(start)}) + if err != nil { failures = append(failures, "golangci-lint") } } if opts.Deadcode { - if err := RunDeadcode(opts.Out, opts.Err, opts.DeadcodeExclude); err != nil { + start := time.Now() + err := RunDeadcode(opts.Out, opts.Err, opts.DeadcodeExclude) + timings = append(timings, stepTiming{"deadcode", time.Since(start)}) + if err != nil { failures = append(failures, "deadcode") } } - if len(failures) > 0 { - return fmt.Errorf("lint failed: %s", strings.Join(failures, ", ")) + return failures, timings, nil +} + +func printTimings(w io.Writer, timings []stepTiming, total time.Duration) { + _, _ = fmt.Fprintln(w) + _, _ = fmt.Fprintln(w, "Timings") + _, _ = fmt.Fprintln(w, "-------") + for _, t := range timings { + _, _ = fmt.Fprintf(w, " %-16s %s\n", t.name, fmtDuration(t.duration)) } + _, _ = fmt.Fprintf(w, " %-16s %s\n", "total", fmtDuration(total)) +} - _, _ = fmt.Fprintln(opts.Out, "\nLint complete") - return nil +func fmtDuration(d time.Duration) string { + d = d.Round(100 * time.Millisecond) + if d < time.Minute { + return fmt.Sprintf("%.1fs", d.Seconds()) + } + m := int(d.Minutes()) + s := d - time.Duration(m)*time.Minute + return fmt.Sprintf("%dm%02.0fs", m, s.Seconds()) } func runCustomLinter(opts LintOpts) (bool, error) { @@ -103,10 +152,16 @@ func runCustomLinter(opts LintOpts) (bool, error) { return totalDiags > 0, nil } -// RunGolangci runs golangci-lint. -func RunGolangci(out io.Writer, errW io.Writer) error { +// RunGolangci runs golangci-lint. If newFromRev is +// non-empty, only new issues since that revision are +// reported. +func RunGolangci(out io.Writer, errW io.Writer, newFromRev string) error { _, _ = fmt.Fprintln(out, "Running golangci-lint...") - if err := RunExternal(out, errW, "go", "tool", "golangci-lint", "run"); err != nil { + args := []string{"tool", "golangci-lint", "run"} + if newFromRev != "" { + args = append(args, "--new-from-rev="+newFromRev) + } + if err := RunExternal(out, errW, "go", args...); err != nil { _, _ = fmt.Fprintf(errW, "golangci-lint failed: %v\n", err) return err } diff --git a/internal/services/lint/lint_test.go b/internal/services/lint/lint_test.go index ce59346..75e8fc9 100644 --- a/internal/services/lint/lint_test.go +++ b/internal/services/lint/lint_test.go @@ -5,6 +5,7 @@ import ( "io" "strings" "testing" + "time" ) func TestFilterDeadcodeOutput(t *testing.T) { @@ -92,8 +93,15 @@ func TestRunLint_AllSkipped(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(out.String(), "Lint complete") { - t.Errorf("expected 'Lint complete' in output, got %q", out.String()) + s := out.String() + if !strings.Contains(s, "Lint complete") { + t.Errorf("expected 'Lint complete' in output, got %q", s) + } + if !strings.Contains(s, "Timings") { + t.Errorf("expected timing summary in output, got %q", s) + } + if !strings.Contains(s, "total") { + t.Errorf("expected total timing in output, got %q", s) } } @@ -194,10 +202,67 @@ func TestRunLint_EmptyPaths(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if strings.Contains(out.String(), "custom linter") { + s := out.String() + if strings.Contains(s, "custom linter") { t.Error("custom linter should not have run with empty Paths") } - if !strings.Contains(out.String(), "Lint complete") { - t.Errorf("expected 'Lint complete' in output, got %q", out.String()) + if !strings.Contains(s, "Lint complete") { + t.Errorf("expected 'Lint complete' in output, got %q", s) + } +} + +func TestFmtDuration_Seconds(t *testing.T) { + tests := []struct { + d time.Duration + want string + }{ + {0, "0.0s"}, + {500 * time.Millisecond, "0.5s"}, + {3200 * time.Millisecond, "3.2s"}, + {59*time.Second + 900*time.Millisecond, "59.9s"}, + } + for _, tt := range tests { + got := fmtDuration(tt.d) + if got != tt.want { + t.Errorf("fmtDuration(%v) = %q, want %q", tt.d, got, tt.want) + } + } +} + +func TestFmtDuration_Minutes(t *testing.T) { + tests := []struct { + d time.Duration + want string + }{ + {60 * time.Second, "1m00s"}, + {7*time.Minute + 45*time.Second, "7m45s"}, + } + for _, tt := range tests { + got := fmtDuration(tt.d) + if got != tt.want { + t.Errorf("fmtDuration(%v) = %q, want %q", tt.d, got, tt.want) + } + } +} + +func TestPrintTimings(t *testing.T) { + var buf bytes.Buffer + timings := []stepTiming{ + {"gofumpt", 400 * time.Millisecond}, + {"golangci-lint", 7*time.Minute + 45*time.Second}, + } + printTimings(&buf, timings, 8*time.Minute+10*time.Second) + s := buf.String() + if !strings.Contains(s, "Timings") { + t.Error("missing Timings header") + } + if !strings.Contains(s, "gofumpt") { + t.Error("missing gofumpt timing") + } + if !strings.Contains(s, "golangci-lint") { + t.Error("missing golangci-lint timing") + } + if !strings.Contains(s, "total") { + t.Error("missing total timing") } } diff --git a/plans/completed/timing-and-new-from-rev.md b/plans/completed/timing-and-new-from-rev.md new file mode 100644 index 0000000..5501d10 --- /dev/null +++ b/plans/completed/timing-and-new-from-rev.md @@ -0,0 +1,76 @@ +# ExecPlan: Timing Output + --new-from-rev for golangci-lint + +## Goal + +Two enhancements to the `miru` CLI: + +1. **Timing output** — Show elapsed time for each lint step and each covgate package measurement so users can identify bottlenecks. +2. **`--new-from-rev` flag** — Allow `miru lint` to pass `--new-from-rev=` to golangci-lint, enabling differential linting on PRs. + +## Milestones + +### M1: Lint step timing + +**Files:** +- `internal/services/lint/lint.go` — Add timing around each step in `RunLint`, print summary. +- `internal/services/lint/lint_test.go` — Update tests that assert output content. + +**Steps:** +1. Add `"time"` import to `lint.go`. +2. In `RunLint`, record `time.Now()` before each step (custom linter, gofumpt, golangci-lint, deadcode) and compute duration after. +3. Collect step timings into a slice of `{name, duration}` pairs (only for steps that actually ran). +4. After the failure check, print a timing summary section: + ``` + Timings + ------- + custom linter 3.2s + gofumpt 0.4s + golangci-lint 7m45s + deadcode 1.1s + ``` +5. Update the `"Lint complete"` message to also include total time. +6. Update tests: `TestRunLint_AllSkipped` and `TestRunLint_EmptyPaths` currently check for `"Lint complete"` — they should still pass since no steps ran and timings section is still printed (empty or with 0 entries). + +### M2: Covgate per-package timing + +**Files:** +- `internal/services/covgate/covgate.go` — Add duration to results, TIME column, total time. +- `internal/services/covgate/covgate_test.go` — Update tests that assert output format. + +**Steps:** +1. Add `"time"` import to `covgate.go`. +2. Add `duration time.Duration` field to `checkResult` struct. +3. In `checkPackage`, wrap the `r.measure()` call with `time.Now()` / `time.Since()`, store in result. +4. Update `printHeader` to add a `TIME` column. +5. Update the format strings in `checkPackage` (both pass and fail paths) to include the duration. +6. In `printResults`, compute and print total elapsed time from sum of all durations. +7. Update `TestPrintHeader` to check for the new `TIME` column. +8. Update `TestCheckPackage_*` tests — the `fakeMeasure` function returns instantly, so durations will be ~0. Assertions on output format should account for the TIME column. + +### M3: --new-from-rev flag for golangci-lint + +**Files:** +- `internal/services/lint/lint.go` — Add `NewFromRev` to `LintOpts`, modify `RunGolangci` signature. +- `internal/commands/lint.go` — Bind the new flag. +- `internal/services/lint/lint_test.go` — Add test for the new flag plumbing. + +**Steps:** +1. Add `NewFromRev string` field to `LintOpts`. +2. Change `RunGolangci(out, errW io.Writer)` signature to `RunGolangci(out, errW io.Writer, newFromRev string)`. +3. In `RunGolangci`, build the args slice conditionally: `["go", "tool", "golangci-lint", "run"]` + `["--new-from-rev="]` if non-empty. +4. Update the call site in `RunLint` to pass `opts.NewFromRev`. +5. In `internal/commands/lint.go`, add `fl.StringVar(&opts.NewFromRev, "new-from-rev", "", ...)` in `bindLintFlags`. +6. Add a test that verifies the flag is wired (command-level test or unit test). + +## Validation + +- All existing tests pass: `go test ./...` from the gotools root. +- New tests cover the timing output format and `--new-from-rev` flag wiring. +- Preflight must report clean before changes are published. + +## Test Steps + +1. Run `go test ./internal/services/lint/...` — all pass. +2. Run `go test ./internal/services/covgate/...` — all pass. +3. Run `go test ./...` — full suite passes. +4. Manual smoke: `go run ./cmd/miru lint --no-golangci --no-gofumpt` shows timing output.