From 8c71ae994cfee1e5d7b9ab22242cd7e696cb2c5a Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 02:58:49 -0400 Subject: [PATCH] test(sweep): drive the delayed-reviewer pass's failure arm end-to-end and raise the e2e branch floor to 99 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bundle acceptance run reached every src/ branch but one: the catch of the delayed-only pass in src/cronJobs/sweep.ts (the forEachLimited over delayedOnly), whose String(e) fold had zero e2e hits while the candidate pass's equivalent at :132 is driven by nonErrorRejectionPluginArms.test.ts. __tests__/bundle/blunderbussDelayFailure.test.ts runs the sweep over two delayed-only pull requests and asserts that a failed timeline read, and a review read rejecting with a string via nonErrorRejectionPreload.cjs, are each recorded as 'blunderbuss: …' against that number only, that the other pull request still gets reviewers, and that the run fails once naming it. With no zero-hit e2e branch left (1686/1688, the two misses being the negative-count await artifacts docs/contributing.md describes) the e2e branch floor in vitest.e2e-coverage.config.mjs rises from 98 to 99, the same floor the unit run already enforces. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- .../bundle/blunderbussDelayFailure.test.ts | 130 ++++++++++++++++++ docs/contributing.md | 2 +- vitest.e2e-coverage.config.mjs | 16 ++- 3 files changed, 140 insertions(+), 8 deletions(-) create mode 100644 __tests__/bundle/blunderbussDelayFailure.test.ts diff --git a/__tests__/bundle/blunderbussDelayFailure.test.ts b/__tests__/bundle/blunderbussDelayFailure.test.ts new file mode 100644 index 00000000..10ddb9ad --- /dev/null +++ b/__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: ` 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) { + 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`]) + }) +}) diff --git a/docs/contributing.md b/docs/contributing.md index 5f1ccc83..5a37659f 100644 --- a/docs/contributing.md +++ b/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/` diff --git a/vitest.e2e-coverage.config.mjs b/vitest.e2e-coverage.config.mjs index c9d00dbd..0c4e6c81 100644 --- a/vitest.e2e-coverage.config.mjs +++ b/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, },