diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 273d0fe7..a4dfeaac 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -19,7 +19,6 @@ const { FROM_COMMENT, FROM_REVIEW_COMMENT } = REVIEW_SOURCES; const SECOND = 1000; const MINUTE = SECOND * 60; -const HOUR = MINUTE * 60; const WAIT_TIME_MULTI_APPROVAL = 24 * 2; const WAIT_TIME_SINGLE_APPROVAL = 24 * 7; @@ -196,10 +195,18 @@ export default class PRChecker { const createTime = new Date(this.pr.createdAt); const msFromCreateTime = now.getTime() - createTime.getTime(); - const minutesFromCreateTime = Math.ceil(msFromCreateTime / MINUTE); - const hoursFromCreateTime = Math.ceil(msFromCreateTime / HOUR); - let timeLeftMulti = this.waitTimeMultiApproval - hoursFromCreateTime; - const timeLeftSingle = this.waitTimeSingleApproval - hoursFromCreateTime; + const minutesFromCreateTime = msFromCreateTime / MINUTE; + const timeLeftMulti = this.waitTimeMultiApproval * 60 - minutesFromCreateTime; + const timeLeftSingle = this.waitTimeSingleApproval * 60 - minutesFromCreateTime; + const timeToText = (time, liaison_word = undefined) => { + let unity = 'minute'; + if (time > 59) { + unity = 'hour'; + time /= 60; + } + time = Math.ceil(time); + return `${time} ${liaison_word ? liaison_word + ' ' : ''}${unity}${time === 1 ? '' : 's'}`; + }; if (approved.length >= 2) { if (isFastTracked || isCodeAndLearn) { @@ -208,15 +215,9 @@ export default class PRChecker { if (timeLeftMulti < 0) { return true; } - if (timeLeftMulti === 0) { - const timeLeftMins = - this.waitTimeMultiApproval * 60 - minutesFromCreateTime; - cli.error(`This PR needs to wait ${timeLeftMins} ` + - `more minutes to land${fastTrackAppendix}`); - return false; - } - cli.error(`This PR needs to wait ${timeLeftMulti} more ` + - `hours to land${fastTrackAppendix}`); + cli.error( + `This PR needs to wait ${timeToText(timeLeftMulti, 'more')} to land${fastTrackAppendix}` + ); return false; } @@ -224,10 +225,9 @@ export default class PRChecker { if (timeLeftSingle < 0) { return true; } - timeLeftMulti = timeLeftMulti < 0 || isFastTracked ? 0 : timeLeftMulti; - cli.error(`This PR needs to wait ${timeLeftSingle} more hours to land ` + - `(or ${timeLeftMulti} hours if there is one more approval)` + - fastTrackAppendix); + cli.error(`This PR needs to wait ${timeToText(timeLeftSingle, 'more')} to land (or ${ + timeToText(timeLeftMulti < 0 || isFastTracked ? 0 : timeLeftMulti) + } if there is one more approval)${fastTrackAppendix}`); return false; } } diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index eff5812b..4542e194 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -50,6 +50,7 @@ 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 LT_48H_GT_47H = '2018-11-29T17:55:44.477Z'; +const LT_48H_GT_47_59 = '2018-11-29T17:51:43.477Z'; const NOW = '2018-11-31T17:50:44.477Z'; const argv = { maxCommits: 3 }; @@ -304,6 +305,43 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); + it('should error when PR is younger than 48h and older than 47h58min', () => { + 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 Thu, 29 Nov 2018 17:51:43 GMT']], + error: [['This PR needs to wait 1 more minute to land']] + }; + + const youngPR = Object.assign({}, firstTimerPR, { + createdAt: LT_48H_GT_47_59 + }); + + const data = { + pr: youngPR, + reviewers: allGreenReviewers, + 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(); @@ -350,7 +388,7 @@ describe('PRChecker', () => { 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)']] + '(or 0 minutes if there is one more approval)']] }; const youngPR = Object.assign({}, firstTimerPR, {