diff --git a/lib/collaborators.js b/lib/collaborators.js index 580c43ca..2019cfc0 100644 --- a/lib/collaborators.js +++ b/lib/collaborators.js @@ -31,7 +31,7 @@ class Collaborator { } getName() { - return `${this.name}(${this.login})`; + return `${this.name} (@${this.login})`; } getContact() { diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 4ccd8fab..f079327b 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -4,7 +4,8 @@ const SECOND = 1000; const MINUTE = SECOND * 60; const HOUR = MINUTE * 60; -const WAIT_TIME = 48; +const WAIT_TIME_MULTI_APPROVAL = 24 * 2; +const WAIT_TIME_SINGLE_APPROVAL = 24 * 7; const { REVIEW_SOURCES: { FROM_COMMENT, FROM_REVIEW_COMMENT } @@ -48,12 +49,11 @@ class PRChecker { ); } - checkAll(comments = false) { + checkAll(checkComments = false) { const status = [ - this.checkReviews(comments), this.checkCommitsAfterReview(), this.checkCI(), - this.checkPRWait(new Date()), + this.checkReviewsAndWait(new Date(), checkComments), this.checkMergeableState(), this.checkPRState(), this.checkGitConfig() @@ -69,117 +69,104 @@ class PRChecker { return status.every((i) => i); } - getTSCHint(people) { - const tsc = people + getTSC(people) { + return people .filter((p) => p.reviewer.isTSC()) .map((p) => p.reviewer.login); + } + + formatReview(reviewer, review) { let hint = ''; - if (tsc.length > 0) { - const list = `(${tsc.join(', ')})`; - hint = `, ${tsc.length} from TSC ${list}`; + if (reviewer.isTSC()) { + hint = ' (TSC)'; } - return hint; + return `- ${reviewer.getName()}${hint}: ${review.ref}`; } - checkReviews(comments = false) { - const { - pr, cli, reviewers: { requestedChanges, approved } - } = this; - let status = true; - - if (requestedChanges.length === 0) { - cli.ok(`Requested Changes: 0`); - } else { - status = false; - let hint = this.getTSCHint(requestedChanges); - cli.error(`Requested Changes: ${requestedChanges.length}${hint}`); + displayReviews(checkComments) { + const { cli, reviewers: { requestedChanges, approved } } = this; + if (requestedChanges.length > 0) { + cli.error(`Requested Changes: ${requestedChanges.length}`); for (const { reviewer, review } of requestedChanges) { - cli.error(`- ${reviewer.getName()}: ${review.ref}`); + cli.error(this.formatReview(reviewer, review)); } } + if (approved.length === 0) { - status = false; cli.error(`Approvals: 0`); - } else { - let hint = this.getTSCHint(approved); - cli.ok(`Approvals: ${approved.length}${hint}`); - - if (comments) { - for (const { reviewer, review } of approved) { - if (review.source === FROM_COMMENT || - review.source === FROM_REVIEW_COMMENT) { - cli.info( - `- ${reviewer.getName()} approved in via LGTM in comments`); - } - } - } + return; + } - const labels = pr.labels.nodes.map((l) => l.name); - if (labels.includes('semver-major')) { - const tscApproval = approved.filter((p) => p.reviewer.isTSC()).length; - if (tscApproval < 2) { - status = false; - cli.error('semver-major requires at least two TSC approvals'); - } + cli.ok(`Approvals: ${approved.length}`); + for (const { reviewer, review } of approved) { + cli.ok(this.formatReview(reviewer, review)); + if (checkComments && + [FROM_COMMENT, FROM_REVIEW_COMMENT].includes(review.source)) { + cli.info(`- ${reviewer.getName()} approved in via LGTM in comments`); } } - - return status; } - /** - * @param {Date} now - */ - getWait(now) { + checkReviewsAndWait(now, checkComments) { + const { + pr, cli, reviewers + } = this; + const { requestedChanges, approved } = reviewers; + const labels = pr.labels.nodes.map((l) => l.name); + + const isFastTracked = labels.includes('fast-track'); + const isSemverMajor = labels.includes('semver-major'); + + const dateStr = new Date(pr.createdAt).toUTCString(); + cli.info(`This PR was created on ${dateStr}`); + this.displayReviews(checkComments); + // NOTE: a semver-major PR with fast-track should have either one of + // these labels removed because that doesn't make sense + if (isFastTracked) { + cli.info('This PR is being fast-tracked'); + } + + if (approved.length === 0 || requestedChanges.length > 0) { + return false; + } + + if (isSemverMajor) { + const tscApproved = approved + .filter((p) => p.reviewer.isTSC()) + .map((p) => p.reviewer.login); + if (tscApproved.length < 2) { + cli.error('semver-major requires at least 2 TSC approvals'); + return false; // 7 day rule doesn't matter here + } + } + const createTime = new Date(this.pr.createdAt); - const timeLeft = WAIT_TIME - Math.ceil( + const hoursFromCreateTime = Math.ceil( (now.getTime() - createTime.getTime()) / HOUR ); + let timeLeftMulti = WAIT_TIME_MULTI_APPROVAL - hoursFromCreateTime; + const timeLeftSingle = WAIT_TIME_SINGLE_APPROVAL - hoursFromCreateTime; - return { - timeLeft - }; - } - - // TODO: skip some PRs...we might need a label for that - /** - * @param {Date} now - */ - checkPRWait(now) { - const { - pr, cli, reviewers, CIStatus - } = this; - const labels = pr.labels.nodes; - - const fast = - labels.some((l) => ['fast-track'].includes(l.name)); - if (fast) { - const { approved } = reviewers; - if (approved.length > 1 && CIStatus) { - cli.info('This PR is being fast-tracked'); + if (approved.length >= 2) { + if (isFastTracked) { return true; - } else { - const msg = ['This PR is being fast-tracked, but awaiting ']; - if (approved.length < 2) msg.push('approvals of 2 contributors'); - if (!CIStatus) msg.push('a CI run'); - - let warnMsg = msg.length === 2 - ? msg.join('') : `${msg[0] + msg[1]} and ${msg[2]}`; - cli.warn(warnMsg); } - + if (timeLeftMulti < 0) { + return true; + } + cli.error(`This PR needs to wait ${timeLeftMulti} more hours to land`); return false; } - const wait = this.getWait(now); - if (wait.timeLeft > 0) { - const dateStr = new Date(pr.createdAt).toDateString(); - cli.info(`This PR was created on ${dateStr}`); - cli.warn(`Wait at least ${wait.timeLeft} more hours before landing`); + if (approved.length === 1) { + if (timeLeftSingle < 0) { + return true; + } + timeLeftMulti = timeLeftMulti < 0 ? 0 : timeLeftMulti; + cli.error(`This PR needs to wait ${timeLeftSingle} more hours to land ` + + `(or ${timeLeftMulti} hours if there is one more approval)`); return false; } - - return true; } hasFullOrLiteCI(ciMap) { diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 37535453..6795d0b1 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -15,6 +15,10 @@ const allGreenReviewers = { approved, requestedChanges: [] }; +const singleGreenReviewer = { + approved: [approved[0]], + requestedChanges: [] +}; const requestedChangesReviewers = { requestedChanges, approved: [] @@ -76,6 +80,7 @@ module.exports = { approved, requestedChanges, allGreenReviewers, + singleGreenReviewer, requestedChangesReviewers, approvingReviews, requestingChangesReviews, diff --git a/test/unit/collaborators.test.js b/test/unit/collaborators.test.js index 166fc98c..f58f7078 100644 --- a/test/unit/collaborators.test.js +++ b/test/unit/collaborators.test.js @@ -62,7 +62,7 @@ describe('collaborators', function() { collaborators.forEach(collaborator => { assert.strictEqual( collaborator.getName(), - `${collaborator.name}(${collaborator.login})`); + `${collaborator.name} (@${collaborator.login})`); }); }); }); diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index b35ac2e8..7e82f059 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -8,8 +8,14 @@ const TestCLI = require('../fixtures/test_cli'); const PRData = require('../../lib/pr_data'); const PRChecker = require('../../lib/pr_checker'); +const GT_7D = '2018-11-23T17:50:44.477Z'; +const LT_7D_GT_48H = '2018-11-27T17:50:44.477Z'; +const LT_48H = '2018-11-30T17:50:44.477Z'; +const NOW = '2018-11-31T17:50:44.477Z'; + const { allGreenReviewers, + singleGreenReviewer, requestedChangesReviewers, approvingReviews, requestingChangesReviews, @@ -52,8 +58,7 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - let checkReviewsStub; - let checkPRWaitStub; + let checkReviewsAndWaitStub; let checkCIStub; let checkAuthorStub; let checkCommitsAfterReviewStub; @@ -62,8 +67,7 @@ describe('PRChecker', () => { let checkGitConfigStub; before(() => { - checkReviewsStub = sinon.stub(checker, 'checkReviews'); - checkPRWaitStub = sinon.stub(checker, 'checkPRWait'); + checkReviewsAndWaitStub = sinon.stub(checker, 'checkReviewsAndWait'); checkCIStub = sinon.stub(checker, 'checkCI'); checkAuthorStub = sinon.stub(checker, 'checkAuthor'); checkCommitsAfterReviewStub = @@ -74,8 +78,7 @@ describe('PRChecker', () => { }); after(() => { - checkReviewsStub.restore(); - checkPRWaitStub.restore(); + checkReviewsAndWaitStub.restore(); checkCIStub.restore(); checkAuthorStub.restore(); checkCommitsAfterReviewStub.restore(); @@ -87,8 +90,7 @@ describe('PRChecker', () => { it('should run necessary checks', () => { const status = checker.checkAll(); assert.strictEqual(status, false); - assert.strictEqual(checkReviewsStub.calledOnce, true); - assert.strictEqual(checkPRWaitStub.calledOnce, true); + assert.strictEqual(checkReviewsAndWaitStub.calledOnce, true); assert.strictEqual(checkCIStub.calledOnce, true); assert.strictEqual(checkAuthorStub.calledOnce, true); assert.strictEqual(checkCommitsAfterReviewStub.calledOnce, true); @@ -98,26 +100,33 @@ describe('PRChecker', () => { }); }); - describe('checkReviews', () => { - it('should warn about semver-major PR without enough TSC approvals', () => { + describe('checkReviewsAndWait', () => { + it('should error when semver-major PR has only 1 TSC approval', () => { const cli = new TestCLI(); const expectedLogs = { error: [ - ['semver-major requires at least two TSC approvals'] + ['semver-major requires at least 2 TSC approvals'] ], ok: [ - ['Requested Changes: 0'], - ['Approvals: 4, 1 from TSC (bar)'] + ['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: [ - ['- Quux User(Quux) approved in via LGTM in comments'], - ['- Bar User(bar) approved in via LGTM in comments'] + ['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({}, semverMajorPR, { + createdAt: GT_7D + }); const data = { - pr: semverMajorPR, + pr, reviewers: allGreenReviewers, comments: commentsWithLGTM, reviews: approvingReviews, @@ -130,25 +139,32 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - const status = checker.checkReviews(true); + const status = checker.checkReviewsAndWait(new Date(NOW), true); assert(!status); cli.assertCalledWith(expectedLogs); }); - it('should warn about PR with rejections & without approvals', () => { + it('should error when PR has change requests', () => { const cli = new TestCLI(); const expectedLogs = { error: [ - ['Requested Changes: 2, 1 from TSC (bar)'], - ['- Foo User(foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'], - ['- Bar User(bar): https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624'], + ['Requested Changes: 2'], + ['- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'], + ['- Bar User (@bar) (TSC): https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624'], ['Approvals: 0'] + ], + info: [ + ['This PR was created on Fri, 23 Nov 2018 17:50:44 GMT'] ] }; + const pr = Object.assign({}, firstTimerPR, { + createdAt: GT_7D + }); + const data = { - pr: firstTimerPR, + pr, reviewers: requestedChangesReviewers, comments: [], reviews: requestingChangesReviews, @@ -158,24 +174,27 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - const status = checker.checkReviews(); + const status = checker.checkReviewsAndWait(new Date(NOW)); assert(!status); cli.assertCalledWith(expectedLogs); }); - }); - describe('checkPRWait', () => { - it('should warn about PR younger than 48h', () => { + it('should error when PR is younger than 48h', () => { const cli = new TestCLI(); const expectedLogs = { - warn: [['Wait at least 22 more hours before landing']], - info: [['This PR was created on Tue Oct 31 2017']] + 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, 30 Nov 2018 17:50:44 GMT' ] ], + error: [ [ 'This PR needs to wait 24 more hours to land' ] ] }; - const now = new Date('2017-11-01T14:25:41.682Z'); const youngPR = Object.assign({}, firstTimerPR, { - createdAt: '2017-10-31T13:00:41.682Z' + createdAt: LT_48H }); const data = { @@ -192,36 +211,70 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - const status = checker.checkPRWait(now); + const status = checker.checkReviewsAndWait(new Date(NOW)); assert(!status); cli.assertCalledWith(expectedLogs); }); - it('should log as expected if PR can be fast-tracked', () => { + it('should error when PR has only 1 approval < 48h', () => { const cli = new TestCLI(); const expectedLogs = { - info: [ - [ 'This PR is being fast-tracked' ] - ] + ok: + [ [ 'Approvals: 1' ], + [ '- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624' ] ], + info: + [ [ 'This PR was created on Fri, 30 Nov 2018 17:50:44 GMT' ] ], + error: [ [ 'This PR needs to wait 144 more hours to land ' + + '(or 24 hours if there is one more approval)' ] ] }; - const now = new Date('2017-11-01T14:25:41.682Z'); - const PR = Object.assign({}, firstTimerPR, { - createdAt: '2017-10-31T13:00:41.682Z', - labels: { - nodes: [ - { name: 'fast-track' } - ] + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: LT_48H + }); + + const data = { + pr: youngPR, + reviewers: singleGreenReviewer, + comments: commentsWithLGTM, + reviews: approvingReviews, + commits: simpleCommits, + collaborators, + authorIsNew: () => true, + getThread() { + return PRData.prototype.getThread.call(this); } + }; + const checker = new PRChecker(cli, data, argv); + + const status = checker.checkReviewsAndWait(new Date(NOW)); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should error when PR has only 1 approval >48h', () => { + const cli = new TestCLI(); + + const expectedLogs = { + ok: + [ [ 'Approvals: 1' ], + [ '- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624' ] ], + info: + [ [ 'This PR was created on Tue, 27 Nov 2018 17:50:44 GMT' ] ], + error: [ [ 'This PR needs to wait 72 more hours to land ' + + '(or 0 hours if there is one more approval)' ] ] + }; + + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: LT_7D_GT_48H }); const data = { - pr: PR, - reviewers: allGreenReviewers, - comments: commentsWithCI, + pr: youngPR, + reviewers: singleGreenReviewer, + comments: commentsWithLGTM, reviews: approvingReviews, - commits: [], + commits: simpleCommits, collaborators, authorIsNew: () => true, getThread() { @@ -230,26 +283,28 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - checker.checkCI(); - cli.clearCalls(); - const status = checker.checkPRWait(now); - assert(status); + const status = checker.checkReviewsAndWait(new Date(NOW)); + assert(!status); cli.assertCalledWith(expectedLogs); }); - it('should warn about approvals and CI for fast-tracked PR', () => { + it('should log as expected if PR can be fast-tracked', () => { const cli = new TestCLI(); const expectedLogs = { - warn: [ - [ 'This PR is being fast-tracked, but awaiting ' + - 'approvals of 2 contributors and a CI run' ] - ] + 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, 30 Nov 2018 17:50:44 GMT' ], + [ 'This PR is being fast-tracked' ] ] }; - const now = new Date('2017-11-01T14:25:41.682Z'); - const PR = Object.assign({}, firstTimerPR, { - createdAt: '2017-10-31T13:00:41.682Z', + const pr = Object.assign({}, firstTimerPR, { + createdAt: LT_48H, labels: { nodes: [ { name: 'fast-track' } @@ -258,11 +313,11 @@ describe('PRChecker', () => { }); const data = { - pr: PR, - reviewers: requestedChangesReviewers, - comments: [], - reviews: requestingChangesReviews, - commits: simpleCommits, + pr, + reviewers: allGreenReviewers, + comments: commentsWithCI, + reviews: approvingReviews, + commits: [], collaborators, authorIsNew: () => true, getThread() { @@ -271,26 +326,28 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - checker.checkCI(); cli.clearCalls(); - const status = checker.checkPRWait(now); - assert(!status); + const status = checker.checkReviewsAndWait(new Date(NOW)); + assert(status); cli.assertCalledWith(expectedLogs); }); - it('should warn cannot be fast-tracked because of approvals', () => { + it('should warn about approvals and CI for fast-tracked PR', () => { const cli = new TestCLI(); const expectedLogs = { - warn: [ - [ 'This PR is being fast-tracked, but awaiting ' + - 'approvals of 2 contributors' ] - ] + info: + [ ['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'], + ['This PR is being fast-tracked'] ], + error: + [ [ 'Requested Changes: 2' ], + [ '- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624' ], + [ '- Bar User (@bar) (TSC): https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624' ], + [ 'Approvals: 0' ] ] }; - const now = new Date('2017-11-01T14:25:41.682Z'); - const PR = Object.assign({}, firstTimerPR, { - createdAt: '2017-10-31T13:00:41.682Z', + const pr = Object.assign({}, firstTimerPR, { + createdAt: LT_48H, labels: { nodes: [ { name: 'fast-track' } @@ -299,11 +356,11 @@ describe('PRChecker', () => { }); const data = { - pr: PR, + pr, reviewers: requestedChangesReviewers, - comments: commentsWithCI, - reviews: approvingReviews, - commits: [], + comments: [], + reviews: requestingChangesReviews, + commits: simpleCommits, collaborators, authorIsNew: () => true, getThread() { @@ -312,25 +369,28 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - checker.checkCI(); cli.clearCalls(); - const status = checker.checkPRWait(now); + const status = checker.checkReviewsAndWait(new Date(NOW)); assert(!status); cli.assertCalledWith(expectedLogs); }); - it('should warn if the PR has no CI and cannot be fast-tracked', () => { + it('should warn cannot be fast-tracked because of approvals', () => { const cli = new TestCLI(); const expectedLogs = { - warn: [ - [ 'This PR is being fast-tracked, but awaiting a CI run' ] - ] + info: + [ [ 'This PR was created on Fri, 30 Nov 2018 17:50:44 GMT' ], + [ 'This PR is being fast-tracked' ] ], + error: + [ [ 'Requested Changes: 2' ], + [ '- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624' ], + [ '- Bar User (@bar) (TSC): https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624' ], + [ 'Approvals: 0' ] ] }; - const now = new Date('2017-11-01T14:25:41.682Z'); const PR = Object.assign({}, firstTimerPR, { - createdAt: '2017-10-31T13:00:41.682Z', + createdAt: LT_48H, labels: { nodes: [ { name: 'fast-track' } @@ -340,10 +400,10 @@ describe('PRChecker', () => { const data = { pr: PR, - reviewers: allGreenReviewers, - comments: [], + reviewers: requestedChangesReviewers, + comments: commentsWithCI, reviews: approvingReviews, - commits: simpleCommits, + commits: [], collaborators, authorIsNew: () => true, getThread() { @@ -352,16 +412,15 @@ describe('PRChecker', () => { }; const checker = new PRChecker(cli, data, argv); - checker.checkCI(); cli.clearCalls(); - const status = checker.checkPRWait(now); + const status = checker.checkReviewsAndWait(new Date(NOW)); assert(!status); cli.assertCalledWith(expectedLogs); }); }); describe('checkCI', () => { - it('should warn if no CI runs detected', () => { + it('should error if no CI runs detected', () => { const cli = new TestCLI(); const expectedLogs = {