diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 5e41eb8b..c743ddfe 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -26,6 +26,14 @@ const WAIT_TIME_SINGLE_APPROVAL = 24 * 7; const GITHUB_SUCCESS_CONCLUSIONS = ['SUCCESS', 'NEUTRAL', 'SKIPPED']; const GITHUB_ACTIONS_APP = 'github-actions'; +// https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md +const LARGE_PR_LINE_THRESHOLD = 5000; +const LARGE_PR_REQUIRED_TSC_APPROVALS = 2; +// Labels marking pull requests the large pull request policy does not apply +// to. Routine dependency updates are excluded by the policy, and release +// proposals are machine-generated and follow the release process instead. +const LARGE_PR_EXEMPT_LABELS = ['dependencies', 'release']; + 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 +51,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', @@ -139,6 +148,31 @@ export default class PRChecker { .map((p) => p.reviewer.login); } + // Net change in lines, as the large pull request policy counts it. Returns + // undefined when the data is not available, so callers can tell "not large" + // apart from "cannot tell". + getNetLineChange() { + const { additions, deletions } = this.pr; + + if (typeof additions !== 'number' || typeof deletions !== 'number') { + return undefined; + } + + return additions - deletions; + } + + // Whether the pull request falls into one of the categories the large pull + // request policy exempts. Only the categories that can be told apart from + // the pull request data alone are covered here: the remaining ones (routine + // dependency updates, WPT imports, bot-issued pull requests, test-only + // refactors) need a judgement this cannot make, which is why a suspected + // large pull request is reported rather than blocked outright. + isExemptFromLargePRPolicy() { + const labels = this.pr.labels?.nodes?.map((l) => l.name) ?? []; + + return LARGE_PR_EXEMPT_LABELS.some((label) => labels.includes(label)); + } + formatReview(reviewer, review) { let hint = ''; if (reviewer.isTSC()) { @@ -221,6 +255,38 @@ export default class PRChecker { } } + const netLineChange = this.getNetLineChange(); + const isLargePR = netLineChange > LARGE_PR_LINE_THRESHOLD; + + // A semver-major pull request has already been held to the same approval + // requirement above, so only check the ones that have not been. + if (isLargePR && !isSemverMajor) { + const tscApproved = this.getTSC(approved); + + if (this.isExemptFromLargePRPolicy()) { + cli.info( + `This PR changes ${netLineChange} lines, but is exempt from the ` + + 'large pull request policy' + ); + } else if (tscApproved.length < LARGE_PR_REQUIRED_TSC_APPROVALS) { + const message = 'large pull requests require at least ' + + `${LARGE_PR_REQUIRED_TSC_APPROVALS} TSC approvals`; + cli.error(message); + cli.info( + `This PR changes ${netLineChange} lines, over the ` + + `${LARGE_PR_LINE_THRESHOLD} line threshold. See ` + + 'https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md' + ); + this.addReason( + PR_CHECK_REASON_CODES.MISSING_LARGE_PR_TSC_APPROVAL, message, { + approvals: tscApproved.length, + required: LARGE_PR_REQUIRED_TSC_APPROVALS, + lines: netLineChange + }); + return false; + } + } + let fastTrackAppendix = ''; if (isFastTracked) { const comment = [...this.comments].reverse().find((c) => diff --git a/lib/queries/PR.gql b/lib/queries/PR.gql index 10400af4..ee52f593 100644 --- a/lib/queries/PR.gql +++ b/lib/queries/PR.gql @@ -57,6 +57,8 @@ query PR($prid: Int!, $owner: String!, $repo: String!) { baseRefName, headRefName, changedFiles, + additions, + deletions, mergeable, closed, closedAt, diff --git a/test/fixtures/data.js b/test/fixtures/data.js index c05b5d05..62bf5766 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -86,6 +86,8 @@ 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 largeExemptPR = readJSON('large_pr_exempt.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..4205a842 --- /dev/null +++ b/test/fixtures/large_pr.json @@ -0,0 +1,20 @@ +{ + "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": [] + }, + "title": "lib: awesome changes", + "baseRefName": "main", + "headRefName": "awesome-changes", + "additions": 6000, + "deletions": 500 +} diff --git a/test/fixtures/large_pr_exempt.json b/test/fixtures/large_pr_exempt.json new file mode 100644 index 00000000..8e78d0d5 --- /dev/null +++ b/test/fixtures/large_pr_exempt.json @@ -0,0 +1,24 @@ +{ + "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": "dependencies" + } + ] + }, + "title": "deps: update a dependency", + "baseRefName": "main", + "headRefName": "awesome-changes", + "additions": 6000, + "deletions": 500 +} diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index a64c4855..87753ec6 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -40,6 +40,8 @@ import { firstTimerPR, firstTimerPrivatePR, semverMajorPR, + largePR, + largeExemptPR, conflictingPR, closedPR, mergedPR, @@ -61,6 +63,7 @@ const { MISSING_APPROVAL, MISSING_GITHUB_CI, MISSING_JENKINS_CI, + MISSING_LARGE_PR_TSC_APPROVAL, MISSING_TSC_APPROVAL, REQUESTED_CHANGES, STALE_REVIEW, @@ -176,6 +179,132 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should error when a large PR has fewer than 2 TSC approvals', () => { + 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'], + ['This PR changes 5500 lines, over the 5000 line threshold. See ' + + 'https://github.com/nodejs/node/blob/main/doc/contributing/large-pull-requests.md'] + ] + }; + 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, + lines: 5500 + }]); + cli.assertCalledWith(expectedLogs); + }); + + it('should not require TSC approvals for an exempt large PR', () => { + const cli = new TestCLI(); + + const expectedLogs = { + 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'], + ['This PR changes 5500 lines, but is exempt from the large pull ' + + 'request policy'] + ] + }; + const pr = Object.assign({}, largeExemptPR, { + 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, []); + cli.assertCalledWith(expectedLogs); + }); + + it('should not apply the large PR check when the line counts are absent', + () => { + const cli = new TestCLI(); + + // Older fixtures, and any PR queried before `additions`/`deletions` + // were added to the query, carry no line counts. + const pr = Object.assign({}, largePR, { + createdAt: GT_7D, + additions: undefined, + deletions: undefined + }); + + 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, []); + }); + it('should error when PR has change requests', () => { const cli = new TestCLI();