From d25517c904bc65525b104e13458af6c4f654e7fb Mon Sep 17 00:00:00 2001 From: quality Date: Mon, 5 Oct 2026 15:57:20 -0400 Subject: [PATCH 1/3] test(bundle): drive the approve plugin's evaluation arms through dist/index.js __tests__/bundle only reaches src/plugins/approve.ts' happy paths: on main @ 3fc21f2 the e2e run (npm run test:coverage:e2e) covers 164/193 of its lines while the unit suite covers all of them. Add approveEvaluationArms.test.ts to run the remaining arms against the fake api through the shipped bundle: - approveOnPullRequest skipping an unsupported action, approveOnReview skipping the mirrored approval review itself, and a payload without a pull request failing the run - evaluateApproval leaving a pull request alone when the branch tip has no OWNERS files although the probe did - the notifier for a pull request changing no files, for files no OWNERS file covers (listed last, sorted, nobody to suggest), and the up-to-date notifier that is neither re-posted nor edited - the updateComment, listComments and listReviews failures - approve.lgtm_acts_as_approve (/lgtm adds, /remove-lgtm withdraws) and approve.require_self_approval (the author's /approve ignored and the author excluded from the suggestion; the issue_comment refusal) plugins/approve.ts e2e lines go to 191/193; the two left are the exhaustive-switch default of effectiveKind and the `if (!hasOwners)` statement whose body is reached. Closes #332 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality --- .../bundle/approveEvaluationArms.test.ts | 306 ++++++++++++++++++ 1 file changed, 306 insertions(+) create mode 100644 __tests__/bundle/approveEvaluationArms.test.ts diff --git a/__tests__/bundle/approveEvaluationArms.test.ts b/__tests__/bundle/approveEvaluationArms.test.ts new file mode 100644 index 00000000..550b7643 --- /dev/null +++ b/__tests__/bundle/approveEvaluationArms.test.ts @@ -0,0 +1,306 @@ +import type { FakeGithub } from './fakeGithub' +import { Buffer } from 'node:buffer' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' +import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import pullReqReviewSubmittedEvent from '../fixtures/pullReq/pullReqReviewSubmittedEvent.json' +import { blobSha, prCommentEvent } from '../utils/ownersFixtures' +import { start } from './fakeGithub' +import { helpersFor, repo, token } from './helpers' +import { runBundle } from './runBundle' + +vi.setConfig({ testTimeout: 30_000 }) + +// the arms of src/plugins/approve.ts that bundle.test.ts' happy paths never reach — the handlers' early +// returns, evaluateApproval's read and write failures, the notifier's no-files / uncoverable-file / +// up-to-date renderings and the lgtm_acts_as_approve and require_self_approval settings — driven through +// dist/index.js against the fake api +describe('dist/index.js approve plugin arms', () => { + let gh: FakeGithub + const { routeOwners } = helpersFor(() => gh) + + const marker = '' + const reviewMarker = '' + const bot = { login: 'github-actions[bot]', type: 'Bot' } + const commentsRead = `GET ${repo}/issues/1/comments?per_page=100` + const reviewsRead = `GET ${repo}/pulls/1/reviews?per_page=100` + + const ownersFiles: Record = { + 'OWNERS': 'approvers:\n- alice\n', + 'sdk/OWNERS': 'approvers:\n- bob\n- dave\n', + } + + beforeAll(async () => { + gh = await start() + }) + beforeEach(() => gh.mergeQueueFallback({ pullRequestId: 'PR_none', headOid: pullReqOpenedEvent.pull_request.head.sha, enabled: false })) + afterEach(() => gh.reset()) + afterAll(() => gh.close()) + + function calls() { + return gh.requests.map(r => `${r.method} ${r.path}`) + } + + function prowConfig(text: string) { + const file = structuredClone(labelFileContents) + file.content = Buffer.from(text).toString('base64') + gh.route('GET', `${repo}/contents/.github%2Fprow.yaml`, { status: 200, body: file }) + } + + interface EvaluationOptions { + owners?: Record + author?: string + labels?: string[] + comments?: unknown[] + reviews?: unknown[] + } + + // a pull request on a base branch with OWNERS files, as approve reads and writes it; the tide gate that follows + // approve on the same event sees no lgtm and skips, so no merge is routed + function routeEvaluation(files: string[], options: EvaluationOptions = {}) { + const owners = options.owners ?? ownersFiles + gh.route('GET', `${repo}/git/trees/master`, { status: 200, body: { sha: 'master', truncated: false, tree: Object.keys(owners).map(path => ({ path, type: 'blob', sha: blobSha(path) })) } }) + routeOwners(owners, files, { user: { login: options.author ?? 'Codertocat' }, labels: (options.labels ?? []).map(name => ({ name })) }) + gh.route('GET', `${repo}/issues/1/comments`, { status: 200, body: options.comments ?? [] }) + gh.route('GET', `${repo}/pulls/1/reviews`, { status: 200, body: options.reviews ?? [] }) + gh.route('GET', `${repo}/labels`, { status: 200, body: [{ name: 'approved' }, { name: 'lgtm' }] }) + gh.route('POST', `${repo}/issues/1/labels`, { status: 200, body: [] }) + gh.route('DELETE', `${repo}/issues/1/labels/approved`, { status: 200, body: [] }) + gh.route('POST', `${repo}/issues/1/comments`, { status: 201, body: {} }) + gh.route('PATCH', `${repo}/issues/comments/900`, { status: 200, body: {} }) + } + + function runReview(payload: unknown = pullReqReviewSubmittedEvent) { + return runBundle({ eventName: 'pull_request_review', payload, inputs: token, apiUrl: gh.url }) + } + + function postedNotifier() { + const comments = gh.requestsMatching('POST', /\/issues\/1\/comments$/) + expect(comments).toHaveLength(1) + return (comments[0].body as { body: string }).body + } + + describe('handler early returns', () => { + it('pull_request closed: approve skips the action before any read of the pull request\'s comments', async () => { + routeOwners({}, ['sdk/x.go']) + + const result = await runBundle({ eventName: 'pull_request', payload: { ...pullReqOpenedEvent, action: 'closed' }, inputs: token, apiUrl: gh.url }) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: skipping closed action') + expect(calls()).not.toContain(commentsRead) + expect(gh.requestsMatching('POST', /\/issues\/1\/(labels|comments)$/)).toEqual([]) + }) + + it('pull_request_review submitted by the mirrored approval review itself evaluates nothing', async () => { + routeEvaluation(['sdk/x.go']) + const review = { ...pullReqReviewSubmittedEvent.review, id: 71, body: `Approved via /approve by bob (OWNERS).\n${reviewMarker}` } + + const result = await runReview({ ...pullReqReviewSubmittedEvent, review }) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: review 71 is the approval review this action mirrors; nothing to evaluate') + expect(calls()).not.toContain(commentsRead) + expect(calls()).not.toContain(reviewsRead) + expect(gh.requestsMatching('POST', /\/issues\/1\/(labels|comments)$/)).toEqual([]) + }) + + it('pull_request_review without a pull request in the payload fails the run naming the payload', async () => { + const { pull_request: _dropped, ...payload } = pullReqReviewSubmittedEvent + + const result = await runReview(payload) + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes('github context payload missing pull request'))).toBe(true) + expect(gh.requestsMatching('POST', /./)).toEqual([]) + }) + + it('the OWNERS files vanishing between the branch probe and the evaluation leaves the pull request alone', async () => { + // the probe sees OWNERS on the branch, the branch tip read for the evaluation finds an empty tree + gh.route('GET', `${repo}/git/trees/master`, { status: 200, body: { sha: 'master', truncated: false, tree: [{ path: 'OWNERS', type: 'blob', sha: blobSha('OWNERS') }] } }) + routeOwners({}, ['sdk/x.go']) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: the base of #1 has no OWNERS files, nothing to evaluate') + expect(calls()).not.toContain(commentsRead) + expect(gh.requestsMatching('POST', /\/issues\/1\/(labels|comments)$/)).toEqual([]) + expect(gh.requestsMatching('DELETE', /./)).toEqual([]) + }) + }) + + describe('the notifier', () => { + it('a pull request changing no files is not approved and says so, leaving the absent label alone', async () => { + routeEvaluation([]) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: #1 is not approved; nobody approves anything') + expect(result.stdout).toContain('approve: the approved label on #1 is already correct') + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) + expect(gh.requestsMatching('DELETE', /./)).toEqual([]) + expect(postedNotifier()).toBe([ + '[APPROVALNOTIFIER] This PR is **NOT APPROVED**', + '', + 'This pull request changes no files, so there is nothing to approve.', + marker, + ].join('\n')) + }) + + it('files no OWNERS file covers are listed after the OWNERS entries, sorted, with nobody to suggest', async () => { + // no root OWNERS: docs/ is covered by nothing, so no set of approvers can ever complete the approval + routeEvaluation(['docs/b.md', 'sdk/x.go', 'docs/a.md'], { + owners: { 'sdk/OWNERS': ownersFiles['sdk/OWNERS'] }, + comments: [{ id: 1, body: '/approve', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:01Z' }], + }) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: #1 is not approved; nobody approves docs/b.md, docs/a.md') + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) + const body = postedNotifier() + expect(body).toContain('This pull-request has been approved by: *bob*') + expect(body).not.toContain('please assign') + const sdk = `- ~~[sdk/OWNERS](https://github.com/Codertocat/Hello-World/blob/basesha/sdk/OWNERS)~~ [bob]` + expect(body).toContain(`${sdk}\n- **docs/a.md** (no OWNERS file covers this file)\n- **docs/b.md** (no OWNERS file covers this file)\n`) + }) + + it('a notifier already saying what the evaluation would say is neither re-posted nor edited', async () => { + const comments = [{ id: 1, body: '/approve', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:01Z' }] + routeEvaluation(['sdk/x.go'], { comments }) + const first = await runReview() + expect(first.status, first.stdout).toBe(0) + const body = postedNotifier() + expect(body).toContain('This PR is **APPROVED**') + gh.reset() + + // the same evaluation again, approved already on and the bot's notifier already carrying that body + routeEvaluation(['sdk/x.go'], { labels: ['approved'], comments: [{ id: 900, body, user: bot, created_at: '2024-01-01T00:00:00Z' }, ...comments] }) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: the notifier on #1 is up to date') + expect(result.stdout).toContain('approve: the approved label on #1 is already correct') + expect(gh.requestsMatching('POST', /\/issues\/1\/(labels|comments)$/)).toEqual([]) + expect(gh.requestsMatching('PATCH', /./)).toEqual([]) + }) + }) + + describe('read and write failures fail the run after the label is synced', () => { + // the first matching route wins in the fake, so each failure goes in before routeEvaluation's success + it('a refused notifier edit', async () => { + gh.route('PATCH', `${repo}/issues/comments/900`, { status: 500, body: { message: 'boom' } }) + routeEvaluation(['sdk/x.go'], { + comments: [ + { id: 900, body: `stale\n${marker}`, user: bot, created_at: '2024-01-01T00:00:00Z' }, + { id: 1, body: '/approve', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:01Z' }, + ], + }) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes('could not update the approval notifier: HttpError: boom'))).toBe(true) + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/).map(r => r.body)).toEqual([{ labels: ['approved'] }]) + expect(gh.requestsMatching('POST', /\/issues\/1\/comments$/)).toEqual([]) + }) + + it.each([ + ['comments', `${repo}/issues/1/comments`, 'could not list comments: HttpError: boom'], + ['reviews', `${repo}/pulls/1/reviews`, 'could not list reviews: HttpError: boom'], + ])('a failed %s read, before any write', async (_name, path, wantErr) => { + gh.route('GET', path, { status: 500, body: { message: 'boom' } }) + routeEvaluation(['sdk/x.go']) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes(wantErr))).toBe(true) + expect(gh.requestsMatching('POST', /\/issues\/1\/(labels|comments)$/)).toEqual([]) + expect(gh.requestsMatching('PATCH', /./)).toEqual([]) + }) + }) + + describe('approve settings', () => { + const lgtm = { id: 1, body: '/lgtm', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:01Z' } + const removeLgtm = { id: 2, body: '/remove-lgtm', user: { login: 'bob', type: 'User' }, created_at: '2024-01-01T00:00:02Z' } + + it('by default /lgtm from an approver is ignored: not approved, and an approver is suggested', async () => { + routeEvaluation(['sdk/x.go'], { comments: [lgtm] }) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: #1 is not approved; nobody approves sdk/x.go') + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) + // alice (root OWNERS) and bob each cover the one file; the tie goes alphabetically + expect(postedNotifier()).toContain('please assign **alice**') + }) + + it.each([ + ['/lgtm approves', [lgtm], true, 'approve: #1 is approved by bob'], + ['/remove-lgtm after it withdraws', [lgtm, removeLgtm], false, 'approve: #1 is not approved; nobody approves sdk/x.go'], + ])('with lgtm_acts_as_approve, %s', async (_name, comments, approved, wantInfo) => { + prowConfig('approve:\n lgtm_acts_as_approve: true\n') + routeEvaluation(['sdk/x.go'], { comments }) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain(wantInfo) + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/).map(r => r.body)).toEqual(approved ? [{ labels: ['approved'] }] : []) + expect(postedNotifier()).toContain(approved ? 'This PR is **APPROVED**' : 'This PR is **NOT APPROVED**') + }) + + it('with require_self_approval the author\'s /approve is ignored and another approver of the file is suggested', async () => { + prowConfig('approve:\n require_self_approval: true\n') + // alice would otherwise win the suggestion tie (see above); as the author she is excluded from it too + routeEvaluation(['sdk/x.go'], { + author: 'alice', + comments: [{ id: 1, body: '/approve', user: { login: 'alice', type: 'User' }, created_at: '2024-01-01T00:00:01Z' }], + }) + + const result = await runReview() + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout).toContain('approve: #1 is not approved; nobody approves sdk/x.go') + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) + const body = postedNotifier() + expect(body).toContain('This pull-request has been approved by:\n') + expect(body).toContain('please assign **bob**') + }) + + it('with require_self_approval an issue_comment /approve by the author is refused with a comment before any evaluation', async () => { + prowConfig('approve:\n require_self_approval: true\n') + routeEvaluation(['sdk/x.go'], { author: 'some-author' }) + + const result = await runBundle({ + eventName: 'issue_comment', + payload: prCommentEvent('/approve', 'some-author'), + inputs: { ...token, 'prow-commands': '/approve' }, + apiUrl: gh.url, + }) + + const wantErr = 'Cannot approve the pull request: you cannot approve your own PR (approve.require_self_approval is set).' + expect(result.status, result.stdout).toBe(1) + expect(result.errors).toContain(wantErr) + expect(postedNotifier()).toBe(wantErr) + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) + expect(calls()).not.toContain(commentsRead) + }) + }) +}) From 2ba9d37a7d986ff27cd549c7e0580b427fff98d3 Mon Sep 17 00:00:00 2001 From: quality Date: Wed, 7 Oct 2026 22:11:05 -0400 Subject: [PATCH 2/3] test(bundle): use the shared calls() helper, check an error fragment and drop the duplicated lgtm_acts_as_approve row Signed-off-by: quality --- .../bundle/approveEvaluationArms.test.ts | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/__tests__/bundle/approveEvaluationArms.test.ts b/__tests__/bundle/approveEvaluationArms.test.ts index 550b7643..1cd9450b 100644 --- a/__tests__/bundle/approveEvaluationArms.test.ts +++ b/__tests__/bundle/approveEvaluationArms.test.ts @@ -18,7 +18,7 @@ vi.setConfig({ testTimeout: 30_000 }) // dist/index.js against the fake api describe('dist/index.js approve plugin arms', () => { let gh: FakeGithub - const { routeOwners } = helpersFor(() => gh) + const { calls, routeOwners } = helpersFor(() => gh) const marker = '' const reviewMarker = '' @@ -38,10 +38,6 @@ describe('dist/index.js approve plugin arms', () => { afterEach(() => gh.reset()) afterAll(() => gh.close()) - function calls() { - return gh.requests.map(r => `${r.method} ${r.path}`) - } - function prowConfig(text: string) { const file = structuredClone(labelFileContents) file.content = Buffer.from(text).toString('base64') @@ -249,20 +245,17 @@ describe('dist/index.js approve plugin arms', () => { expect(postedNotifier()).toContain('please assign **alice**') }) - it.each([ - ['/lgtm approves', [lgtm], true, 'approve: #1 is approved by bob'], - ['/remove-lgtm after it withdraws', [lgtm, removeLgtm], false, 'approve: #1 is not approved; nobody approves sdk/x.go'], - ])('with lgtm_acts_as_approve, %s', async (_name, comments, approved, wantInfo) => { + it('with lgtm_acts_as_approve, /remove-lgtm after it withdraws', async () => { prowConfig('approve:\n lgtm_acts_as_approve: true\n') - routeEvaluation(['sdk/x.go'], { comments }) + routeEvaluation(['sdk/x.go'], { comments: [lgtm, removeLgtm] }) const result = await runReview() expect(result.status, result.stdout).toBe(0) expect(result.errors).toEqual([]) - expect(result.stdout).toContain(wantInfo) - expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/).map(r => r.body)).toEqual(approved ? [{ labels: ['approved'] }] : []) - expect(postedNotifier()).toContain(approved ? 'This PR is **APPROVED**' : 'This PR is **NOT APPROVED**') + expect(result.stdout).toContain('approve: #1 is not approved; nobody approves sdk/x.go') + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/).map(r => r.body)).toEqual([]) + expect(postedNotifier()).toContain('This PR is **NOT APPROVED**') }) it('with require_self_approval the author\'s /approve is ignored and another approver of the file is suggested', async () => { @@ -297,7 +290,7 @@ describe('dist/index.js approve plugin arms', () => { const wantErr = 'Cannot approve the pull request: you cannot approve your own PR (approve.require_self_approval is set).' expect(result.status, result.stdout).toBe(1) - expect(result.errors).toContain(wantErr) + expect(result.errors.some(e => e.includes('you cannot approve your own PR'))).toBe(true) expect(postedNotifier()).toBe(wantErr) expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) expect(calls()).not.toContain(commentsRead) From 12fd9158c0e2c7e525f34d7db11561087870cd0f Mon Sep 17 00:00:00 2001 From: quality Date: Thu, 8 Oct 2026 22:53:15 -0400 Subject: [PATCH 3/3] test(bundle): drop the no-files notifier case approveReviewShortCircuits already reaches Signed-off-by: quality --- .../bundle/approveEvaluationArms.test.ts | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/__tests__/bundle/approveEvaluationArms.test.ts b/__tests__/bundle/approveEvaluationArms.test.ts index 1cd9450b..4401d164 100644 --- a/__tests__/bundle/approveEvaluationArms.test.ts +++ b/__tests__/bundle/approveEvaluationArms.test.ts @@ -131,25 +131,6 @@ describe('dist/index.js approve plugin arms', () => { }) describe('the notifier', () => { - it('a pull request changing no files is not approved and says so, leaving the absent label alone', async () => { - routeEvaluation([]) - - const result = await runReview() - - expect(result.status, result.stdout).toBe(0) - expect(result.errors).toEqual([]) - expect(result.stdout).toContain('approve: #1 is not approved; nobody approves anything') - expect(result.stdout).toContain('approve: the approved label on #1 is already correct') - expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) - expect(gh.requestsMatching('DELETE', /./)).toEqual([]) - expect(postedNotifier()).toBe([ - '[APPROVALNOTIFIER] This PR is **NOT APPROVED**', - '', - 'This pull request changes no files, so there is nothing to approve.', - marker, - ].join('\n')) - }) - it('files no OWNERS file covers are listed after the OWNERS entries, sorted, with nobody to suggest', async () => { // no root OWNERS: docs/ is covered by nothing, so no set of approvers can ever complete the approval routeEvaluation(['docs/b.md', 'sdk/x.go', 'docs/a.md'], {