diff --git a/CHANGELOG.md b/CHANGELOG.md index b989c4a..4a90e9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Integration tests against the real GitHub API (`make integration`, build tag `integration`), run nightly by a new `Integration` workflow. They fail against the old status-only logic. ### Fixed +- A check whose workflow ran more than once for the same commit (e.g. an older run cancelled by `concurrency: cancel-in-progress`) kept the PR failed even after the newer run passed. Only the newest run of each check counts now, like on GitHub. +- `prw wait` and the MCP `wait_for_ci` tool could return the final result of the previous commit when a new one was pushed during a poll. The PR head is now re-checked before returning, and waiting continues on the new commit. +- `prw wait --timeout`, the MCP `timeout_seconds` / cancellation and Ctrl-C now cancel the GitHub requests in flight; on a stalled network they used to be noticed only after the whole poll finished (up to 15 seconds per request). `prw run` also stops at once on Ctrl-C. +- PRs auto-watched with `--review-requested` that were merged or closed while `prw` wasn't running are no longer dropped as withdrawn review requests: they get their final `merged`/`closed` notification first. +- A `no_ci` PR no longer flaps to `pending` (with a "no_ci → pending" notification) when someone comments or changes a label; it stays `no_ci` until an actual check or status appears. +- The ETag cache no longer clears itself entirely when full, which made almost every request unconditional when watching many PRs; it evicts one entry instead. +- GovSlack webhook URLs (`hooks.slack-gov.com`) are detected as Slack. +- PRs without any CI are reported as `no_ci` instead of `pending` forever. Previously `prw wait` and the MCP `wait_for_ci` tool blocked until their timeout on such PRs (repositories without CI, or stacked PRs whose base branch runs none). A PR is `no_ci` once it has no checks or statuses at all and hasn't been updated for 3 minutes, so CI that is still registering after a push isn't mistaken for missing CI. `prw wait` exits `4` for it; `prw run` doesn't notify about it. - PR owner and repository names are compared case-insensitively (as on GitHub), so `DevBlac/PRW#5` and `devblac/prw#5` are no longer two separate watch-list entries. - Requests without a token no longer send an empty `Authorization: Bearer ` header, which GitHub rejects with 401 even for public repositories. - `MultiNotifier` (used whenever more than one notification channel is enabled) stopped at the first failing notifier. A down webhook silently suppressed the console and native notifications that were supposed to run after it; now every notifier always runs and their errors are combined. diff --git a/README.md b/README.md index 669de8f..739db12 100644 --- a/README.md +++ b/README.md @@ -238,6 +238,7 @@ prw wait "$PR_URL" --timeout 15m --json | `1` | CI failed (`failure` or `error`) | | `2` | Timed out (`--timeout`, default 30m) while CI was still pending | | `3` | Other error: invalid URL, API error, missing token, PR closed without merging | +| `4` | The PR has **no CI**: no checks or statuses were reported within 3 minutes of its last update (see below) | | `130` | Interrupted (Ctrl+C) | It fails fast — as soon as one check fails it exits `1` with the failed checks and their links, without waiting for the rest. If someone pushes to the PR meanwhile, it follows the new head commit. Transient errors (network, 5xx, rate limits) are retried up to 3 times in a row. Polls every 15s by default (`--interval`). @@ -260,7 +261,9 @@ With `--json`, stdout is a single object: } ``` -`state` is one of `success`, `failure`, `error`, `pending` (with `timed_out` or `interrupted` set), `merged`, or `closed`. +`state` is one of `success`, `failure`, `error`, `pending` (with `timed_out` or `interrupted` set), `merged`, `closed`, or `no_ci`. + +**PRs without CI.** Some PRs never get a single check — the repository has no CI, or none runs for the PR's base branch (for example a stacked PR whose base isn't `main`). Right after a push, though, every PR briefly has zero checks while CI registers them. So prw only concludes `no_ci` once the PR has had no checks or statuses at all **and** hasn't been updated for 3 minutes; until then it reports `pending`. `no_ci` exits with `4`, not `0`, so `prw wait && deploy` doesn't deploy a PR that no CI looked at. `prw run` shows `no_ci` in `prw list` but doesn't send a notification for it, and notifies normally if CI shows up later (e.g. after the PR is retargeted). ## Using prw from AI agents: `prw mcp` @@ -286,7 +289,7 @@ Tools: | Tool | What it does | |------|--------------| -| `get_ci_status` | Current state of a PR (`success`, `failure`, `error`, `pending`, `merged`, `closed`), check counts, failed checks with links. Returns immediately. | +| `get_ci_status` | Current state of a PR (`success`, `failure`, `error`, `pending`, `merged`, `closed`, `no_ci`), check counts, failed checks with links. Returns immediately. | | `wait_for_ci` | Blocks until CI finishes (`timeout_seconds` default 600, max 3600). Fails fast on the first failed check, follows new pushes, sends progress notifications. On timeout it returns `pending` with `timed_out: true` so the agent can call it again. | | `list_watched` | PRs on the local watch list with their last known state. | | `watch_pr` / `unwatch_pr` | Add or remove a PR from the watch list (a running `prw run` picks it up). | @@ -384,7 +387,7 @@ If you configure a `webhook_url`, `prw` will POST to it on every status change. | URL | Format | What you get | |-----|--------|--------------| -| `https://hooks.slack.com/...` | `slack` | A message with the PR link, status transition and failed checks | +| `https://hooks.slack.com/...` (or GovSlack `hooks.slack-gov.com`) | `slack` | A message with the PR link, status transition and failed checks | | `https://discord.com/api/webhooks/...` | `discord` | A colored embed (green / red / yellow) with the same details | | anything else | `generic` | Raw JSON (below), for your own services | diff --git a/TASKS.md b/TASKS.md index fdbec24..6c34e5a 100644 --- a/TASKS.md +++ b/TASKS.md @@ -89,7 +89,9 @@ Legend — Impact: 1–5 · Effort: S (≤½ day), M (1–2 days), L (3+ days) - ✅ #14 MCP server — done (`prw mcp`: 5 tools, progress + cancellation, protocol 2025-06-18/2025-03-26/2024-11-05; verified end-to-end over stdio against the real API with the token from `gh auth token`). Not yet tried inside a real MCP client. - ✅ #10 `prw watch --mine` / `--review-requested` — done (search-backed subscriptions refreshed every 5 min by `prw run`; verified live: 18 open PRs across 7 repos found, CI checked, cleanly removed by `unwatch --mine`) - 🆕 #18 **Report "no CI" instead of `pending` forever** (found while testing #10: 14 of 18 real PRs have zero checks — repos without CI, or stacked PRs whose base isn't `main`). Today `prw wait` / `wait_for_ci` would wait until the timeout on them. Needs a grace period after the last push (CI takes a moment to register checks) before concluding "no CI". Impact 4, effort S–M. -- Next: #18, then #11 review & mergeability events +- ✅ #18 `no_ci` state — done (0 checks + PR not updated for 3 min → `no_ci`; `prw wait` exits 4; no notification; verified live: 11 real PRs went from `pending` to `no_ci`, `prw wait` on one returns in 2s instead of the 30-min timeout) +- ✅ Codex review feedback on #6, #7, #13–#17 — done in `fix/codex-review`: superseded check runs (verified live on zephyrproject-rtos/zephyr#105183: was `failure`, now `success`), head re-check in `prw wait`, context propagation into every API request, closed review-request PRs keep their notification, no `no_ci` flapping, ETag cache eviction, GovSlack. +- Next: #11 review & mergeability events ## Planned diff --git a/cmd/prw/broadcast.go b/cmd/prw/broadcast.go index bf2138d..312a463 100644 --- a/cmd/prw/broadcast.go +++ b/cmd/prw/broadcast.go @@ -1,6 +1,7 @@ package main import ( + "context" "fmt" "strings" "time" @@ -70,17 +71,18 @@ var broadcastCmd = &cobra.Command{ for i := range cfg.WatchedPRs { pr := &cfg.WatchedPRs[i] - ghPR, err := client.GetPullRequest(pr.Owner, pr.Repo, pr.Number) + ghPR, err := client.GetPullRequest(context.Background(), pr.Owner, pr.Repo, pr.Number) if err != nil { fmt.Printf("Error fetching PR %s/%s#%d: %v\n", pr.Owner, pr.Repo, pr.Number, err) continue } - ci, err := github.FetchCIStatus(client, pr.Owner, pr.Repo, ghPR.Head.SHA) + ci, err := github.FetchCIStatus(context.Background(), client, pr.Owner, pr.Repo, ghPR.Head.SHA) if err != nil { fmt.Printf("Error fetching status for %s/%s#%d: %v\n", pr.Owner, pr.Repo, pr.Number, err) continue } + github.ApplyNoCI(ci, ghPR.UpdatedAt, time.Now()) currentState := ci.State previousState := github.NormalizeState(pr.LastKnownState) diff --git a/cmd/prw/main.go b/cmd/prw/main.go index a7a7665..bb10c5c 100644 --- a/cmd/prw/main.go +++ b/cmd/prw/main.go @@ -157,7 +157,7 @@ func watchSinglePR(prURL string) error { // Try to fetch the PR to validate it exists and get title client := newGitHubClient(token) - pr, err := client.GetPullRequest(owner, repo, number) + pr, err := client.GetPullRequest(context.Background(), owner, repo, number) if err != nil { return fmt.Errorf("failed to fetch PR: %w", err) } @@ -229,7 +229,7 @@ func enableAutoWatch(sources []string) error { } // Run the searches now so the user sees the result immediately. - res, err := watcher.SyncAutoWatch(newGitHubClient(token), sources) + res, err := watcher.SyncAutoWatch(context.Background(), newGitHubClient(token), sources) for _, warning := range res.Warnings { fmt.Printf("Warning: %s\n", warning) } diff --git a/cmd/prw/mcp.go b/cmd/prw/mcp.go index 44d7240..2a434a5 100644 --- a/cmd/prw/mcp.go +++ b/cmd/prw/mcp.go @@ -43,7 +43,7 @@ const ( mcpInstructions = `prw reports the CI status of GitHub pull requests, aggregating GitHub Actions check runs and legacy commit statuses. Use get_ci_status for a one-off check, and wait_for_ci after pushing to a PR to block until CI finishes (it fails fast and lists failed checks with links). -States: success, failure, error, pending, merged, closed.` +States: success, failure, error, pending, merged, closed, and no_ci (the PR has no checks or statuses at all; waiting longer will not help).` ) func init() { @@ -428,7 +428,7 @@ var mcpTools = []map[string]interface{}{ { "name": "get_ci_status", "title": "Get PR CI status", - "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed), check counts, and failed checks with links. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", + "description": "Get the current CI status of a GitHub pull request: overall state (success, failure, error, pending, merged, closed, or no_ci when the PR has no checks at all), check counts, and failed checks with links. Aggregates GitHub Actions check runs and legacy commit statuses. Returns immediately.", "inputSchema": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{"pr_url": prURLSchema}, @@ -440,7 +440,7 @@ var mcpTools = []map[string]interface{}{ { "name": "wait_for_ci", "title": "Wait for PR CI", - "description": "Block until a GitHub pull request's CI finishes, then return the result. Fails fast: returns as soon as any check fails, with the failed checks and links. Follows new pushes to the PR. If the timeout is reached first, returns state 'pending' with timed_out=true (call again to keep waiting). A merge while waiting returns state 'merged'.", + "description": "Block until a GitHub pull request's CI finishes, then return the result. Fails fast: returns as soon as any check fails, with the failed checks and links. Follows new pushes to the PR. If the timeout is reached first, returns state 'pending' with timed_out=true (call again to keep waiting). A merge while waiting returns state 'merged'; a PR with no checks or statuses at all returns 'no_ci' instead of waiting until the timeout.", "inputSchema": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{ @@ -545,6 +545,8 @@ func ciSummary(r *waitResult) string { return fmt.Sprintf("CI of %s is pending: %s.", pr, pendingSummary(r)) case r.State == "success": return fmt.Sprintf("CI of %s passed (%d check(s)).", pr, r.TotalChecks) + case r.State == github.StateNoCI: + return fmt.Sprintf("%s has no CI: no checks or statuses were reported within %s of its last update, so nothing verified it.", pr, github.NoCIGracePeriod) } names := make([]string, len(r.FailedChecks)) for i, c := range r.FailedChecks { @@ -567,7 +569,7 @@ func toolGetCIStatus(ctx context.Context, s *mcpServer, args json.RawMessage, _ return toolError("%v", err) } result := &waitResult{Owner: owner, Repo: repo, Number: number, URL: github.FormatPRURL(owner, repo, number)} - if _, err := pollOnce(client, result); err != nil { + if _, err := pollOnce(ctx, client, result); err != nil { return toolError("failed to get CI status of %s/%s#%d: %v", owner, repo, number, err) } return toolOK(ciSummary(result), result) @@ -664,7 +666,7 @@ func toolWatchPR(ctx context.Context, s *mcpServer, args json.RawMessage, _ io.W if err != nil { return toolError("%v", err) } - pr, err := client.GetPullRequest(owner, repo, number) + pr, err := client.GetPullRequest(ctx, owner, repo, number) if err != nil { return toolError("failed to fetch %s/%s#%d: %v", owner, repo, number, err) } diff --git a/cmd/prw/noci_test.go b/cmd/prw/noci_test.go new file mode 100644 index 0000000..221fd01 --- /dev/null +++ b/cmd/prw/noci_test.go @@ -0,0 +1,91 @@ +package main + +import ( + "bytes" + "context" + "fmt" + "net/http" + "strings" + "testing" + "time" + + "github.com/devblac/prw/internal/github" +) + +func staleOpenPR(sha string, quietFor time.Duration) *github.PullRequest { + pr := openPR(sha) + pr.UpdatedAt = time.Now().Add(-quietFor) + return pr +} + +func TestWaitForCI_NoCIStopsInsteadOfWaitingForTheTimeout(t *testing.T) { + noSleep(t) + client := &scriptedClient{steps: []pollStep{{pr: staleOpenPR("sha", time.Hour)}}} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.State != github.StateNoCI || client.polls != 1 { + t.Errorf("state=%q polls=%d, want no_ci after one poll", result.State, client.polls) + } + if code := exitCode(result.exitErr()); code != exitWaitNoCI { + t.Errorf("exit code = %d, want 4 (no CI must not look like success)", code) + } +} + +func TestWaitForCI_WaitsForCIToRegisterAfterAPush(t *testing.T) { + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: staleOpenPR("new", 10*time.Second)}, // just pushed, nothing registered yet + {pr: staleOpenPR("new", 20*time.Second), runs: []github.CheckRun{run("Test", "queued", "")}}, + {pr: staleOpenPR("new", 30*time.Second), runs: []github.CheckRun{run("Test", "completed", "success")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.State != "success" || client.polls != 3 { + t.Errorf("state=%q polls=%d, want success after 3 polls", result.State, client.polls) + } +} + +func TestPrintWaitResult_NoCI(t *testing.T) { + var buf bytes.Buffer + printWaitResult(&buf, &waitResult{Owner: "o", Repo: "r", Number: 1, State: github.StateNoCI, URL: "https://github.com/o/r/pull/1"}, false) + if !strings.Contains(buf.String(), "⚪ o/r#1") || !strings.Contains(buf.String(), "No CI: no checks or statuses") { + t.Errorf("output:\n%s", buf.String()) + } +} + +func TestCISummary_NoCI(t *testing.T) { + got := ciSummary(&waitResult{Owner: "o", Repo: "r", Number: 1, State: github.StateNoCI}) + if !strings.HasPrefix(got, "o/r#1 has no CI") { + t.Errorf("summary = %q", got) + } +} + +func TestWaitCmd_NoCIExitCode(t *testing.T) { + updated := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) + setupWaitCommand(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/pulls/1"): + fmt.Fprintf(w, `{"number":1,"title":"Stacked","state":"open","updated_at":%q,"head":{"sha":"abc1234"}}`, updated) + case strings.HasSuffix(r.URL.Path, "/status"): + fmt.Fprint(w, `{"state":"pending","total_count":0,"statuses":[]}`) + default: + fmt.Fprint(w, `{"total_count":0,"check_runs":[]}`) + } + }) + + out, err := captureStdout(func() error { + return waitCmd.RunE(waitCmd, []string{"https://github.com/owner/repo/pull/1"}) + }) + if code := exitCode(err); code != exitWaitNoCI { + t.Errorf("exit code = %d (err=%v), want 4", code, err) + } + if !strings.Contains(out, "No CI") { + t.Errorf("stdout:\n%s", out) + } +} diff --git a/cmd/prw/wait.go b/cmd/prw/wait.go index 7c8e4b1..32895a0 100644 --- a/cmd/prw/wait.go +++ b/cmd/prw/wait.go @@ -24,6 +24,7 @@ const ( exitWaitFailed = 1 exitWaitTimeout = 2 exitWaitError = 3 + exitWaitNoCI = 4 exitWaitInterrupted = 130 // maxConsecutiveWaitErrors is how many transient API errors in a row `prw wait` @@ -82,6 +83,8 @@ Exit codes: 1 CI failed (failure or error) 2 timed out while CI was still pending 3 other error (invalid URL, API error, PR closed without merging) + 4 the PR has no CI: no checks or statuses reported within 3 minutes of its + last update (e.g. the repository has no CI, or none runs for its base branch) 130 interrupted (Ctrl+C) Progress goes to stderr; the result goes to stdout (as JSON with --json).`, @@ -177,13 +180,16 @@ func (r *waitResult) exitErr() error { return waitErrorf(exitWaitFailed, "CI of %s finished with %s (%d failed check(s))", pr, r.State, len(r.FailedChecks)) case "closed": return waitErrorf(exitWaitError, "%s was closed without merging", pr) + case github.StateNoCI: + // Not a success: `prw wait && deploy` must not deploy a PR that no CI looked at. + return waitErrorf(exitWaitNoCI, "%s has no CI: no checks or statuses were reported", pr) } return waitErrorf(exitWaitError, "unexpected state %q for %s", r.State, pr) } // ciClient is the subset of the GitHub client used by waitForCI. type ciClient interface { - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) github.CIFetcher } @@ -197,11 +203,29 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number return result } + stopped := func() *waitResult { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + result.TimedOut = true + } else { + result.Interrupted = true + } + return finish() + } + var lastLine string consecutiveErrors := 0 for { delay := interval - state, err := pollOnce(client, result) + state, err := pollOnce(ctx, client, result) + moved := false + if err == nil && state != "pending" { + // Errors confirming the head are handled like poll errors: retried, or + // reported as a timeout / interruption, never taken as "head unchanged". + moved, err = headMoved(ctx, client, result) + } + if err != nil && ctx.Err() != nil { + return stopped(), nil // the timeout or Ctrl-C cancelled the poll's requests + } if limited, resetAt := github.IsRateLimited(err); limited { // Not a failure of the PR or the network: wait for the quota to reset // (still bounded by --timeout) instead of burning the retry budget. @@ -221,6 +245,10 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number fmt.Fprintf(progress, "⚠️ %v (retrying)\n", err) } else { consecutiveErrors = 0 + if moved { + fmt.Fprintf(progress, "↻ %s/%s#%d: new commit pushed; waiting for its CI\n", owner, repo, number) + continue + } if state != "pending" { return finish(), nil } @@ -233,19 +261,29 @@ func waitForCI(ctx context.Context, client ciClient, owner, repo string, number sleepFunc(ctx, delay) if ctx.Err() != nil { - if errors.Is(ctx.Err(), context.DeadlineExceeded) { - result.TimedOut = true - } else { - result.Interrupted = true - } - return finish(), nil + return stopped(), nil } } } +// headMoved reports whether a new commit was pushed to the PR since result was +// polled. The CI lookup follows the PR lookup, so a push in between would otherwise +// return the result of the previous commit. Merged or closed PRs don't move. +// Unchanged PRs answer 304, so this costs no quota. +func headMoved(ctx context.Context, client ciClient, result *waitResult) (bool, error) { + if result.State == "merged" || result.State == "closed" { + return false, nil + } + pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) + if err != nil { + return false, fmt.Errorf("failed to confirm the PR head: %w", err) + } + return pr.Head.SHA != result.SHA, nil +} + // pollOnce refreshes result from GitHub and returns the current state. -func pollOnce(client ciClient, result *waitResult) (string, error) { - pr, err := client.GetPullRequest(result.Owner, result.Repo, result.Number) +func pollOnce(ctx context.Context, client ciClient, result *waitResult) (string, error) { + pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) if err != nil { return "", fmt.Errorf("failed to fetch PR: %w", err) } @@ -263,10 +301,11 @@ func pollOnce(client ciClient, result *waitResult) (string, error) { return result.State, nil } - ci, err := github.FetchCIStatus(client, result.Owner, result.Repo, pr.Head.SHA) + ci, err := github.FetchCIStatus(ctx, client, result.Owner, result.Repo, pr.Head.SHA) if err != nil { return "", err } + github.ApplyNoCI(ci, pr.UpdatedAt, time.Now()) result.State = ci.State result.FailedChecks = ci.FailedChecks result.TotalChecks = ci.Total @@ -313,6 +352,8 @@ func printWaitResult(w io.Writer, r *waitResult, asJSON bool) error { icon = "✅" case "failure", "error", "closed": icon = "❌" + case github.StateNoCI: + icon = "⚪" } header := fmt.Sprintf("%s %s/%s#%d", icon, r.Owner, r.Repo, r.Number) if r.Title != "" { @@ -328,6 +369,8 @@ func printWaitResult(w io.Writer, r *waitResult, asJSON bool) error { fmt.Fprintf(w, "Timed out after %s; CI is still %s\n", elapsed, r.State) case r.State == "merged" || r.State == "closed": fmt.Fprintf(w, "PR was %s (after %s)\n", r.State, elapsed) + case r.State == github.StateNoCI: + fmt.Fprintf(w, "No CI: no checks or statuses were reported for this PR (after %s)\n", elapsed) default: fmt.Fprintf(w, "CI: %s (%d check(s), %s)\n", r.State, r.TotalChecks, elapsed) } diff --git a/cmd/prw/wait_ctx_test.go b/cmd/prw/wait_ctx_test.go new file mode 100644 index 0000000..e2db5a7 --- /dev/null +++ b/cmd/prw/wait_ctx_test.go @@ -0,0 +1,135 @@ +package main + +import ( + "bytes" + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/devblac/prw/internal/github" +) + +func TestWaitForCI_FollowsPushThatLandsDuringThePoll(t *testing.T) { + // The PR is read at "old", then "new" is pushed before the CI lookup of "old" + // returns success: that result must not be reported for the PR. + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: openPR("old"), runs: []github.CheckRun{run("Test", "completed", "success")}, movedTo: openPR("new")}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "in_progress", "")}}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "completed", "failure")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.SHA != "new" || result.State != "failure" { + t.Errorf("got %s on %s, want the failure of the new head", result.State, result.SHA) + } +} + +func TestWaitForCI_ConfirmsHeadBeforeReturning(t *testing.T) { + noSleep(t) + client := &scriptedClient{steps: []pollStep{{pr: openPR("sha"), runs: []github.CheckRun{run("Test", "completed", "success")}}}} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.State != "success" || client.polls != 1 || client.revalidations != 1 { + t.Errorf("state=%q polls=%d revalidations=%d, want success after one poll and one head check", + result.State, client.polls, client.revalidations) + } +} + +func TestWaitForCI_RetriesWhenTheHeadCheckFails(t *testing.T) { + // A push landed during the poll, and the request that would notice it failed: the + // old commit's result must not be returned. + noSleep(t) + client := &scriptedClient{steps: []pollStep{ + {pr: openPR("old"), runs: []github.CheckRun{run("Test", "completed", "success")}, + revalidate: func() error { return errors.New("connection reset") }}, + {pr: openPR("new"), runs: []github.CheckRun{run("Test", "completed", "failure")}}, + }} + + result, err := waitForCI(context.Background(), client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if result.SHA != "new" || result.State != "failure" { + t.Errorf("got %s on %s, want the failure of the new head", result.State, result.SHA) + } +} + +func TestWaitForCI_InterruptDuringTheHeadCheck(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + client := &scriptedClient{steps: []pollStep{{ + pr: openPR("sha"), runs: []github.CheckRun{run("Test", "completed", "success")}, + revalidate: func() error { cancel(); return ctx.Err() }, // Ctrl-C while it's in flight + }}} + + result, err := waitForCI(ctx, client, "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if !result.Interrupted || exitCode(result.exitErr()) != exitWaitInterrupted { + t.Errorf("result = %+v, want interrupted (exit 130), not success", result) + } +} + +// hangingClient is a real client against a server that never answers until the +// request is cancelled, like a stalled network. +func hangingClient(t *testing.T) *github.Client { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-time.After(10 * time.Second): + } + })) + t.Cleanup(srv.Close) + client := github.NewClient("token") + client.BaseURL = srv.URL + return client +} + +func TestWaitForCI_TimeoutCancelsTheRequestInFlight(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + start := time.Now() + result, err := waitForCI(ctx, hangingClient(t), "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("returned after %s; the timeout must cancel the hanging request", elapsed) + } + if !result.TimedOut || exitCode(result.exitErr()) != exitWaitTimeout { + t.Errorf("result = %+v, want timed out (exit 2)", result) + } +} + +func TestWaitForCI_InterruptCancelsTheRequestInFlight(t *testing.T) { + noSleep(t) + ctx, cancel := context.WithCancel(context.Background()) + time.AfterFunc(100*time.Millisecond, cancel) // Ctrl-C + + start := time.Now() + result, err := waitForCI(ctx, hangingClient(t), "o", "r", 1, time.Second, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("returned after %s; Ctrl-C must cancel the hanging request", elapsed) + } + if !result.Interrupted || exitCode(result.exitErr()) != exitWaitInterrupted { + t.Errorf("result = %+v, want interrupted (exit 130)", result) + } +} diff --git a/cmd/prw/wait_test.go b/cmd/prw/wait_test.go index 295b300..c3da461 100644 --- a/cmd/prw/wait_test.go +++ b/cmd/prw/wait_test.go @@ -25,16 +25,46 @@ type pollStep struct { status *github.CombinedStatus runs []github.CheckRun ciErr error + // movedTo, if set, is what the PR looks like when waitForCI re-reads it to + // confirm the head hasn't moved, i.e. a push that landed during this poll. + movedTo *github.PullRequest + // revalidate, if set, runs on that re-read and its error is returned. + revalidate func() error } -// scriptedClient returns steps[i] on the i-th poll (repeating the last one). +// scriptedClient returns steps[i] on the i-th poll (repeating the last one). A PR +// lookup right after a poll that found a final CI result is the head re-check, not a +// new poll. type scriptedClient struct { - steps []pollStep - polls int - current pollStep + steps []pollStep + polls int + revalidations int + current pollStep + afterCI bool } -func (c *scriptedClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (c *scriptedClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + if c.afterCI { + c.afterCI = false + status := c.current.status + if status == nil { + status = &github.CombinedStatus{State: "pending"} + } + ci := github.SummarizeCI(status, c.current.runs) + github.ApplyNoCI(ci, c.current.pr.UpdatedAt, time.Now()) + if ci.State != "pending" { + c.revalidations++ + if c.current.revalidate != nil { + if err := c.current.revalidate(); err != nil { + return nil, err + } + } + if c.current.movedTo != nil { + return c.current.movedTo, nil + } + return c.current.pr, nil + } + } i := c.polls if i >= len(c.steps) { i = len(c.steps) - 1 @@ -44,7 +74,7 @@ func (c *scriptedClient) GetPullRequest(owner, repo string, number int) (*github return c.current.pr, c.current.prErr } -func (c *scriptedClient) GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) { +func (c *scriptedClient) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) { if c.current.ciErr != nil { return nil, c.current.ciErr } @@ -54,7 +84,8 @@ func (c *scriptedClient) GetCombinedStatus(owner, repo, ref string) (*github.Com return &github.CombinedStatus{State: "pending"}, nil } -func (c *scriptedClient) GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) { +func (c *scriptedClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { + c.afterCI = true return c.current.runs, nil } diff --git a/internal/github/client.go b/internal/github/client.go index 2a771c3..a5b9069 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -1,6 +1,7 @@ package github import ( + "context" "encoding/json" "errors" "fmt" @@ -108,6 +109,31 @@ type PullRequest struct { Head struct { SHA string `json:"sha"` } `json:"head"` + // UpdatedAt changes on every push (and on comments, labels, ...), so it is an + // upper bound for how recently the head commit was pushed. + UpdatedAt time.Time `json:"updated_at"` +} + +// StateNoCI is reported instead of "pending" for a PR that has no checks or +// statuses at all and wasn't updated for NoCIGracePeriod, i.e. one whose repository +// (or base branch) runs no CI for it. +const StateNoCI = "no_ci" + +// NoCIGracePeriod is how long after the last update of a PR prw keeps waiting for +// CI to report its first check before concluding the PR has none. GitHub Actions +// usually registers checks within seconds; external CI can take longer. +const NoCIGracePeriod = 3 * time.Minute + +// ApplyNoCI turns a "pending" status with no checks or statuses at all into StateNoCI +// once the PR has gone NoCIGracePeriod without updates. updatedAt is the PR's +// UpdatedAt; when it's unknown (zero), the status is left as pending. +func ApplyNoCI(ci *CIStatus, updatedAt, now time.Time) { + if ci.State != "pending" || ci.Total != 0 || updatedAt.IsZero() { + return + } + if now.Sub(updatedAt) >= NoCIGracePeriod { + ci.State = StateNoCI + } } // IsClosed reports whether the PR is closed, merged or not (GitHub reports both as @@ -134,10 +160,14 @@ type CommitStatus struct { // CheckRun represents a single check run (e.g. a GitHub Actions job). type CheckRun struct { + ID int64 `json:"id"` Name string `json:"name"` Status string `json:"status"` // queued, in_progress, completed, ... Conclusion string `json:"conclusion"` // success, failure, neutral, cancelled, ... HTMLURL string `json:"html_url"` + App struct { + ID int64 `json:"id"` + } `json:"app"` } type checkRunsResponse struct { @@ -163,8 +193,8 @@ type CIStatus struct { // CIFetcher is the subset of GitHub operations needed to compute CI status. type CIFetcher interface { - GetCombinedStatus(owner, repo, ref string) (*CombinedStatus, error) - GetCheckRuns(owner, repo, ref string) ([]CheckRun, error) + GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*CombinedStatus, error) + GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]CheckRun, error) } // ParsePRURL extracts owner, repo, and PR number from a GitHub PR URL. @@ -187,36 +217,36 @@ func ParsePRURL(prURL string) (owner, repo string, number int, err error) { } // GetPullRequest fetches a pull request by owner, repo, and PR number. -func (c *Client) GetPullRequest(owner, repo string, number int) (*PullRequest, error) { +func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) { path := fmt.Sprintf("/repos/%s/%s/pulls/%d", owner, repo, number) var pr PullRequest - if err := c.getJSON(path, &pr); err != nil { + if err := c.getJSON(ctx, path, &pr); err != nil { return nil, err } return &pr, nil } // GetCombinedStatus fetches the combined legacy commit status for a commit. -func (c *Client) GetCombinedStatus(owner, repo, ref string) (*CombinedStatus, error) { +func (c *Client) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*CombinedStatus, error) { path := fmt.Sprintf("/repos/%s/%s/commits/%s/status", owner, repo, url.PathEscape(ref)) var status CombinedStatus - if err := c.getJSON(path, &status); err != nil { + if err := c.getJSON(ctx, path, &status); err != nil { return nil, err } return &status, nil } // GetCheckRuns fetches the latest check runs (e.g. GitHub Actions jobs) for a commit. -func (c *Client) GetCheckRuns(owner, repo, ref string) ([]CheckRun, error) { +func (c *Client) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]CheckRun, error) { var runs []CheckRun for page := 1; page <= maxCheckRunPages; page++ { path := fmt.Sprintf("/repos/%s/%s/commits/%s/check-runs?per_page=%d&page=%d", owner, repo, url.PathEscape(ref), checkRunsPerPage, page) var resp checkRunsResponse - if err := c.getJSON(path, &resp); err != nil { + if err := c.getJSON(ctx, path, &resp); err != nil { var apiErr *APIError if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden && !apiErr.RateLimited { return nil, fmt.Errorf("%w (fine-grained tokens need the \"Checks: read\" permission)", err) @@ -269,12 +299,12 @@ type searchIssuesResponse struct { // SearchPullRequests runs a GitHub issue search (e.g. "is:pr is:open author:@me") and // returns the pull requests it finds, up to 300. Qualifiers like @me need a token. -func (c *Client) SearchPullRequests(query string) (*SearchResult, error) { +func (c *Client) SearchPullRequests(ctx context.Context, query string) (*SearchResult, error) { result := &SearchResult{} for page := 1; page <= maxSearchPages; page++ { path := fmt.Sprintf("/search/issues?q=%s&per_page=%d&page=%d", url.QueryEscape(query), searchPerPage, page) var resp searchIssuesResponse - if err := c.getJSON(path, &resp); err != nil { + if err := c.getJSON(ctx, path, &resp); err != nil { return nil, err } result.Total = resp.TotalCount @@ -302,13 +332,13 @@ func (c *Client) SearchPullRequests(query string) (*SearchResult, error) { } // FetchCIStatus fetches commit statuses and check runs for a commit and aggregates them. -func FetchCIStatus(c CIFetcher, owner, repo, ref string) (*CIStatus, error) { - status, err := c.GetCombinedStatus(owner, repo, ref) +func FetchCIStatus(ctx context.Context, c CIFetcher, owner, repo, ref string) (*CIStatus, error) { + status, err := c.GetCombinedStatus(ctx, owner, repo, ref) if err != nil { return nil, fmt.Errorf("failed to fetch commit status: %w", err) } - runs, err := c.GetCheckRuns(owner, repo, ref) + runs, err := c.GetCheckRuns(ctx, owner, repo, ref) if err != nil { return nil, fmt.Errorf("failed to fetch check runs: %w", err) } @@ -352,7 +382,7 @@ func SummarizeCI(status *CombinedStatus, runs []CheckRun) *CIStatus { } } - for _, run := range runs { + for _, run := range latestCheckRuns(runs) { total++ if NormalizeState(run.Status) != "completed" { hasPending = true @@ -382,6 +412,35 @@ func SummarizeCI(status *CombinedStatus, runs []CheckRun) *CIStatus { return &CIStatus{State: state, FailedChecks: failed, Total: total, Pending: pending} } +// latestCheckRuns drops check runs superseded by a newer run of the same check (same +// app and name). The API returns one run per check suite, so a workflow that ran more +// than once for the same commit (e.g. an older run cancelled by `concurrency: +// cancel-in-progress`) shows up twice; like GitHub's merge box and required checks, +// only the newest one counts. Run IDs grow with creation time. +func latestCheckRuns(runs []CheckRun) []CheckRun { + type checkKey struct { + app int64 + name string + } + newest := make(map[checkKey]int64, len(runs)) + for _, run := range runs { + key := checkKey{run.App.ID, run.Name} + if id, seen := newest[key]; !seen || run.ID > id { + newest[key] = run.ID + } + } + if len(newest) == len(runs) { + return runs + } + latest := make([]CheckRun, 0, len(newest)) + for _, run := range runs { + if newest[checkKey{run.App.ID, run.Name}] == run.ID { + latest = append(latest, run) + } + } + return latest +} + func hasCISignal(status *CombinedStatus, runs []CheckRun) bool { if len(runs) > 0 { return true @@ -394,8 +453,8 @@ func hasCISignal(status *CombinedStatus, runs []CheckRun) bool { } // getJSON performs an authenticated GET request and decodes the JSON response into v. -func (c *Client) getJSON(path string, v interface{}) error { - req, err := http.NewRequest("GET", c.BaseURL+path, nil) +func (c *Client) getJSON(ctx context.Context, path string, v interface{}) error { + req, err := http.NewRequestWithContext(ctx, "GET", c.BaseURL+path, nil) if err != nil { return err } @@ -447,9 +506,19 @@ func (c *Client) getJSON(path string, v interface{}) error { func (c *Client) storeCache(path string, entry cachedResponse) { c.mu.Lock() defer c.mu.Unlock() - if c.cache == nil || len(c.cache) >= maxCacheEntries { + if c.cache == nil { c.cache = make(map[string]cachedResponse) } + if _, exists := c.cache[path]; !exists && len(c.cache) >= maxCacheEntries { + // Make room by evicting one entry at random (map iteration order is random). + // Clearing the whole cache, or LRU, would make every request unconditional + // once more paths are polled in rotation than fit: polling cycles through + // the same paths, which is LRU's worst case. Random eviction keeps most hits. + for victim := range c.cache { + delete(c.cache, victim) + break + } + } c.cache[path] = entry } diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 25b688f..0f7168f 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "fmt" "io" "net/http" @@ -144,7 +145,7 @@ func TestGetPullRequest(t *testing.T) { }, } - pr, err := client.GetPullRequest("owner", "repo", 123) + pr, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if tt.wantErr { if err == nil { t.Error("expected error, got nil") @@ -228,7 +229,7 @@ func TestGetCombinedStatus(t *testing.T) { }, } - status, err := client.GetCombinedStatus("owner", "repo", "abc123") + status, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if tt.wantErr { if err == nil { t.Error("expected error, got nil") @@ -333,7 +334,7 @@ func TestGetPullRequest_NetworkError(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for network failure, got nil") } @@ -355,7 +356,7 @@ func TestGetCombinedStatus_NetworkError(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for network failure, got nil") } @@ -381,7 +382,7 @@ func TestGetPullRequest_InvalidJSON(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for invalid JSON, got nil") } @@ -407,7 +408,7 @@ func TestGetCombinedStatus_InvalidJSON(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for invalid JSON, got nil") } @@ -433,7 +434,7 @@ func TestGetPullRequest_ErrorResponse(t *testing.T) { }, } - _, err := client.GetPullRequest("owner", "repo", 123) + _, err := client.GetPullRequest(context.Background(), "owner", "repo", 123) if err == nil { t.Error("expected error for 403 response, got nil") } @@ -459,7 +460,7 @@ func TestGetCombinedStatus_ErrorResponse(t *testing.T) { }, } - _, err := client.GetCombinedStatus("owner", "repo", "abc123") + _, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err == nil { t.Error("expected error for 404 response, got nil") } @@ -539,7 +540,7 @@ func TestGetCombinedStatus_EmptyState(t *testing.T) { }, } - status, err := client.GetCombinedStatus("owner", "repo", "abc123") + status, err := client.GetCombinedStatus(context.Background(), "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -570,7 +571,7 @@ func TestGetJSON_OmitsAuthorizationWithoutToken(t *testing.T) { }, } - if _, err := client.GetPullRequest("owner", "repo", 1); err != nil { + if _, err := client.GetPullRequest(context.Background(), "owner", "repo", 1); err != nil { t.Fatalf("unexpected error: %v", err) } if len(gotAuth) != 0 { @@ -607,7 +608,7 @@ func TestGetCheckRuns_Paginates(t *testing.T) { }, } - runs, err := client.GetCheckRuns("owner", "repo", "abc123") + runs, err := client.GetCheckRuns(context.Background(), "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -632,7 +633,7 @@ func TestGetCheckRuns_ForbiddenHint(t *testing.T) { }, } - _, err := client.GetCheckRuns("owner", "repo", "abc123") + _, err := client.GetCheckRuns(context.Background(), "owner", "repo", "abc123") if err == nil { t.Fatal("expected error for 403 response, got nil") } @@ -662,7 +663,7 @@ func TestFetchCIStatus_ActionsOnlyRepo(t *testing.T) { }, } - ci, err := FetchCIStatus(client, "owner", "repo", "abc123") + ci, err := FetchCIStatus(context.Background(), client, "owner", "repo", "abc123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -690,7 +691,7 @@ func TestFetchCIStatus_PropagatesErrors(t *testing.T) { }, } - _, err := FetchCIStatus(client, "owner", "repo", "abc123") + _, err := FetchCIStatus(context.Background(), client, "owner", "repo", "abc123") if err == nil || !strings.Contains(err.Error(), "check runs") { t.Errorf("expected check runs error, got %v", err) } diff --git a/internal/github/efficiency_test.go b/internal/github/efficiency_test.go index 8e79432..8c6ac79 100644 --- a/internal/github/efficiency_test.go +++ b/internal/github/efficiency_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "errors" "fmt" "net/http" @@ -37,7 +38,7 @@ func TestGetJSON_UsesETagAndServesCachedBodyOn304(t *testing.T) { }) for i := 0; i < 3; i++ { - pr, err := c.GetPullRequest("o", "r", 7) + pr, err := c.GetPullRequest(context.Background(), "o", "r", 7) if err != nil { t.Fatalf("request %d: %v", i, err) } @@ -63,13 +64,13 @@ func TestGetJSON_RefreshesCacheWhenResourceChanges(t *testing.T) { fmt.Fprintf(w, `{"state":"pending","total_count":%d}`, v) }) - s, _ := c.GetCombinedStatus("o", "r", "sha") + s, _ := c.GetCombinedStatus(context.Background(), "o", "r", "sha") atomic.StoreInt32(&version, 2) - s2, err := c.GetCombinedStatus("o", "r", "sha") + s2, err := c.GetCombinedStatus(context.Background(), "o", "r", "sha") if err != nil { t.Fatal(err) } - s3, _ := c.GetCombinedStatus("o", "r", "sha") + s3, _ := c.GetCombinedStatus(context.Background(), "o", "r", "sha") if s.TotalCount != 1 || s2.TotalCount != 2 || s3.TotalCount != 2 { t.Errorf("got %d, %d, %d; want 1, 2, 2", s.TotalCount, s2.TotalCount, s3.TotalCount) } @@ -85,8 +86,8 @@ func TestGetJSON_CachesPerPath(t *testing.T) { fmt.Fprintf(w, `{"number":%s}`, n) }) - a, _ := c.GetPullRequest("o", "r", 1) - b, _ := c.GetPullRequest("o", "r", 2) + a, _ := c.GetPullRequest(context.Background(), "o", "r", 1) + b, _ := c.GetPullRequest(context.Background(), "o", "r", 2) if a.Number != 1 || b.Number != 2 { t.Errorf("got #%d and #%d", a.Number, b.Number) } @@ -99,15 +100,15 @@ func TestGetJSON_NoETagMeansNoConditionalRequest(t *testing.T) { } fmt.Fprint(w, `{"number":1}`) }) - c.GetPullRequest("o", "r", 1) - c.GetPullRequest("o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) } func TestGetJSON_Unexpected304IsAnError(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotModified) }) - if _, err := c.GetPullRequest("o", "r", 1); err == nil { + if _, err := c.GetPullRequest(context.Background(), "o", "r", 1); err == nil { t.Error("a 304 without a cached body must not decode as an empty PR") } } @@ -134,7 +135,7 @@ func TestClient_RecordsRateLimitHeaders(t *testing.T) { if c.RateLimit().Known { t.Error("rate limit should be unknown before any request") } - c.GetPullRequest("o", "r", 1) + c.GetPullRequest(context.Background(), "o", "r", 1) rl := c.RateLimit() if !rl.Known || rl.Limit != 5000 || rl.Remaining != 4321 || rl.Reset.Unix() != reset { t.Errorf("RateLimit() = %+v", rl) @@ -251,7 +252,7 @@ func TestGetCheckRuns_RateLimitHasNoPermissionHint(t *testing.T) { w.WriteHeader(http.StatusForbidden) fmt.Fprint(w, `{"message":"API rate limit exceeded"}`) }) - _, err := c.GetCheckRuns("o", "r", "sha") + _, err := c.GetCheckRuns(context.Background(), "o", "r", "sha") if limited, _ := IsRateLimited(err); !limited { t.Fatalf("expected a rate limit error, got %v", err) } @@ -273,9 +274,51 @@ func TestClient_ConcurrentRequestsAreSafe(t *testing.T) { wg.Add(1) go func(n int) { defer wg.Done() - c.GetPullRequest("o", "r", n%3) + c.GetPullRequest(context.Background(), "o", "r", n%3) c.RateLimit() }(i) } wg.Wait() } + +func TestStoreCache_FullCacheKeepsOtherEntries(t *testing.T) { + c := NewClient("") + for i := 0; i < maxCacheEntries; i++ { + c.storeCache(fmt.Sprintf("/p/%d", i), cachedResponse{etag: "e"}) + } + c.storeCache("/p/0", cachedResponse{etag: "refreshed"}) // a changed resource + if n := len(c.cache); n != maxCacheEntries || c.cache["/p/0"].etag != "refreshed" { + t.Errorf("refreshing a cached path must not evict anything: %d entries", n) + } + c.storeCache("/new", cachedResponse{etag: "e"}) + if n := len(c.cache); n != maxCacheEntries { + t.Errorf("a new path must evict exactly one entry, cache has %d", n) + } +} + +func TestStoreCache_PollingMorePathsThanFitStillHits(t *testing.T) { + // e.g. ~200 watched PRs, 3 paths each, polled in rotation. + c := NewClient("") + paths := maxCacheEntries + maxCacheEntries/4 + hits, lookups := 0, 0 + for round := 0; round < 20; round++ { + for i := 0; i < paths; i++ { + path := fmt.Sprintf("/p/%d", i) + if _, ok := c.cache[path]; ok { + if round > 0 { + hits++ + } + } else { + c.storeCache(path, cachedResponse{etag: "e"}) + } + if round > 0 { + lookups++ + } + } + } + rate := float64(hits) / float64(lookups) + t.Logf("hit rate %.0f%% polling %d paths through a %d-entry cache", rate*100, paths, maxCacheEntries) + if rate < 0.5 { + t.Errorf("hit rate %.0f%%; overflowing the cache must not make every request unconditional", rate*100) + } +} diff --git a/internal/github/integration_test.go b/internal/github/integration_test.go index 0c23377..26f4817 100644 --- a/internal/github/integration_test.go +++ b/internal/github/integration_test.go @@ -14,6 +14,7 @@ package github import ( + "context" "errors" "net/http" "os" @@ -40,7 +41,7 @@ func integrationClient(t *testing.T) *Client { func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { c := integrationClient(t) - status, err := c.GetCombinedStatus(fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + status, err := c.GetCombinedStatus(context.Background(), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("GetCombinedStatus: %v", err) } @@ -49,7 +50,7 @@ func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { t.Logf("note: legacy status changed to state=%q total_count=%d", status.State, status.TotalCount) } - runs, err := c.GetCheckRuns(fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + runs, err := c.GetCheckRuns(context.Background(), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("GetCheckRuns: %v", err) } @@ -65,7 +66,7 @@ func TestIntegration_ActionsOnlyCommitResolvesToSuccess(t *testing.T) { } func TestIntegration_FetchCIStatus(t *testing.T) { - ci, err := FetchCIStatus(integrationClient(t), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) + ci, err := FetchCIStatus(context.Background(), integrationClient(t), fixtureOwner, fixtureRepo, fixtureActionsOnlySHA) if err != nil { t.Fatalf("FetchCIStatus: %v", err) } @@ -78,7 +79,7 @@ func TestIntegration_FetchCIStatus(t *testing.T) { } func TestIntegration_MergedPullRequest(t *testing.T) { - pr, err := integrationClient(t).GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + pr, err := integrationClient(t).GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("GetPullRequest: %v", err) } @@ -116,13 +117,13 @@ func TestIntegration_ConditionalRequestsAreFree(t *testing.T) { rec := &statusRecorder{} c.HTTPClient.Transport = rec - first, err := c.GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + first, err := c.GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("first request: %v", err) } before := c.RateLimit() - second, err := c.GetPullRequest(fixtureOwner, fixtureRepo, fixtureMergedPR) + second, err := c.GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, fixtureMergedPR) if err != nil { t.Fatalf("second request: %v", err) } @@ -146,7 +147,7 @@ func TestIntegration_ConditionalRequestsAreFree(t *testing.T) { } func TestIntegration_MissingPullRequestIs404(t *testing.T) { - _, err := integrationClient(t).GetPullRequest(fixtureOwner, fixtureRepo, 999999) + _, err := integrationClient(t).GetPullRequest(context.Background(), fixtureOwner, fixtureRepo, 999999) var apiErr *APIError if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusNotFound { t.Errorf("expected a 404 APIError, got %v", err) diff --git a/internal/github/noci_test.go b/internal/github/noci_test.go new file mode 100644 index 0000000..8a89082 --- /dev/null +++ b/internal/github/noci_test.go @@ -0,0 +1,64 @@ +package github + +import ( + "encoding/json" + "testing" + "time" +) + +func TestApplyNoCI(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + tests := []struct { + name string + ci CIStatus + updatedAt time.Time + want string + }{ + {"no checks, quiet past the grace period", CIStatus{State: "pending"}, now.Add(-NoCIGracePeriod), StateNoCI}, + {"no checks, quiet for days", CIStatus{State: "pending"}, now.Add(-72 * time.Hour), StateNoCI}, + {"no checks yet, just pushed", CIStatus{State: "pending"}, now.Add(-30 * time.Second), "pending"}, + {"no checks, just short of the grace period", CIStatus{State: "pending"}, now.Add(-NoCIGracePeriod + time.Second), "pending"}, + {"no checks, unknown update time", CIStatus{State: "pending"}, time.Time{}, "pending"}, + {"checks still running", CIStatus{State: "pending", Total: 2, Pending: 1}, now.Add(-time.Hour), "pending"}, + {"success is left alone", CIStatus{State: "success", Total: 1}, now.Add(-time.Hour), "success"}, + {"failure is left alone", CIStatus{State: "failure", Total: 1}, now.Add(-time.Hour), "failure"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ci := tt.ci + ApplyNoCI(&ci, tt.updatedAt, now) + if ci.State != tt.want { + t.Errorf("state = %q, want %q", ci.State, tt.want) + } + }) + } +} + +func TestApplyNoCI_WithSummarizeCI(t *testing.T) { + now := time.Now() + old := now.Add(-time.Hour) + + // The shape the real API returns for a commit nothing reported on. + none := SummarizeCI(&CombinedStatus{State: "pending", TotalCount: 0}, nil) + ApplyNoCI(none, old, now) + if none.State != StateNoCI { + t.Errorf("empty CI = %q, want no_ci", none.State) + } + + // A legacy status that is still pending is a real signal, not "no CI". + legacy := SummarizeCI(&CombinedStatus{State: "pending", TotalCount: 1, Statuses: []CommitStatus{{Context: "ci", State: "pending"}}}, nil) + ApplyNoCI(legacy, old, now) + if legacy.State != "pending" { + t.Errorf("pending legacy status = %q, want pending", legacy.State) + } +} + +func TestPullRequest_DecodesUpdatedAt(t *testing.T) { + var pr PullRequest + if err := json.Unmarshal([]byte(`{"number":7,"updated_at":"2026-10-18T03:27:41Z"}`), &pr); err != nil { + t.Fatal(err) + } + if want := time.Date(2026, 10, 18, 3, 27, 41, 0, time.UTC); !pr.UpdatedAt.Equal(want) { + t.Errorf("UpdatedAt = %v, want %v", pr.UpdatedAt, want) + } +} diff --git a/internal/github/search_test.go b/internal/github/search_test.go index 3fcc3aa..3416580 100644 --- a/internal/github/search_test.go +++ b/internal/github/search_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "fmt" "net/http" "strconv" @@ -27,7 +28,7 @@ func TestSearchPullRequests_ParsesResultsAndSkipsIssues(t *testing.T) { searchItem("Alice", "api", 1, true), searchItem("bob", "web", 2, false), searchItem("bob", "web", 3, true)) }) - res, err := c.SearchPullRequests("is:pr is:open author:@me") + res, err := c.SearchPullRequests(context.Background(), "is:pr is:open author:@me") if err != nil { t.Fatal(err) } @@ -62,7 +63,7 @@ func TestSearchPullRequests_Paginates(t *testing.T) { fmt.Fprintf(w, `{"total_count":%d,"items":[%s]}`, searchPerPage+5, strings.Join(items, ",")) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -82,7 +83,7 @@ func TestSearchPullRequests_CapsAndMarksIncomplete(t *testing.T) { fmt.Fprintf(w, `{"total_count":5000,"items":[%s]}`, strings.Join(items, ",")) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -98,7 +99,7 @@ func TestSearchPullRequests_IncompleteFromGitHub(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, `{"total_count":1,"incomplete_results":true,"items":[%s]}`, searchItem("o", "r", 1, true)) }) - res, err := c.SearchPullRequests("q") + res, err := c.SearchPullRequests(context.Background(), "q") if err != nil { t.Fatal(err) } @@ -111,7 +112,7 @@ func TestSearchPullRequests_Error(t *testing.T) { c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"message":"Validation Failed"}`, http.StatusUnprocessableEntity) }) - if _, err := c.SearchPullRequests("q"); err == nil || !strings.Contains(err.Error(), "422") { + if _, err := c.SearchPullRequests(context.Background(), "q"); err == nil || !strings.Contains(err.Error(), "422") { t.Errorf("expected a 422 error, got %v", err) } } diff --git a/internal/github/superseded_test.go b/internal/github/superseded_test.go new file mode 100644 index 0000000..0de87b4 --- /dev/null +++ b/internal/github/superseded_test.go @@ -0,0 +1,52 @@ +package github + +import "testing" + +func actionsRun(id int64, name, status, conclusion string) CheckRun { + run := CheckRun{ID: id, Name: name, Status: status, Conclusion: conclusion} + run.App.ID = 15368 // github-actions + return run +} + +func TestSummarizeCI_IgnoresRunSupersededByNewerRun(t *testing.T) { + // Shape of zephyrproject-rtos/zephyr#105183: a workflow with cancel-in-progress ran + // twice for the same commit, in two check suites; the API returns both runs. + runs := []CheckRun{ + actionsRun(66407602784, "Prevent Merging", "completed", "success"), + actionsRun(66407483043, "Prevent Merging", "completed", "cancelled"), + actionsRun(66407400000, "Build", "completed", "success"), + } + ci := SummarizeCI(&CombinedStatus{State: "pending"}, runs) + if ci.State != "success" || len(ci.FailedChecks) != 0 || ci.Total != 2 { + t.Errorf("got state=%q failed=%v total=%d, want success with 2 checks", ci.State, ci.FailedChecks, ci.Total) + } +} + +func TestSummarizeCI_NewerFailureStillCounts(t *testing.T) { + runs := []CheckRun{ + actionsRun(1, "Test", "completed", "success"), + actionsRun(2, "Test", "completed", "failure"), + } + if ci := SummarizeCI(nil, runs); ci.State != "failure" || len(ci.FailedChecks) != 1 { + t.Errorf("got state=%q failed=%v, want the newer failure", ci.State, ci.FailedChecks) + } +} + +func TestSummarizeCI_NewerRunStillInProgress(t *testing.T) { + runs := []CheckRun{ + actionsRun(1, "Test", "completed", "failure"), + actionsRun(2, "Test", "in_progress", ""), + } + if ci := SummarizeCI(nil, runs); ci.State != "pending" || ci.Pending != 1 || len(ci.FailedChecks) != 0 { + t.Errorf("got state=%q pending=%d failed=%v, want pending on the re-run", ci.State, ci.Pending, ci.FailedChecks) + } +} + +func TestSummarizeCI_SameNameFromDifferentAppsIsKept(t *testing.T) { + other := CheckRun{ID: 1, Name: "Test", Status: "completed", Conclusion: "failure"} + other.App.ID = 999 // e.g. an external CI that also names its check "Test" + runs := []CheckRun{other, actionsRun(2, "Test", "completed", "success")} + if ci := SummarizeCI(nil, runs); ci.State != "failure" { + t.Errorf("state = %q; a failure from another app must not be hidden", ci.State) + } +} diff --git a/internal/notify/webhook_format.go b/internal/notify/webhook_format.go index c91e6c3..7f7f29a 100644 --- a/internal/notify/webhook_format.go +++ b/internal/notify/webhook_format.go @@ -29,6 +29,7 @@ const ( colorSuccess = 0x2EA043 colorFailure = 0xD1242F colorPending = 0xDBAB09 + colorNoCI = 0x8C959F ) // IsValidWebhookFormat reports whether value is a supported webhook format. @@ -58,7 +59,7 @@ func DetectWebhookFormat(rawURL string) string { } host := strings.ToLower(u.Hostname()) switch { - case host == "hooks.slack.com": + case host == "hooks.slack.com" || host == "hooks.slack-gov.com": // GovSlack uses its own domain return WebhookFormatSlack case (host == "discord.com" || host == "discordapp.com" || strings.HasSuffix(host, ".discord.com") || strings.HasSuffix(host, ".discordapp.com")) && strings.HasPrefix(u.Path, "/api/webhooks/"): @@ -110,6 +111,8 @@ func stateEmoji(state string) string { return "✅" case "failure", "error": return "❌" + case github.StateNoCI: + return "⚪" default: return "⏳" } @@ -121,6 +124,8 @@ func stateColor(state string) int { return colorSuccess case "failure", "error": return colorFailure + case github.StateNoCI: + return colorNoCI default: return colorPending } diff --git a/internal/notify/webhook_format_test.go b/internal/notify/webhook_format_test.go index ce37e3b..fcd175e 100644 --- a/internal/notify/webhook_format_test.go +++ b/internal/notify/webhook_format_test.go @@ -35,6 +35,7 @@ func TestDetectWebhookFormat(t *testing.T) { want string }{ {"https://hooks.slack.com/services/T000/B000/XXXX", WebhookFormatSlack}, + {"https://hooks.slack-gov.com/services/T000/B000/XXXX", WebhookFormatSlack}, {"https://HOOKS.SLACK.COM/services/T000/B000/XXXX", WebhookFormatSlack}, {"https://discord.com/api/webhooks/123/token", WebhookFormatDiscord}, {"https://discordapp.com/api/webhooks/123/token", WebhookFormatDiscord}, diff --git a/internal/watcher/autowatch.go b/internal/watcher/autowatch.go index 297fdd5..bf53781 100644 --- a/internal/watcher/autowatch.go +++ b/internal/watcher/autowatch.go @@ -1,7 +1,10 @@ package watcher import ( + "context" + "errors" "fmt" + "net/http" "strings" "time" @@ -26,9 +29,11 @@ var AutoWatchDescription = map[string]string{ config.AutoWatchReviewRequested: "PRs where your review is requested", } -// Searcher runs GitHub pull request searches. +// Searcher runs GitHub pull request searches. GetPullRequest tells a review request +// that was withdrawn apart from a PR that was merged or closed. type Searcher interface { - SearchPullRequests(query string) (*github.SearchResult, error) + SearchPullRequests(ctx context.Context, query string) (*github.SearchResult, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) } // SyncResult reports what SyncAutoWatch changed. @@ -42,12 +47,13 @@ type SyncResult struct { // SyncAutoWatch runs the search of every source in sources and updates the watch list: // matching PRs that aren't watched yet are added (tagged with their source), and PRs // added for "review-requested" that no longer match are removed, since the request -// was withdrawn or answered. PRs added for "mine" are not removed when they stop -// matching: that means they were merged or closed, and `prw run` should still notify -// about it before dropping them. PRs watched by hand are never touched. +// was withdrawn or answered. PRs that stopped matching because they were merged or +// closed are kept instead (for "mine" that's always the reason), so that `prw run` +// still notifies about it before dropping them. PRs watched by hand are never touched. // -// It returns an error only if every search failed; other problems are Warnings. -func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { +// It returns an error if every search failed or ctx was cancelled (then nothing is +// changed); other problems are Warnings. +func SyncAutoWatch(ctx context.Context, client Searcher, sources []string) (*SyncResult, error) { result := &SyncResult{} found := map[string]*github.SearchResult{} var lastErr error @@ -58,7 +64,7 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { result.Warnings = append(result.Warnings, fmt.Sprintf("unknown auto-watch source %q ignored", source)) continue } - res, err := client.SearchPullRequests(query) + res, err := client.SearchPullRequests(ctx, query) if err != nil { lastErr = err result.Warnings = append(result.Warnings, fmt.Sprintf("search for %s failed: %v", AutoWatchDescription[source], err)) @@ -73,6 +79,10 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { if len(found) == 0 && lastErr != nil { return result, lastErr } + withdrawn := withdrawnReviewRequests(ctx, client, found) + if err := ctx.Err(); err != nil { + return result, err // interrupted: lookups that didn't finish prove nothing + } err := config.Update(func(c *config.Config) error { changed := false @@ -97,7 +107,8 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { } kept := c.WatchedPRs[:0] for _, pr := range c.WatchedPRs { - if pr.Source == source && !matches[prKey(pr.Owner, pr.Repo, pr.Number)] { + key := prKey(pr.Owner, pr.Repo, pr.Number) + if pr.Source == source && !matches[key] && withdrawn[key] { result.Removed = append(result.Removed, pr) changed = true continue @@ -117,12 +128,50 @@ func SyncAutoWatch(client Searcher, sources []string) (*SyncResult, error) { return result, nil } +// withdrawnReviewRequests finds the review-request PRs that can be dropped: they no +// longer match their search and are still open (so the request was withdrawn or +// answered), or are gone (404). The search only returns open PRs, so a merged or +// closed PR looks the same until it's looked up; those are kept for the regular check +// to notify about. A failed lookup (network, rate limit, cancellation) proves nothing, +// so that PR is kept until the next sync. It runs before the config is locked, as it +// makes API calls. +func withdrawnReviewRequests(ctx context.Context, client Searcher, found map[string]*github.SearchResult) map[string]bool { + res, ok := found[config.AutoWatchReviewRequested] + if !ok || res.Incomplete { + return nil // nothing will be removed + } + cfg, err := config.Load() + if err != nil { + return nil + } + matches := make(map[string]bool, len(res.PRs)) + for _, pr := range res.PRs { + matches[prKey(pr.Owner, pr.Repo, pr.Number)] = true + } + withdrawn := map[string]bool{} + for _, pr := range cfg.WatchedPRs { + key := prKey(pr.Owner, pr.Repo, pr.Number) + if pr.Source != config.AutoWatchReviewRequested || matches[key] { + continue + } + ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number) + var apiErr *github.APIError + switch { + case err == nil: + withdrawn[key] = !ghPR.IsClosed() + case errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound: + withdrawn[key] = true // deleted, or no longer visible to us + } + } + return withdrawn +} + func prKey(owner, repo string, number int) string { return strings.ToLower(owner) + "/" + strings.ToLower(repo) + "#" + fmt.Sprint(number) } // syncAutoWatch refreshes the auto-watch searches during `prw run` and reports changes. -func (w *Watcher) syncAutoWatch() { +func (w *Watcher) syncAutoWatch(ctx context.Context) { sources := w.config.AutoWatch if fresh, err := config.Load(); err == nil { sources = fresh.AutoWatch // `prw watch --mine` may have run since startup @@ -136,7 +185,10 @@ func (w *Watcher) syncAutoWatch() { if !ok { return } - res, err := SyncAutoWatch(searcher, sources) + res, err := SyncAutoWatch(ctx, searcher, sources) + if ctx.Err() != nil { + return // stopping; the failed searches and lookups aren't worth reporting + } for _, warning := range res.Warnings { fmt.Printf("Warning: auto-watch: %s\n", warning) } diff --git a/internal/watcher/autowatch_test.go b/internal/watcher/autowatch_test.go index 08fbe63..d08bf4e 100644 --- a/internal/watcher/autowatch_test.go +++ b/internal/watcher/autowatch_test.go @@ -22,7 +22,7 @@ type fakeSearcher struct { searches int } -func (f *fakeSearcher) SearchPullRequests(query string) (*github.SearchResult, error) { +func (f *fakeSearcher) SearchPullRequests(ctx context.Context, query string) (*github.SearchResult, error) { f.searches++ for source, q := range autoWatchQueries { if q == query { @@ -82,7 +82,7 @@ func TestSyncAutoWatch_AddsMatchesTaggedWithSource(t *testing.T) { config.AutoWatchReviewRequested: found(3), }} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -115,8 +115,9 @@ func TestSyncAutoWatch_DropsWithdrawnReviewRequestsButKeepsMine(t *testing.T) { config.AutoWatchMine: found(), config.AutoWatchReviewRequested: found(21), }} + s.prs = map[string]*github.PullRequest{"owner/repo/20": openReviewPR(20)} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -138,7 +139,7 @@ func TestSyncAutoWatch_IncompleteResultsNeverRemove(t *testing.T) { partial.Incomplete, partial.Total = true, 2 s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: partial}} - res, err := SyncAutoWatch(s, []string{config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchReviewRequested}) if err != nil { t.Fatal(err) } @@ -161,7 +162,7 @@ func TestSyncAutoWatch_OneFailedSearchDoesNotBlockTheOther(t *testing.T) { errs: map[string]error{config.AutoWatchReviewRequested: errors.New("boom")}, } - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine, config.AutoWatchReviewRequested}) if err != nil { t.Fatalf("a partial failure is a warning, got %v", err) } @@ -179,7 +180,7 @@ func TestSyncAutoWatch_AllSearchesFailing(t *testing.T) { boom := errors.New("network down") s := &fakeSearcher{errs: map[string]error{config.AutoWatchMine: boom}} - if _, err := SyncAutoWatch(s, []string{config.AutoWatchMine}); !errors.Is(err, boom) { + if _, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine}); !errors.Is(err, boom) { t.Errorf("err = %v, want the search error", err) } } @@ -189,7 +190,7 @@ func TestSyncAutoWatch_SkipsSourceDisabledMeanwhile(t *testing.T) { saveConfig(t, &config.Config{}) // `prw unwatch --mine` ran after the caller read the config s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchMine: found(1)}} - res, err := SyncAutoWatch(s, []string{config.AutoWatchMine}) + res, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchMine}) if err != nil { t.Fatal(err) } @@ -201,7 +202,7 @@ func TestSyncAutoWatch_SkipsSourceDisabledMeanwhile(t *testing.T) { func TestSyncAutoWatch_UnknownSource(t *testing.T) { useTempConfig(t) saveConfig(t, &config.Config{AutoWatch: []string{"bogus"}}) - res, err := SyncAutoWatch(&fakeSearcher{}, []string{"bogus"}) + res, err := SyncAutoWatch(context.Background(), &fakeSearcher{}, []string{"bogus"}) if err != nil { t.Fatal(err) } @@ -301,3 +302,113 @@ func TestRun_PicksUpAutoWatchEnabledWhileRunning(t *testing.T) { t.Errorf("watch list = %s", got) } } + +func TestRunOnce_NotifiesReviewRequestMergedWhileStopped(t *testing.T) { + // The PR was merged while prw wasn't running: the is:open search no longer returns + // it, which must not be mistaken for a withdrawn review request. + useTempConfig(t) + cfg := &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{ + {Owner: "owner", Repo: "repo", Number: 20, Source: config.AutoWatchReviewRequested, LastKnownState: "success"}, + }, + } + saveConfig(t, cfg) + merged := newPR(20, "sha20") + merged.State, merged.Merged = "closed", true + s := &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found()}} + s.prs = map[string]*github.PullRequest{"owner/repo/20": merged} + notifier := &mockNotifier{} + + if err := New(s, cfg, notifier).RunOnce(context.Background()); err != nil { + t.Fatal(err) + } + if len(notifier.events) != 1 || notifier.events[0].CurrentState != "merged" { + t.Errorf("events = %+v, want one merged notification", notifier.events) + } + if got := watchList(t); got != "" { + t.Errorf("watch list = %q, want the merged PR dropped after notifying", got) + } +} + +func openReviewPR(number int) *github.PullRequest { + pr := newPR(number, fmt.Sprintf("sha%d", number)) + pr.State = "open" + return pr +} + +// failingLookups answers GetPullRequest with errs (keyed like prs) for the given PRs. +type failingLookups struct { + *fakeSearcher + errs map[string]error +} + +func (f *failingLookups) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + if err := f.errs[fmt.Sprintf("%s/%s/%d", owner, repo, number)]; err != nil { + return nil, err + } + return f.fakeSearcher.GetPullRequest(ctx, owner, repo, number) +} + +func TestSyncAutoWatch_RemovesReviewRequestsOnlyWhenProvenWithdrawn(t *testing.T) { + useTempConfig(t) + reviewPR := func(n int) config.WatchedPR { + return config.WatchedPR{Owner: "owner", Repo: "repo", Number: n, Source: config.AutoWatchReviewRequested} + } + saveConfig(t, &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{reviewPR(1), reviewPR(2), reviewPR(3)}, + }) + s := &failingLookups{ + fakeSearcher: &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found()}}, + errs: map[string]error{ + "owner/repo/2": &github.APIError{StatusCode: 404}, // deleted or no access + "owner/repo/3": errors.New("connection reset"), + }, + } + s.prs = map[string]*github.PullRequest{"owner/repo/1": openReviewPR(1)} + + if _, err := SyncAutoWatch(context.Background(), s, []string{config.AutoWatchReviewRequested}); err != nil { + t.Fatal(err) + } + if got := watchList(t); got != "3:review-requested" { + t.Errorf("watch list = %q, want #1 (open) and #2 (404) dropped, #3 (lookup failed) kept", got) + } +} + +func TestSyncAutoWatch_InterruptedDuringLookupsChangesNothing(t *testing.T) { + useTempConfig(t) + saveConfig(t, &config.Config{ + AutoWatch: []string{config.AutoWatchReviewRequested}, + WatchedPRs: []config.WatchedPR{ + {Owner: "owner", Repo: "repo", Number: 1, Source: config.AutoWatchReviewRequested}, + {Owner: "owner", Repo: "repo", Number: 2, Source: config.AutoWatchReviewRequested}, + }, + }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + s := &failingLookups{ + fakeSearcher: &fakeSearcher{results: map[string]*github.SearchResult{config.AutoWatchReviewRequested: found(9)}}, + } + s.prs = map[string]*github.PullRequest{"owner/repo/1": openReviewPR(1)} + s.errs = map[string]error{"owner/repo/2": context.Canceled} + cancelOnLookup := &cancellingSearcher{failingLookups: s, cancel: cancel} + + if _, err := SyncAutoWatch(ctx, cancelOnLookup, []string{config.AutoWatchReviewRequested}); !errors.Is(err, context.Canceled) { + t.Fatalf("err = %v, want context.Canceled", err) + } + if got := watchList(t); got != "1:review-requested 2:review-requested" { + t.Errorf("watch list = %q, want it untouched (no removals, no additions)", got) + } +} + +// cancellingSearcher cancels the context (Ctrl-C) when the first PR lookup starts. +type cancellingSearcher struct { + *failingLookups + cancel context.CancelFunc +} + +func (c *cancellingSearcher) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { + c.cancel() + return c.failingLookups.GetPullRequest(ctx, owner, repo, number) +} diff --git a/internal/watcher/closed_pr_test.go b/internal/watcher/closed_pr_test.go index 6127ddc..f5e69bd 100644 --- a/internal/watcher/closed_pr_test.go +++ b/internal/watcher/closed_pr_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "testing" "github.com/devblac/prw/internal/config" @@ -19,7 +20,7 @@ func TestCheckPR_MergedPRIsRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "success")}} w := New(client, cfg, notifier) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -44,7 +45,7 @@ func TestCheckPR_ClosedWithoutMergeIsRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "failure")}} w := New(client, cfg, notifier) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -62,7 +63,7 @@ func TestCheckPR_MergedNotifiesEvenWithFailFilter(t *testing.T) { cfg := &config.Config{NotificationFilter: config.NotificationFilterFail, WatchedPRs: []config.WatchedPR{watched(1, "success")}} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("unexpected error: %v", err) } if len(notifier.events) != 1 { @@ -80,7 +81,7 @@ func TestCheckPR_OpenPRIsNotRemoved(t *testing.T) { cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "pending")}} w := New(client, cfg, &mockNotifier{}) - closed, err := w.checkPR(&cfg.WatchedPRs[0]) + closed, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -107,7 +108,7 @@ func TestCheckAllPRs_RemovesOnlyClosedPRs(t *testing.T) { inMemory := &config.Config{PollIntervalSeconds: 20, WatchedPRs: []config.WatchedPR{watched(1, "pending"), watched(2, "success")}} w := New(client, inMemory, &mockNotifier{}) - w.checkAllPRs() + w.checkAllPRs(context.Background()) if len(w.config.WatchedPRs) != 1 || w.config.WatchedPRs[0].Number != 1 { t.Fatalf("in-memory watch list after checkAllPRs = %+v, want only #1", w.config.WatchedPRs) @@ -144,7 +145,7 @@ func TestCheckAllPRs_RemovalSurvivesConcurrentChange(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 1 || loaded.WatchedPRs[0].Number != 2 { diff --git a/internal/watcher/noci_test.go b/internal/watcher/noci_test.go new file mode 100644 index 0000000..d5b32e9 --- /dev/null +++ b/internal/watcher/noci_test.go @@ -0,0 +1,104 @@ +package watcher + +import ( + "context" + "testing" + "time" + + "github.com/devblac/prw/internal/config" + "github.com/devblac/prw/internal/github" +) + +func quietPR(number int, sha string, quietFor time.Duration, now time.Time) *github.PullRequest { + pr := newPR(number, sha) + pr.State = "open" + pr.UpdatedAt = now.Add(-quietFor) + return pr +} + +func TestCheckPR_ReportsNoCIWithoutNotifying(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", time.Hour, now)}} + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "pending")}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { + t.Errorf("state = %q, want no_ci", got) + } + if len(notifier.events) != 0 { + t.Errorf("pending → no_ci must not notify, got %+v", notifier.events[0]) + } +} + +func TestCheckPR_StaysPendingDuringGracePeriod(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", 20*time.Second, now)}} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "")}} + w := New(client, cfg, &mockNotifier{}) + w.now = func() time.Time { return now } + + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) + if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" { + t.Errorf("a PR pushed 20s ago must stay pending while CI registers, got %q", got) + } +} + +func TestCheckPR_NotifiesWhenCIShowsUpLater(t *testing.T) { + // e.g. a stacked PR is retargeted to main and CI starts running. + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", time.Minute, now)}, + checkRuns: map[string][]github.CheckRun{"sha1": {{Name: "Test", Status: "completed", Conclusion: "success"}}}, + } + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) + if len(notifier.events) != 1 || notifier.events[0].PreviousState != github.StateNoCI || notifier.events[0].CurrentState != "success" { + t.Errorf("events = %+v, want one no_ci → success", notifier.events) + } +} + +func TestCheckPR_StaysNoCIWhenOnlyMetadataChanges(t *testing.T) { + // A comment or label bumps updated_at, but no check or status appeared. + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha1", 10*time.Second, now)}} + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { + t.Fatal(err) + } + if got := cfg.WatchedPRs[0].LastKnownState; got != github.StateNoCI { + t.Errorf("state = %q, want no_ci", got) + } + if len(notifier.events) != 0 { + t.Errorf("a comment must not notify no_ci → pending, got %+v", notifier.events[0]) + } +} + +func TestCheckPR_NoCIToPendingWhenCIStartsAfterAPush(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + client := &mockGitHubClient{ + prs: map[string]*github.PullRequest{"owner/repo/1": quietPR(1, "sha2", 10*time.Second, now)}, + checkRuns: map[string][]github.CheckRun{"sha2": {{Name: "Test", Status: "queued"}}}, + } + notifier := &mockNotifier{} + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, github.StateNoCI)}} + w := New(client, cfg, notifier) + w.now = func() time.Time { return now } + + w.checkPR(context.Background(), &cfg.WatchedPRs[0]) + if got := cfg.WatchedPRs[0].LastKnownState; got != "pending" || len(notifier.events) != 1 { + t.Errorf("state = %q events = %d, want pending with one notification once a check exists", got, len(notifier.events)) + } +} diff --git a/internal/watcher/ratelimit_test.go b/internal/watcher/ratelimit_test.go index 146883f..d15e962 100644 --- a/internal/watcher/ratelimit_test.go +++ b/internal/watcher/ratelimit_test.go @@ -16,12 +16,12 @@ type countingClient struct { prErr error } -func (c *countingClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (c *countingClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { c.calls++ if c.prErr != nil { return nil, c.prErr } - return c.mockGitHubClient.GetPullRequest(owner, repo, number) + return c.mockGitHubClient.GetPullRequest(context.Background(), owner, repo, number) } func TestCheckAllPRs_StopsAndPausesOnRateLimit(t *testing.T) { @@ -37,7 +37,7 @@ func TestCheckAllPRs_StopsAndPausesOnRateLimit(t *testing.T) { w := New(client, cfg, &mockNotifier{}) w.now = func() time.Time { return reset.Add(-10 * time.Minute) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) if client.calls != 1 { t.Errorf("must stop after the first rate-limited request, made %d", client.calls) @@ -95,7 +95,7 @@ func TestCheckAllPRs_OtherErrorsDoNotPause(t *testing.T) { } w := New(client, cfg, &mockNotifier{}) - w.checkAllPRs() + w.checkAllPRs(context.Background()) if client.calls != 2 { t.Errorf("a non-rate-limit error must not stop the round, made %d calls", client.calls) diff --git a/internal/watcher/watcher.go b/internal/watcher/watcher.go index 1dc9ada..53aa364 100644 --- a/internal/watcher/watcher.go +++ b/internal/watcher/watcher.go @@ -12,9 +12,9 @@ import ( // GitHubClient defines the interface for GitHub operations. type GitHubClient interface { - GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) - GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) - GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) + GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) + GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) + GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) } // ConfigStore defines the interface for config persistence. @@ -70,10 +70,10 @@ func (w *Watcher) Run(ctx context.Context) error { // Check immediately on startup if len(w.config.AutoWatch) > 0 { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } - w.checkAllPRs() + w.checkAllPRs(ctx) for { select { @@ -86,10 +86,10 @@ func (w *Watcher) Run(ctx context.Context) error { } w.reloadWatchedPRs() if len(w.config.AutoWatch) > 0 && w.now().Sub(w.lastAutoWatch) >= AutoWatchInterval { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } - w.checkAllPRs() + w.checkAllPRs(ctx) } } } @@ -114,23 +114,28 @@ func (w *Watcher) reloadWatchedPRs() { func (w *Watcher) RunOnce(ctx context.Context) error { fmt.Printf("Running one-time check with %d second poll interval...\n", w.config.PollIntervalSeconds) if len(w.config.AutoWatch) > 0 { - w.syncAutoWatch() + w.syncAutoWatch(ctx) w.reloadWatchedPRs() } if len(w.config.WatchedPRs) == 0 { fmt.Println(noPRsMessage) return nil } - w.checkAllPRs() + w.checkAllPRs(ctx) return nil } -func (w *Watcher) checkAllPRs() { +func (w *Watcher) checkAllPRs(ctx context.Context) { var stillWatched, done []config.WatchedPR for i := range w.config.WatchedPRs { pr := w.config.WatchedPRs[i] - closed, err := w.checkPR(&pr) + closed, err := w.checkPR(ctx, &pr) + if err != nil && ctx.Err() != nil { + // Stopped mid-round (Ctrl-C): keep this and the remaining PRs unchanged. + stillWatched = append(stillWatched, w.config.WatchedPRs[i:]...) + break + } if err != nil { stillWatched = append(stillWatched, pr) // keep watching; preserve any partial update (e.g. title) if limited, resetAt := github.IsRateLimited(err); limited { @@ -176,9 +181,9 @@ func (w *Watcher) pauseForRateLimit(resetAt time.Time) { // checkPR checks one PR's CI status and reports whether it is now closed (merged or // not), in which case the caller should stop watching it. -func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { +func (w *Watcher) checkPR(ctx context.Context, pr *config.WatchedPR) (closed bool, err error) { // Fetch the PR to get its state and current head SHA - ghPR, err := w.client.GetPullRequest(pr.Owner, pr.Repo, pr.Number) + ghPR, err := w.client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number) if err != nil { return false, fmt.Errorf("failed to fetch PR: %w", err) } @@ -215,16 +220,24 @@ func (w *Watcher) checkPR(pr *config.WatchedPR) (closed bool, err error) { currentSHA := ghPR.Head.SHA // Aggregate commit statuses and check runs (GitHub Actions) for the head commit - ci, err := github.FetchCIStatus(w.client, pr.Owner, pr.Repo, currentSHA) + ci, err := github.FetchCIStatus(ctx, w.client, pr.Owner, pr.Repo, currentSHA) if err != nil { return false, fmt.Errorf("failed to fetch status: %w", err) } + github.ApplyNoCI(ci, ghPR.UpdatedAt, w.now()) currentState := ci.State previousState := github.NormalizeState(pr.LastKnownState) + if previousState == github.StateNoCI && currentState == "pending" && ci.Total == 0 { + // Comments and label changes also bump updated_at, which restarts the grace + // period; until an actual check or status shows up, the PR still has no CI. + currentState = github.StateNoCI + } - // Check if status changed - if previousState != "" && previousState != currentState && shouldNotify(w.config.NotificationFilter, currentState) { + // Check if status changed. Finding out that a PR has no CI is not a CI result, so + // it isn't announced (it would otherwise ping once for every such PR). + if previousState != "" && previousState != currentState && currentState != github.StateNoCI && + shouldNotify(w.config.NotificationFilter, currentState) { event := ¬ify.StatusChangeEvent{ Owner: pr.Owner, Repo: pr.Repo, diff --git a/internal/watcher/watcher_persist_test.go b/internal/watcher/watcher_persist_test.go index 2bdd92d..80b057e 100644 --- a/internal/watcher/watcher_persist_test.go +++ b/internal/watcher/watcher_persist_test.go @@ -1,6 +1,7 @@ package watcher import ( + "context" "os" "path/filepath" "testing" @@ -63,7 +64,7 @@ func TestWatcherReloadsWatchListEachTick(t *testing.T) { t.Fatalf("watcher did not pick up the new watch list: %+v", w.config.WatchedPRs) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, err := config.Load() if err != nil { @@ -106,7 +107,7 @@ func TestWatcherDoesNotResurrectUnwatchedPR(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 1 || loaded.WatchedPRs[0].Number != 1 { @@ -141,7 +142,7 @@ func TestWatcherKeepsPRAddedDuringPoll(t *testing.T) { t.Fatal(err) } - w.checkAllPRs() + w.checkAllPRs(context.Background()) loaded, _ := config.Load() if len(loaded.WatchedPRs) != 2 { @@ -171,7 +172,7 @@ func TestWatcherDoesNotPersistRunOnlyOverrides(t *testing.T) { prs: map[string]*github.PullRequest{"owner/repo/1": newPR(1, "sha1")}, statuses: map[string]*github.CombinedStatus{"sha1": {State: "success", SHA: "sha1"}}, } - New(client, inMemory, &mockNotifier{}).checkAllPRs() + New(client, inMemory, &mockNotifier{}).checkAllPRs(context.Background()) loaded, _ := config.Load() if loaded.NotificationFilter != config.NotificationFilterChange { @@ -200,3 +201,26 @@ func TestWatcherReloadKeepsListOnLoadError(t *testing.T) { t.Errorf("a config read error must not empty the watch list: %+v", w.config.WatchedPRs) } } + +func TestCheckAllPRs_StoppedMidRoundKeepsEveryPR(t *testing.T) { + useTempConfig(t) + cfg := &config.Config{WatchedPRs: []config.WatchedPR{watched(1, "success"), watched(2, "failure")}} + if err := cfg.Save(); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() // Ctrl-C: every request now fails with context.Canceled + client := &mockGitHubClient{err: ctx.Err()} + notifier := &mockNotifier{} + w := New(client, cfg, notifier) + + w.checkAllPRs(ctx) + + loaded, _ := config.Load() + if len(loaded.WatchedPRs) != 2 || loaded.WatchedPRs[0].LastKnownState != "success" || loaded.WatchedPRs[1].LastKnownState != "failure" { + t.Errorf("watch list = %+v, want both PRs unchanged", loaded.WatchedPRs) + } + if len(notifier.events) != 0 { + t.Errorf("events = %+v", notifier.events) + } +} diff --git a/internal/watcher/watcher_test.go b/internal/watcher/watcher_test.go index 4cecfcd..53b45d8 100644 --- a/internal/watcher/watcher_test.go +++ b/internal/watcher/watcher_test.go @@ -20,14 +20,14 @@ type mockGitHubClient struct { err error } -func (m *mockGitHubClient) GetCheckRuns(owner, repo, ref string) ([]github.CheckRun, error) { +func (m *mockGitHubClient) GetCheckRuns(ctx context.Context, owner, repo, ref string) ([]github.CheckRun, error) { if m.err != nil { return nil, m.err } return m.checkRuns[ref], nil } -func (m *mockGitHubClient) GetPullRequest(owner, repo string, number int) (*github.PullRequest, error) { +func (m *mockGitHubClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { if m.err != nil { return nil, m.err } @@ -39,7 +39,7 @@ func (m *mockGitHubClient) GetPullRequest(owner, repo string, number int) (*gith return pr, nil } -func (m *mockGitHubClient) GetCombinedStatus(owner, repo, ref string) (*github.CombinedStatus, error) { +func (m *mockGitHubClient) GetCombinedStatus(ctx context.Context, owner, repo, ref string) (*github.CombinedStatus, error) { if m.err != nil { return nil, m.err } @@ -95,7 +95,7 @@ func TestWatcherNotificationError(t *testing.T) { w := New(client, cfg, notifier) // Should not return error, just print warning - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Errorf("checkPR should not fail when notification fails: %v", err) } } @@ -133,7 +133,7 @@ func TestWatcherNoStatusChange(t *testing.T) { w := New(client, cfg, notifier) // Check the PR once - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -175,7 +175,7 @@ func TestWatcherStatusChange(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -233,7 +233,7 @@ func TestWatcherFirstCheck(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -263,7 +263,7 @@ func TestWatcherGitHubError(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - _, err := w.checkPR(&cfg.WatchedPRs[0]) + _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err == nil { t.Error("expected error when GitHub API fails") } @@ -386,7 +386,7 @@ func TestWatcherUpdateTitle(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -425,7 +425,7 @@ func TestWatcherStatusError(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - _, err := w.checkPR(&cfg.WatchedPRs[0]) + _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]) if err == nil { t.Error("expected error when status fetch fails") } @@ -459,7 +459,7 @@ func TestWatcherCheckAllPRs(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - w.checkAllPRs() + w.checkAllPRs(context.Background()) // Both PRs should be notified if len(notifier.events) != 2 { @@ -502,7 +502,7 @@ func TestWatcherCheckAllPRsWithError(t *testing.T) { w := New(client, cfg, notifier) // This should handle errors gracefully and print warnings - w.checkAllPRs() + w.checkAllPRs(context.Background()) // Only one PR should be successfully checked if len(notifier.events) != 1 { @@ -560,7 +560,7 @@ func TestWatcherNotificationFilterFailOnly(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) } @@ -600,7 +600,7 @@ func TestWatcherNotificationFilterSuccessOnly(t *testing.T) { notifier := &mockNotifier{} w := New(client, cfg, notifier) - if _, err := w.checkPR(&cfg.WatchedPRs[0]); err != nil { + if _, err := w.checkPR(context.Background(), &cfg.WatchedPRs[0]); err != nil { t.Fatalf("checkPR failed: %v", err) }