fix(git-node): harmonize time left error messages

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,26 +215,19 @@ 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;
     }
 
     if (approved.length === 1) {
       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;
     }
   }

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, {