test(sweep): drive the delayed-reviewer pass's failure arm end-to-end and raise the e2e branch floor to 99

__tests__/bundle/blunderbussDelayFailure.test.ts

@@ -0,0 +1,130 @@
+import type { FakeGithub } from './fakeGithub'
+import { Buffer } from 'node:buffer'
+import path from 'node:path'
+import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'
+
+import labelFileContents from '../fixtures/labels/labelFileContentsResp.json'
+import pullReqListPulls from '../fixtures/pullReq/pullReqListPulls.json'
+import { blobSha } from '../utils/ownersData'
+import { start } from './fakeGithub'
+import { repo, token } from './helpers'
+import { runBundle } from './runBundle'
+
+vi.setConfig({ testTimeout: 30_000 })
+
+const preload = path.resolve(__dirname, 'nonErrorRejectionPreload.cjs')
+
+// The failure arm of the delayed-reviewers pass (src/cronJobs/sweep.ts, the
+// `forEachLimited(delayedOnly, …)` catch): a pull request outside the plain
+// window is only ever visited by blunderbuss, and when that visit throws the
+// sweep records `blunderbuss: <message>` against that number, keeps going for
+// the others, and fails the run at the end naming every failure. The catch
+// folds a non-Error with `String(e)`, like the candidate pass does (:132);
+// that arm is driven the same way nonErrorRejectionPluginArms.test.ts drives
+// the candidate pass, with the preload handing the bundle a string rejection.
+describe('dist/index.js delayed blunderbuss failing on one pull request', () => {
+  let gh: FakeGithub
+  const owners = { OWNERS: 'reviewers:\n- bob\n' }
+
+  beforeAll(async () => {
+    gh = await start()
+  })
+  afterEach(() => gh.reset())
+  afterAll(() => gh.close())
+
+  function ago(minutes: number) {
+    return new Date(Date.now() - minutes * 60_000).toISOString()
+  }
+
+  function pull(number: number, minutesAgo: number) {
+    return {
+      ...structuredClone(pullReqListPulls[0]),
+      number,
+      created_at: ago(minutesAgo),
+      updated_at: ago(minutesAgo),
+      labels: [],
+      requested_reviewers: [],
+      requested_teams: [],
+      assignees: [],
+      draft: false,
+      user: { login: 'Codertocat' },
+      base: { ref: 'master', sha: 'basesha' },
+      head: { sha: `sha${number}`, repo: { full_name: 'Codertocat/Hello-World' } },
+    }
+  }
+
+  // both opened inside the default due window [now − 2h, now − 1h] and outside the plain
+  // window [now − 1h, now], so each is delayed-only: the sweep visits them for reviewers and nothing else
+  const pulls = [pull(1, 90), pull(2, 100)]
+
+  function serve(config: string, timelineOf2: { status: number, body: unknown }) {
+    const file = structuredClone(labelFileContents)
+    file.content = Buffer.from(config).toString('base64')
+    gh.route('GET', '/repos/Codertocat/.project/contents/prow.yaml', { status: 200, body: file })
+    gh.route('GET', new RegExp(`^${repo}/pulls\\?`), { status: 200, body: pulls })
+    for (const pr of pulls) {
+      gh.route('GET', `${repo}/pulls/${pr.number}`, { status: 200, body: pr })
+      gh.route('GET', `${repo}/pulls/${pr.number}/files`, { status: 200, body: [{ filename: 'x.go', status: 'modified' }] })
+      gh.route('GET', `${repo}/pulls/${pr.number}/reviews`, { status: 200, body: [] })
+      gh.route('POST', `${repo}/pulls/${pr.number}/requested_reviewers`, { status: 201, body: {} })
+    }
+    gh.route('GET', `${repo}/issues/1/timeline`, { status: 200, body: [] })
+    gh.route('GET', `${repo}/issues/2/timeline`, timelineOf2)
+    gh.route('GET', `${repo}/branches/master`, { status: 200, body: { name: 'master', commit: { sha: 'basesha' } } })
+    const tree = Object.keys(owners).map(path => ({ path, type: 'blob', sha: blobSha(path) }))
+    gh.route('GET', `${repo}/git/trees/master`, { status: 200, body: { sha: 'master', truncated: false, tree } })
+    gh.route('GET', `${repo}/git/trees/basesha`, { status: 200, body: { sha: 'basesha', truncated: false, tree } })
+    for (const [path, contents] of Object.entries(owners)) {
+      gh.route('GET', `${repo}/git/blobs/${blobSha(path)}`, { status: 200, body: { encoding: 'base64', content: Buffer.from(contents).toString('base64') } })
+    }
+    gh.route('GET', `${repo}/labels`, { status: 200, body: [{ name: 'approved' }, { name: 'lgtm' }] })
+  }
+
+  function runSweep(env?: Record<string, string>) {
+    return runBundle({ eventName: 'schedule', payload: {}, inputs: { ...token, jobs: 'sweep' }, apiUrl: gh.url, env })
+  }
+
+  function requested() {
+    return gh.requestsMatching('POST', /\/requested_reviewers$/).map(r => r.path).sort()
+  }
+
+  it('the timeline read of one delayed pull request failing: the other still gets reviewers and the run fails naming blunderbuss and the number', async () => {
+    serve('blunderbuss:\n  delay: 1h\n', { status: 500, body: { message: 'boom' } })
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(1)
+    expect(result.stdout).toContain('sweep: 0 candidates updated since')
+    expect(result.stdout).toContain('sweep: 2 more checked for delayed reviewers')
+    // #1 is unaffected by #2's failure
+    expect(requested()).toEqual([`${repo}/pulls/1/requested_reviewers`])
+    // the one failure is attributed to blunderbuss on #2 and carries the timeline error
+    const failure = result.errors.find(e => e.includes('sweep: 1 pull request(s) failed'))
+    expect(failure, result.errors.join('\n')).toContain('#2 (blunderbuss: could not read the timeline: HttpError: boom)')
+    expect(result.errors.filter(e => e.includes('failed'))).toHaveLength(1)
+  })
+
+  it('the review read of one delayed pull request rejecting with a string: the failure carries the string unwrapped and the other still gets reviewers', async () => {
+    serve('blunderbuss:\n  delay: 1h\n', { status: 200, body: [] })
+    const reviewsRead = `GET ${repo}/pulls/2/reviews`
+
+    const result = await runSweep({ NODE_OPTIONS: `--require "${preload}"`, NON_ERROR_REJECT: reviewsRead })
+
+    expect(result.status, result.stdout).toBe(1)
+    expect(result.errors).toHaveLength(1)
+    expect(result.errors[0]).toBe(`TypeError: error handling cron job: Error: sweep: 1 pull request(s) failed: #2 (blunderbuss: ${reviewsRead} rejected with a string, not an Error)`)
+    expect(result.stdout).not.toContain('[object')
+    expect(result.stderr).not.toMatch(/Unhandled|unhandledRejection/)
+    expect(requested()).toEqual([`${repo}/pulls/1/requested_reviewers`])
+  })
+
+  it('both delayed pull requests healthy: the same routes with a readable timeline request reviewers for both and pass', async () => {
+    serve('blunderbuss:\n  delay: 1h\n', { status: 200, body: [] })
+
+    const result = await runSweep()
+
+    expect(result.status, result.stdout).toBe(0)
+    expect(result.errors).toEqual([])
+    expect(requested()).toEqual([`${repo}/pulls/1/requested_reviewers`, `${repo}/pulls/2/requested_reviewers`])
+  })
+})

docs/contributing.md

@@ -47,7 +47,7 @@ stay in step with `action.yml` and the label catalogue.
 | `npm test` | The whole Vitest suite |
 | `npm run test:coverage` | The suite with v8 coverage; CI enforces the thresholds in `vitest.config.mjs` (lines 99, branches 99, functions 99, statements 99, a floor set just under what `main` reaches) |
 | `npx vitest run __tests__/bundle` | Only the bundle acceptance harness |
-| `npm run test:coverage:e2e` | Only the bundle acceptance harness, run against a source-mapped bundle built from `__tests__/bundle/coverageEntry/` into `.coverage-bundle/` so its hits are reported against `src/`, written to `coverage-e2e/` so it never overwrites the unit run's `coverage/` (ncc's map is line-only, so the build also emits tsc's per-file maps into `.coverage-bundle/tsc/` and `coverageEntry/remap.mjs` composes their columns into it; without that, inner lines of a multi-line expression that tsc flattens onto one JS line report 0 hits). This is end-to-end-only coverage and is deliberately reported separately from `test:coverage`: the unit run already reaches every line, so a union would say nothing about what the bundle exercises, and the two runs' statement and branch maps come from different transforms (vite vs. tsc+webpack) and do not combine. Fails below 99 % lines / 98 % branches / 99 % functions / 99 % statements (`vitest.e2e-coverage.config.mjs`), a floor set just under what `main` reaches so a scenario that stops driving a `src/` path is caught |
+| `npm run test:coverage:e2e` | Only the bundle acceptance harness, run against a source-mapped bundle built from `__tests__/bundle/coverageEntry/` into `.coverage-bundle/` so its hits are reported against `src/`, written to `coverage-e2e/` so it never overwrites the unit run's `coverage/` (ncc's map is line-only, so the build also emits tsc's per-file maps into `.coverage-bundle/tsc/` and `coverageEntry/remap.mjs` composes their columns into it; without that, inner lines of a multi-line expression that tsc flattens onto one JS line report 0 hits). This is end-to-end-only coverage and is deliberately reported separately from `test:coverage`: the unit run already reaches every line, so a union would say nothing about what the bundle exercises, and the two runs' statement and branch maps come from different transforms (vite vs. tsc+webpack) and do not combine. Fails below 99 % lines / 99 % branches / 99 % functions / 99 % statements (`vitest.e2e-coverage.config.mjs`), a floor set just under what `main` reaches so a scenario that stops driving a `src/` path is caught |
 
 Unit tests under `__tests__/` import `src/` directly and mock the GitHub API
 with [msw](https://mswjs.io/). The acceptance harness in `__tests__/bundle/`

vitest.e2e-coverage.config.mjs

@@ -45,16 +45,18 @@ export default mergeConfig(base, defineConfig({
       autoAttachSubprocess: true,
       excludeAfterRemap: true,
       // Regression floor for the bundle-only run, set just under what main reaches
-      // (100 / 99.46 / 100 / 100 at e3119ad, 1686/1695 branches, two of them the
+      // (100 / 99.88 / 100 / 100 once blunderbussDelayFailure.test.ts drives the
+      // delayed pass's catch, 1686/1688 branches, the two misses being the
       // negative-count await artifacts described in docs/contributing.md;
-      // 99.95 / 97.96 / 100 / 99.96 at 1062ce7; 99.54 / 96.25 / 98.79 / 99.56 when
-      // first set at d93ecb8) so a scenario that stops exercising a src/ path fails
-      // this run instead of silently lowering the figure. A single lost branch is
-      // ~0.06 pp and will not cross the floor on its own; the floor bounds drift,
-      // it does not detect one-branch regressions.
+      // 100 / 99.46 / 100 / 100 at e3119ad; 99.95 / 97.96 / 100 / 99.96 at 1062ce7;
+      // 99.54 / 96.25 / 98.79 / 99.56 when first set at d93ecb8) so a scenario that
+      // stops exercising a src/ path fails this run instead of silently lowering
+      // the figure. A single lost branch is ~0.06 pp and will not cross the floor
+      // on its own; the floor bounds drift, it does not detect one-branch
+      // regressions. Branches now share the unit run's 99 floor.
       thresholds: {
         lines: 99,
-        branches: 98,
+        branches: 99,
         functions: 99,
         statements: 99,
       },