diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 5e41eb8b..dd8fef6c 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -43,6 +43,7 @@ export const PR_CHECK_REASON_CODES = Object.freeze({ MISSING_FULL_JENKINS_CI: 'missing-full-jenkins-ci', MISSING_GITHUB_CI: 'missing-github-ci', MISSING_JENKINS_CI: 'missing-jenkins-ci', + MISSING_LARGE_PR_TSC_APPROVAL: 'missing-large-pr-tsc-approval', MISSING_TSC_APPROVAL: 'missing-tsc-approval', NEW_CONTRIBUTOR: 'new-contributor', NO_COMMITS: 'no-commits', @@ -196,6 +197,10 @@ export default class PRChecker { let isFastTracked = labels.includes('fast-track'); const isSemverMajor = labels.includes('semver-major'); + // Whether a pull request is large is a judgement the project records + // with a label, rather than something derived from the diff. + // https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md + const isLargePR = labels.includes('large-pr'); // NOTE: a semver-major PR with fast-track should have either one of // these labels removed because that doesn't make sense if (isFastTracked) { @@ -206,10 +211,11 @@ export default class PRChecker { return false; } + const tscApproved = approved + .filter((p) => p.reviewer.isTSC()) + .map((p) => p.reviewer.login); + if (isSemverMajor) { - const tscApproved = approved - .filter((p) => p.reviewer.isTSC()) - .map((p) => p.reviewer.login); if (tscApproved.length < 2) { const message = 'semver-major requires at least 2 TSC approvals'; cli.error(message); @@ -221,6 +227,23 @@ export default class PRChecker { } } + if (isLargePR) { + if (tscApproved.length < 2) { + const message = + 'large pull requests require at least 2 TSC approvals'; + cli.error(message); + this.addReason( + PR_CHECK_REASON_CODES.MISSING_LARGE_PR_TSC_APPROVAL, + message, + { + approvals: tscApproved.length, + required: 2 + } + ); + return false; // 7 day rule doesn't matter here + } + } + let fastTrackAppendix = ''; if (isFastTracked) { const comment = [...this.comments].reverse().find((c) => diff --git a/test/fixtures/data.js b/test/fixtures/data.js index c05b5d05..5bbc17cd 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -86,6 +86,7 @@ export const firstTimerPR = readJSON('first_timer_pr.json'); export const firstTimerPrivatePR = readJSON('first_timer_pr_with_private_email.json'); export const semverMajorPR = readJSON('semver_major_pr.json'); +export const largePR = readJSON('large_pr.json'); export const fixAndRefPR = readJSON('pr_with_fixes_and_refs.json'); export const fixCrossPR = readJSON('pr_with_fixes_cross.json'); export const duplicateRefPR = readJSON('pr_with_duplicate_refs.json'); diff --git a/test/fixtures/large_pr.json b/test/fixtures/large_pr.json new file mode 100644 index 00000000..3457d892 --- /dev/null +++ b/test/fixtures/large_pr.json @@ -0,0 +1,22 @@ +{ + "createdAt": "2017-10-24T11:13:43Z", + "authorAssociation": "COLLABORATOR", + "author": { + "login": "pr_author", + "email": "pr_author@example.com", + "name": "Their Github Account email" + }, + "url": "https://github.com/nodejs/node/pull/16438", + "bodyHTML": "

Awesome changes

", + "bodyText": "Awesome changes", + "labels": { + "nodes": [ + { + "name": "large-pr" + } + ] + }, + "title": "lib: awesome changes", + "baseRefName": "main", + "headRefName": "awesome-changes" +} diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index a64c4855..e601cbef 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -40,6 +40,7 @@ import { firstTimerPR, firstTimerPrivatePR, semverMajorPR, + largePR, conflictingPR, closedPR, mergedPR, @@ -61,6 +62,7 @@ const { MISSING_APPROVAL, MISSING_GITHUB_CI, MISSING_JENKINS_CI, + MISSING_LARGE_PR_TSC_APPROVAL, MISSING_TSC_APPROVAL, REQUESTED_CHANGES, STALE_REVIEW, @@ -176,6 +178,55 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should error when large PR has only 1 TSC approval', () => { + const cli = new TestCLI(); + + const expectedLogs = { + error: [ + ['large pull requests require at least 2 TSC approvals'] + ], + ok: [ + ['Approvals: 4'], + ['- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'], + ['- Quux User (@Quux): LGTM'], + ['- Baz User (@Baz): https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236'], + ['- Bar User (@bar) (TSC): lgtm'] + ], + info: [ + ['This PR was created on Fri, 23 Nov 2018 17:50:44 GMT'], + ['- Quux User (@Quux) approved in via LGTM in comments'], + ['- Bar User (@bar) approved in via LGTM in comments'] + ] + }; + const pr = Object.assign({}, largePR, { + createdAt: GT_7D + }); + + const data = { + pr, + reviewers: allGreenReviewers, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: simpleCommits, + collaborators, + authorIsNew: () => false, + getThread() { + return PRData.prototype.getThread.call(this); + } + }; + const checker = new PRChecker(cli, data, {}, argv); + + const status = checker.checkReviewsAndWait(new Date(NOW), true); + assert(!status); + assert.deepStrictEqual(checker.reasons, [{ + code: MISSING_LARGE_PR_TSC_APPROVAL, + message: 'large pull requests require at least 2 TSC approvals', + approvals: 1, + required: 2 + }]); + cli.assertCalledWith(expectedLogs); + }); + it('should error when PR has change requests', () => { const cli = new TestCLI();