From 341cff133e6fad5717d2ea87642e23428415c20b Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:21:19 -0400 Subject: [PATCH] test: drive the v3.5.0 failure arms end-to-end and the three unit-only misses The fix PRs that shipped in v3.5.0 (#638, #639, #640, #649) added src/ arms the bundle suite never reached, and the e2e run dropped from 100 % to 99.19 % lines without failing its 99 floor. Add one bundle scenario per reachable arm (exclusive-family refusal, bare /remove, the two milestone write failures, blunderbuss's per-reviewer retry after a 422, the sweep's per-base setup failure) and unit tests for the three branches only the bundle reached (labeledBySelf without a sender, the tokenLogin memo hit, a review whose user is gone). #649 moved the documented await artefact in approve.ts from line 502 to 516; the contributing guide now points at the right line. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- .../bundle/releaseV350FailureArms.test.ts | 172 ++++++++++++++++++ __tests__/plugins/approveEvents.test.ts | 12 ++ __tests__/plugins/approveReview.test.ts | 16 +- __tests__/plugins/blunderbuss.test.ts | 11 ++ docs/contributing.md | 2 +- 5 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 __tests__/bundle/releaseV350FailureArms.test.ts diff --git a/__tests__/bundle/releaseV350FailureArms.test.ts b/__tests__/bundle/releaseV350FailureArms.test.ts new file mode 100644 index 00000000..516eb754 --- /dev/null +++ b/__tests__/bundle/releaseV350FailureArms.test.ts @@ -0,0 +1,172 @@ +import type { FakeGithub } from './fakeGithub' +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 pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import { start } from './fakeGithub' +import { comment, configReads, helpersFor, repo, token } from './helpers' +import { runBundle } from './runBundle' + +vi.setConfig({ testTimeout: 30_000 }) + +// the src/ arms #638, #639 and #640 added that no bundle scenario drove: the exclusive-family refusal of two +// values (prefixed.ts), plain /remove without an argument (remove.ts), the two milestone write failures +// (milestone.ts), blunderbuss's one-at-a-time retry after GitHub refuses the batch (blunderbuss.ts +// requestEach), and the sweep's setup failure when a base branch's tree cannot be listed (sweep.ts) +describe('dist/index.js failure arms added for v3.5.0', () => { + let gh: FakeGithub + const { expectRequests, routeOwners } = helpersFor(() => gh) + const collaboratorRead = `GET ${repo}/collaborators/Codertocat` + + beforeAll(async () => { + gh = await start() + }) + afterEach(() => gh.reset()) + afterAll(() => gh.close()) + + function run(body: string, commands: string) { + return runBundle({ eventName: 'issue_comment', payload: comment(body), inputs: { ...token, 'prow-commands': commands }, apiUrl: gh.url }) + } + + describe('label commands', () => { + it('/level with two values is refused as exclusive, without reading or writing labels', async () => { + gh.route('GET', `${repo}/contents/.prowlabels.yaml`, { status: 200, body: labelFileContents }) + + const result = await run('/level sandbox incubation', '/level') + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes('level: /level takes one value; got level/sandbox, level/incubation'))).toBe(true) + expect(gh.requestsMatching('DELETE', /./)).toEqual([]) + expect(gh.requestsMatching('POST', /./)).toEqual([]) + expectRequests(configReads({ repo: '.prowlabels.yaml' }), []) + }) + + it('/remove by a collaborator with no label named fails as missing args without reading the issue', async () => { + gh.route('GET', `${repo}/collaborators/Codertocat`, { status: 204 }) + + const result = await run('/remove', '/remove') + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes('remove: command args missing from body'))).toBe(true) + expect(gh.requestsMatching('DELETE', /./)).toEqual([]) + expectRequests([collaboratorRead, ...configReads()], []) + }) + }) + + describe('/milestone', () => { + const milestonesRead = `GET ${repo}/milestones?state=open&per_page=100` + const issuePatch = `PATCH ${repo}/issues/1` + + it('clear whose issue update fails reports the write failure and fails the run', async () => { + gh.route('GET', `${repo}/collaborators/Codertocat`, { status: 204 }) + gh.route('PATCH', `${repo}/issues/1`, { status: 500, body: { message: 'boom' } }) + + const result = await run('/milestone clear', '/milestone') + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes('could not clear milestone'))).toBe(true) + expectRequests([collaboratorRead], [issuePatch]) + expect(gh.requestsMatching('PATCH', /\/issues\/1$/)[0].body).toEqual({ milestone: null }) + }) + + it('naming an existing milestone whose issue update fails reports the write failure and fails the run', async () => { + gh.route('GET', `${repo}/collaborators/Codertocat`, { status: 204 }) + gh.route('GET', `${repo}/milestones`, { status: 200, body: [{ number: 3, title: 'v1.0' }] }) + gh.route('PATCH', `${repo}/issues/1`, { status: 500, body: { message: 'boom' } }) + + const result = await run('/milestone v1.0', '/milestone') + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes('could not set milestone'))).toBe(true) + expectRequests([collaboratorRead], [milestonesRead, issuePatch]) + expect(gh.requestsMatching('PATCH', /\/issues\/1$/)[0].body).toEqual({ milestone: 3 }) + }) + }) + + describe('blunderbuss when GitHub refuses the batch', () => { + function requested() { + return gh.requestsMatching('POST', /\/pulls\/1\/requested_reviewers$/).map(r => (r.body as { reviewers: string[] }).reviewers) + } + + function openPullRequest() { + return runBundle({ eventName: 'pull_request', payload: pullReqOpenedEvent, inputs: token, apiUrl: gh.url }) + } + + it('retries each reviewer alone, warns about the one still refused and reports the accepted one', async () => { + routeOwners({ OWNERS: 'reviewers:\n- bob\n- carol\n' }, ['x.go']) + gh.route('POST', `${repo}/pulls/1/requested_reviewers`, (req) => { + const { reviewers } = req.body as { reviewers: string[] } + if (reviewers.length > 1 || reviewers[0] === 'carol') { + return { status: 422, body: { message: 'Reviews may only be requested from collaborators.' } } + } + return { status: 201, body: {} } + }) + + const result = await openPullRequest() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('::warning::blunderbuss: could not request review from carol on #1') + expect(result.stdout).toContain('blunderbuss: requested review from bob on #1') + const posts = requested() + expect(posts).toHaveLength(3) + expect([...posts[0]].sort()).toEqual(['bob', 'carol']) + expect(posts.slice(1).map(r => r.join()).sort()).toEqual(['bob', 'carol']) + }) + + it('fails the run naming the refusal when every reviewer is refused alone too', async () => { + routeOwners({ OWNERS: 'reviewers:\n- bob\n' }, ['x.go']) + gh.route('POST', `${repo}/pulls/1/requested_reviewers`, { status: 422, body: { message: 'Reviews may only be requested from collaborators.' } }) + + const result = await openPullRequest() + + expect(result.status, result.stdout).toBe(1) + expect(result.stdout).toContain('::warning::blunderbuss: could not request review from bob on #1') + expect(result.errors.join('\n')).toContain('could not request reviewers') + expect(requested()).toEqual([['bob'], ['bob']]) + }) + }) + + describe('schedule sweep job', () => { + const listPage = (page: number) => `GET ${repo}/pulls?state=open&sort=updated&direction=desc&per_page=100&page=${page}` + + function freshPr(number: number) { + const stamp = new Date().toISOString() + return { + ...structuredClone(pullReqListPulls[0]), + number, + labels: [], + created_at: stamp, + updated_at: stamp, + requested_reviewers: [], + assignees: [], + draft: false, + user: { login: 'dave' }, + head: { sha: `sha${number}`, repo: { full_name: 'dave/Hello-World' } }, + base: { ref: 'master', sha: 'basesha' }, + } + } + + it('a base branch whose tree cannot be listed fails each of its pull requests at setup, reading nothing else for them', async () => { + gh.route('GET', new RegExp(`^${repo}/pulls\\?`), (req) => { + const page = new URL(req.path, gh.url).searchParams.get('page') + return { status: 200, body: page === '1' ? [freshPr(1), freshPr(2)] : [] } + }) + gh.route('GET', `${repo}/git/trees/master`, { status: 500, body: { message: 'boom' } }) + + const result = await runBundle({ eventName: 'schedule', payload: {}, inputs: { ...token, jobs: 'sweep' }, apiUrl: gh.url }) + + expect(result.status, result.stdout).toBe(1) + expect(result.stdout).toContain('sweep: 2 candidates updated since') + expect(result.stdout).toContain('sweep: #1 evaluated with 1 error(s)') + expect(result.stdout).toContain('sweep: #2 evaluated with 1 error(s)') + const failure = result.errors.find(e => e.includes('sweep: 2 pull request(s) failed')) + expect(failure).toBeDefined() + expect(failure).toContain('#1 (setup: error listing the tree of master: HttpError: boom)') + expect(failure).toContain('#2 (setup: error listing the tree of master: HttpError: boom)') + // the tree is probed once for the shared base branch; neither pull request is read further + expectRequests(configReads(), [listPage(1), `GET ${repo}/git/trees/master?recursive=true`]) + }) + }) +}) diff --git a/__tests__/plugins/approveEvents.test.ts b/__tests__/plugins/approveEvents.test.ts index 107d12a3..e5a78e36 100644 --- a/__tests__/plugins/approveEvents.test.ts +++ b/__tests__/plugins/approveEvents.test.ts @@ -227,6 +227,18 @@ describe('approveOnPullRequest', () => { expect(debug).toHaveBeenCalledWith(`approve: ${action} approved by this bot's own token; nothing to re-evaluate`) }) + // GITHUB_TOKEN's own writes raise no event, so a payload without a sender is somebody else's label + it('labeled approved with no sender in the payload is evaluated without reading the token\'s login', async () => { + const graphqlCalls: utils.GraphqlCall[] = [] + server.use(utils.mergeQueueGraphql({ enabled: false, viewer: 'my-app' }, graphqlCalls).handler) + const writes = serve({ labels: ['approved'], comments: [{ body: '/approve', user: { login: 'bob' } }] }) + + await approveOnPullRequest(prEvent('labeled', { label: { name: 'approved' }, sender: undefined })) + + await expect(writes.removeLabel.called()).resolves.toBe('called') + expect(graphqlCalls.filter(call => call.query.includes('viewer'))).toEqual([]) + }) + it('labeled approved by another login while the token\'s login is unreadable is evaluated', async () => { server.use(utils.mergeQueueGraphql({ enabled: false, viewer: null }).handler) const writes = serve({ labels: ['approved'], comments: [{ body: '/approve', user: { login: 'bob' } }] }) diff --git a/__tests__/plugins/approveReview.test.ts b/__tests__/plugins/approveReview.test.ts index 24824ad7..4ac7c2d8 100644 --- a/__tests__/plugins/approveReview.test.ts +++ b/__tests__/plugins/approveReview.test.ts @@ -8,7 +8,7 @@ import { setupServer } from 'msw/node' import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' import { approveOnPullRequest, approveOnReview } from '../../src/plugins/approve' -import { forbiddenWarning, isOwnReview, notPermittedWarning, reviewBody, reviewMarker, syncApprovalReview, tokenIdentity, withdrawalReason } from '../../src/plugins/approveReview' +import { forbiddenWarning, isOwnReview, notPermittedWarning, reviewBody, reviewMarker, syncApprovalReview, tokenIdentity, tokenLogin, withdrawalReason } from '../../src/plugins/approveReview' import { handlePullReq } from '../../src/pullReq/handlePullReq' import { handlePullReqReview } from '../../src/pullReq/handlePullReqReview' import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' @@ -549,6 +549,20 @@ describe('tokenIdentity', () => { }) }) +describe('tokenLogin', () => { + it('asks the GraphQL viewer once per API base URL and hands later callers the same promise', async () => { + const graphql = vi.fn().mockResolvedValue({ viewer: { login: 'Prow-Bot' } }) + const octokit = { graphql } as unknown as Parameters[0] + + const first = tokenLogin(octokit) + const second = tokenLogin(octokit) + + expect(second).toBe(first) + await expect(first).resolves.toBe('prow-bot') + expect(graphql).toHaveBeenCalledTimes(1) + }) +}) + describe('syncApprovalReview', () => { it('a non-Error rejection of the review is an error', async () => { const octokit = { pulls: { createReview: vi.fn().mockRejectedValue('boom') } } as unknown as Parameters[0] diff --git a/__tests__/plugins/blunderbuss.test.ts b/__tests__/plugins/blunderbuss.test.ts index c114799e..2372ddff 100644 --- a/__tests__/plugins/blunderbuss.test.ts +++ b/__tests__/plugins/blunderbuss.test.ts @@ -239,6 +239,17 @@ describe('blunderbuss handler', () => { expect(await requested(observe)).toEqual(['carol', 'dave', 'rita']) }) + it('a review whose user is gone counts for nobody: every candidate is still eligible', async () => { + serveConfig() + servePr(owners, ['sdk/x.go']) + server.use(http.get(`${repo}/pulls/1/reviews`, utils.mockResponse(200, [{ id: 1, user: null, state: 'COMMENTED' }]))) + const observe = observeRequestReviewers() + + await blunderbuss(prEvent('opened'), first) + + expect(await requested(observe)).toEqual(['alice', 'bob']) + }) + it('does nothing when every candidate has already reviewed', async () => { serveConfig() servePr(owners, ['sdk/x.go'], { reviews: { alice: 'APPROVED', bob: 'APPROVED', carol: 'CHANGES_REQUESTED', dave: 'COMMENTED', rita: 'DISMISSED' } }) diff --git a/docs/contributing.md b/docs/contributing.md index 884d0be6..2bedb2bb 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -91,7 +91,7 @@ a nullish throw fails `toThrow` and `.rejects.toThrow` whatever the argument; ### Phantom "uncovered" branches after an `await` The unit run lists four branches as uncovered that every test run in fact -drives: `close.ts:37`, `reopen.ts:34`, `approve.ts:502` and `auth.ts:237` +drives: `close.ts:37`, `reopen.ts:34`, `approve.ts:516` and `auth.ts:237` (it reports 1815/1819 branches because of them; the e2e run repeats the last two). Each is an `if` with no `else` whose condition was assigned from an expression in which an `await` is only sometimes reached (`a || (await b())`,