diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 5e41eb8b..ef8fe461 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -26,6 +26,10 @@ const WAIT_TIME_SINGLE_APPROVAL = 24 * 7; const GITHUB_SUCCESS_CONCLUSIONS = ['SUCCESS', 'NEUTRAL', 'SKIPPED']; const GITHUB_ACTIONS_APP = 'github-actions'; +// Both semver-major and large pull requests take this approval path. +// https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md +const REQUIRED_TSC_APPROVALS = 2; + const FAST_TRACK_RE = /^Fast-track has been requested by @(.+?)\. Please 👍 to approve\.$/; const FAST_TRACK_MIN_APPROVALS = 2; const GIT_CONFIG_GUIDE_URL = 'https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork'; @@ -43,6 +47,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 +201,10 @@ export default class PRChecker { let isFastTracked = labels.includes('fast-track'); const isSemverMajor = labels.includes('semver-major'); + // Large pull requests follow the same approval path as semver-major + // changes. Whether a pull request is large is a judgement the project + // records with a label, rather than something derived from the diff. + 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,16 +215,26 @@ export default class PRChecker { return false; } - if (isSemverMajor) { + if (isSemverMajor || isLargePR) { 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'; + if (tscApproved.length < REQUIRED_TSC_APPROVALS) { + // A pull request can carry both labels; report it as semver-major, + // since that is the stronger statement about the change. + const code = isSemverMajor + ? PR_CHECK_REASON_CODES.MISSING_TSC_APPROVAL + : PR_CHECK_REASON_CODES.MISSING_LARGE_PR_TSC_APPROVAL; + const subject = isSemverMajor + ? 'semver-major requires' + : 'large pull requests require'; + const message = + `${subject} at least ${REQUIRED_TSC_APPROVALS} ` + + 'TSC approvals'; cli.error(message); - this.addReason(PR_CHECK_REASON_CODES.MISSING_TSC_APPROVAL, message, { + this.addReason(code, message, { approvals: tscApproved.length, - required: 2 + required: REQUIRED_TSC_APPROVALS }); return false; // 7 day rule doesn't matter here } 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();