From 868a83ad3cb48c1a283e2f33fa12807009d21d0a Mon Sep 17 00:00:00 2001 From: Shovon Date: Sun, 7 Oct 2018 09:06:51 -0700 Subject: [PATCH] git-node: wait time for PR with single approval One Collaborator approval is enough only if the pull request has been open for more than 7 days Refs: https://github.com/nodejs/node/pull/22255 --- lib/pr_checker.js | 28 ++++++++++--- test/fixtures/data.js | 5 +++ test/unit/pr_checker.test.js | 77 +++++++++++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 7 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 4ccd8fa..6260e44 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -5,6 +5,7 @@ const MINUTE = SECOND * 60; const HOUR = MINUTE * 60; const WAIT_TIME = 48; +const WAIT_TIME_SINGLE_APPROVAL = 168; const { REVIEW_SOURCES: { FROM_COMMENT, FROM_REVIEW_COMMENT } @@ -132,12 +133,16 @@ class PRChecker { */ getWait(now) { const createTime = new Date(this.pr.createdAt); - const timeLeft = WAIT_TIME - Math.ceil( + const hoursFromCreateTime = Math.ceil( (now.getTime() - createTime.getTime()) / HOUR ); + const timeLeft = WAIT_TIME - hoursFromCreateTime; + const timeLeftSingleApproval = + WAIT_TIME_SINGLE_APPROVAL - hoursFromCreateTime; return { - timeLeft + timeLeft, + timeLeftSingleApproval }; } @@ -149,12 +154,12 @@ class PRChecker { const { pr, cli, reviewers, CIStatus } = this; + const { approved } = reviewers; 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'); return true; @@ -171,14 +176,25 @@ class PRChecker { return false; } + const dateStr = new Date(pr.createdAt).toDateString(); + cli.info(`This PR was created on ${dateStr}`); + const wait = this.getWait(now); - if (wait.timeLeft > 0) { - const dateStr = new Date(pr.createdAt).toDateString(); - cli.info(`This PR was created on ${dateStr}`); + if (approved.length > 1 && wait.timeLeft > 0) { cli.warn(`Wait at least ${wait.timeLeft} more hours before landing`); return false; + } else if (approved.length === 1 && wait.timeLeftSingleApproval > 0) { + const waitMsg = wait.timeLeft > 0 + ? ` and ${wait.timeLeft} more hours` + : ''; + cli.warn('Wait at one of the following:'); + cli.warn(`* another approval${waitMsg}`); + cli.warn(`* ${wait.timeLeftSingleApproval} more hours` + + ' with existing single approval'); + return false; } + cli.info('No more waiting required'); return true; } diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 3753545..6795d0b 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/pr_checker.test.js b/test/unit/pr_checker.test.js index b35ac2e..47f40d2 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -10,6 +10,7 @@ const PRChecker = require('../../lib/pr_checker'); const { allGreenReviewers, + singleGreenReviewer, requestedChangesReviewers, approvingReviews, requestingChangesReviews, @@ -169,7 +170,9 @@ describe('PRChecker', () => { const cli = new TestCLI(); const expectedLogs = { - warn: [['Wait at least 22 more hours before landing']], + warn: [ + ['Wait at least 22 more hours before landing'] + ], info: [['This PR was created on Tue Oct 31 2017']] }; @@ -197,6 +200,78 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should warn about PR with single approval (<48h)', () => { + const cli = new TestCLI(); + + const expectedLogs = { + warn: [ + ['Wait at one of the following:'], + ['* another approval and 22 more hours'], + ['* 142 more hours with existing single approval'] + ], + info: [['This PR was created on Tue Oct 31 2017']] + }; + + const now = new Date('2017-11-01T14:25:41.682Z'); + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: '2017-10-31T13:00:41.682Z' + }); + + 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.checkPRWait(now); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + + it('should warn about PR with single approval (>48h)', () => { + const cli = new TestCLI(); + + const expectedLogs = { + warn: [ + ['Wait at one of the following:'], + ['* another approval'], + ['* 94 more hours with existing single approval'] + ], + info: [['This PR was created on Tue Oct 31 2017']] + }; + + const now = new Date('2017-11-03T14:25:41.682Z'); + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: '2017-10-31T13:00:41.682Z' + }); + + 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.checkPRWait(now); + assert(!status); + cli.assertCalledWith(expectedLogs); + }); + it('should log as expected if PR can be fast-tracked', () => { const cli = new TestCLI();