fix: treat fast-track with not enough approvals as non-fatal (#676)

* fix: treat `fast-track` with not enough approvals as non-fatal * fix: update error messages * fix: error message fixup * fix: error message fixup * test: add test for fast-track with insufficient approvals
nodejs · Mar 3, 2023 · b324c99 · b324c99
1 parent ebcf18e
commit b324c99
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 16 deletions.
diff --git a/lib/pr_checker.js b/lib/pr_checker.js
@@ -27,6 +27,7 @@ const WAIT_TIME_SINGLE_APPROVAL = 24 * 7;
 const GITHUB_SUCCESS_CONCLUSIONS = ['SUCCESS', 'NEUTRAL', 'SKIPPED'];
 
 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';
 
 export default class PRChecker {
@@ -138,7 +139,7 @@ export default class PRChecker {
     const { requestedChanges, approved } = reviewers;
     const labels = pr.labels.nodes.map((l) => l.name);
 
-    const isFastTracked = labels.includes('fast-track');
+    let isFastTracked = labels.includes('fast-track');
     const isCodeAndLearn = labels.includes('code-and-learn');
     const isSemverMajor = labels.includes('semver-major');
 
@@ -168,6 +169,7 @@ export default class PRChecker {
       }
     }
 
+    let fastTrackAppendix = '';
     if (isFastTracked) {
       const comment = [...this.comments].reverse().find((c) =>
         FAST_TRACK_RE.test(c.bodyText));
@@ -183,14 +185,15 @@ export default class PRChecker {
         r.user.login !== pr.author.login &&
         collaborators.includes(r.user.login.toLowerCase())).length;
 
-      if (requester === pr.author.login && approvals < 2) {
-        cli.error('The fast-track request requires' +
-                  " at least two collaborators' approvals (👍).");
-        return false;
-      } else if (approvals === 0) {
-        cli.error('The fast-track request requires' +
-                  " at least one collaborator's approval (👍).");
-        return false;
+      const missingFastTrackApprovals = FAST_TRACK_MIN_APPROVALS - approvals -
+        (requester === pr.author.login ? 0 : 1);
+      if (missingFastTrackApprovals > 0) {
+        isFastTracked = false;
+        fastTrackAppendix = ' (or 0 hours if there ' +
+          `${missingFastTrackApprovals === 1 ? 'is' : 'are'} ` +
+          `${missingFastTrackApprovals} more approval` +
+          `${missingFastTrackApprovals === 1 ? '' : 's'} (👍) of ` +
+          'the fast-track request from collaborators).';
       }
     }
 
@@ -211,10 +214,12 @@ export default class PRChecker {
       if (timeLeftMulti === 0) {
         const timeLeftMins =
           this.waitTimeMultiApproval * 60 - minutesFromCreateTime;
-        cli.error(`This PR needs to wait ${timeLeftMins} more minutes to land`);
+        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`);
+      cli.error(`This PR needs to wait ${timeLeftMulti} more ` +
+                `hours to land${fastTrackAppendix}`);
       return false;
     }
 
@@ -224,7 +229,8 @@ export default class PRChecker {
       }
       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)`);
+                `(or ${timeLeftMulti} hours 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
@@ -644,8 +644,9 @@ describe('PRChecker', () => {
          [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'],
            ['This PR is being fast-tracked']],
         error:
-         [['The fast-track request requires at' +
-           " least two collaborators' approvals (👍)."]]
+          [['This PR needs to wait 24 more hours to land (or 0 hours if ' +
+            'there are 2 more approvals (👍) of the fast-track request from ' +
+            'collaborators).']]
       };
 
       const pr = Object.assign({}, firstTimerPR, {
@@ -694,8 +695,9 @@ describe('PRChecker', () => {
          [['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'],
            ['This PR is being fast-tracked']],
         error:
-         [['The fast-track request requires at' +
-           " least one collaborator's approval (👍)."]]
+          [['This PR needs to wait 24 more hours to land (or 0 hours if ' +
+            'there is 1 more approval (👍) of the fast-track request from ' +
+            'collaborators).']]
       };
 
       const pr = Object.assign({}, firstTimerPR, {
@@ -727,6 +729,51 @@ describe('PRChecker', () => {
       cli.assertCalledWith(expectedLogs);
     });
 
+    it('should error when not enough approvals or fast-track approvals', () => {
+      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 Fri, 30 Nov 2018 17:50:44 GMT'],
+           ['This PR is being fast-tracked']],
+        error:
+          [['This PR needs to wait 144 more hours to land (or 24 hours if ' +
+            'there is one more approval) (or 0 hours if there is 1 more ' +
+            'approval (👍) of the fast-track request from collaborators).']]
+      };
+
+      const pr = Object.assign({}, firstTimerPR, {
+        createdAt: LT_48H,
+        labels: {
+          nodes: [
+            { name: 'fast-track' }
+          ]
+        }
+      });
+
+      const data = {
+        pr,
+        reviewers: singleGreenReviewer,
+        comments: commentsWithFastTrackInsuffientApprovals,
+        reviews: approvingReviews,
+        commits: [],
+        collaborators,
+        authorIsNew: () => true,
+        getThread() {
+          return PRData.prototype.getThread.call(this);
+        }
+      };
+      const checker = new PRChecker(cli, data, {}, argv);
+
+      cli.clearCalls();
+      const status = checker.checkReviewsAndWait(new Date(NOW));
+      assert(!status);
+      cli.assertCalledWith(expectedLogs);
+    });
+
     it('should error when missing fast-track request comment', () => {
       const cli = new TestCLI();