Merge pull request #159 from wilzbach/improve-merge2

Improve mergeableState recognition
merged-on-behalf-of: Sebastian Wilzbach <sebi.wilzbach@gmail.com>

Author: dlang-bot

source/dlangbot/cron.d

@@ -74,15 +74,17 @@ auto detectInactiveStablePR(PRTuple t)
 
 auto detectPRWithMergeConflicts(PRTuple t)
 {
+    import std.typecons : Nullable;
+
     if (t.pr.mergeable.isNull)
     {
-        logInfo("[cron-daily/%s/%d]: detectMerge - mergeable is null.", t.pr.repoSlug, t.pr.number);
+        logInfo("[cron-daily/%s/%d/detectMerge]: mergeable is null.", t.pr.repoSlug, t.pr.number);
         // repeat request to receive computed mergeable information
         foreach (i; 0 .. 4)
         {
             import vibe.core.core : sleep;
             t.config.waitAfterMergeNullState.sleep;
-            logInfo("[cron-daily/%s/%d]: detectMerge - repeating request", t.pr.repoSlug, t.pr.number);
+            logInfo("[cron-daily/%s/%d/detectMerge]: repeating request", t.pr.repoSlug, t.pr.number);
             t.pr = t.pr.refresh;
             if (!t.pr.mergeable.isNull)
                 goto mergable;
@@ -91,30 +93,40 @@ auto detectPRWithMergeConflicts(PRTuple t)
     }
 mergable:
 
-    bool isMergeable;
+    Nullable!bool isMergeable;
     if (!t.pr.mergeableState.isNull)
     {
-        logInfo("[cron-daily/%s/%d]: mergableState = %s", t.pr.repoSlug, t.pr.number, t.pr.mergeableState.get);
+        logInfo("[cron-daily/%s/%d/detectMerge]: mergeableState = %s", t.pr.repoSlug, t.pr.number, t.pr.mergeableState.get);
         with (PullRequest.MergeableState)
         final switch(t.pr.mergeableState)
         {
-            case unknown, checking:
-                // should only be set if mergeable is null
-                return LabelResponse(LabelAction.none, "");
-            case clean, unstable:
+            case clean:
+                // branch is up to date with master and has no conflicts
+                isMergeable = true;
+                break;
+            case unstable:
+                // branch isn't up to date with master, but has no conflicts
                 isMergeable = true;
                 break;
             case dirty:
+                // GitHub detected conflicts
                 isMergeable = false;
                 break;
-            // a reviewer has blocked the merge (not observed yet)
+            case unknown, checking:
+                // should only be set if mergeable is null
             case blocked:
-                return LabelResponse(LabelAction.none, "");
+                // the repo requires reviews and the PR hasn't been approved yet
+                // the repo requires status checks and they have failed
+                break;
         }
     }
-    else
+
+    if (isMergeable.isNull)
     {
-        logInfo("[cron-daily/%s/%d]: mergable = %s", t.pr.repoSlug, t.pr.number, t.pr.mergeable.get);
+        if (t.pr.mergeable.isNull)
+            return LabelResponse(LabelAction.none, "");
+
+        logInfo("[cron-daily/%s/%d/detectMerge]: mergeable = %s", t.pr.repoSlug, t.pr.number, t.pr.mergeable.get);
         isMergeable = t.pr.mergeable.get;
     }
 

test/cronjob.d

@@ -5,6 +5,13 @@ import std.stdio;
 
 string[] repositories = ["dlang/phobos"];
 
+void dontTestStalled(ref Json j)
+{
+    import std.datetime : Clock, days;
+    j[$ - 1]["created_at"] = (Clock.currTime - 2.days).toISOExtString;
+    j[$ - 1]["updated_at"] = (Clock.currTime - 2.days).toISOExtString;
+}
+
 // test the first items of the cron job
 unittest
 {
@@ -62,13 +69,13 @@ unittest
 // test that stalled isn't falsely removed (e.g. by recent labelling)
 unittest
 {
-    import std.datetime : Clock, days;
     setAPIExpectations(
         "/github/repos/dlang/phobos/issues?state=open&sort=updated&direction=asc", (ref Json j) {
             // only test one pull request
             j = Json([j[0]]);
         },
         "/github/repos/dlang/phobos/pulls/2526", (ref Json j) {
+            import std.datetime : Clock, days;
             // simulate a recent label update
             j["updated_at"] = (Clock.currTime - 2.days).toISOExtString;
         },
@@ -88,7 +95,6 @@ unittest
 // test that no label updates are sent if no activity was found
 unittest
 {
-    import std.datetime : Clock, days;
     setAPIExpectations(
         "/github/repos/dlang/phobos/issues?state=open&sort=updated&direction=asc", (ref Json j) {
             // only test one pull request
@@ -98,10 +104,7 @@ unittest
             j["mergeable"] = false;
         },
         "/github/repos/dlang/phobos/status/a04acd6a2813fb344d3e47369cf7fd64523ece44",
-        "/github/repos/dlang/phobos/issues/2526/comments", (ref Json j) {
-            j[$ - 1]["created_at"] = (Clock.currTime - 2.days).toISOExtString;
-            j[$ - 1]["updated_at"] = (Clock.currTime - 2.days).toISOExtString;
-        },
+        "/github/repos/dlang/phobos/issues/2526/comments", &dontTestStalled,
         "/github/repos/dlang/phobos/pulls/2526/comments",
     );
 
@@ -111,7 +114,6 @@ unittest
 // test that the merge state gets refreshed
 unittest
 {
-    import std.datetime : Clock, days;
     setAPIExpectations(
         "/github/repos/dlang/phobos/issues?state=open&sort=updated&direction=asc", (ref Json j) {
             // only test one pull request
@@ -125,10 +127,7 @@ unittest
             j["mergeable_state"] = "dirty";
         },
         "/github/repos/dlang/phobos/status/a04acd6a2813fb344d3e47369cf7fd64523ece44",
-        "/github/repos/dlang/phobos/issues/2526/comments", (ref Json j) {
-            j[$ - 1]["created_at"] = (Clock.currTime - 2.days).toISOExtString;
-            j[$ - 1]["updated_at"] = (Clock.currTime - 2.days).toISOExtString;
-        },
+        "/github/repos/dlang/phobos/issues/2526/comments", &dontTestStalled,
         "/github/repos/dlang/phobos/pulls/2526/comments",
         "/github/repos/dlang/phobos/issues/2526/labels",
         (scope HTTPServerRequest req, scope HTTPServerResponse res){
@@ -139,3 +138,30 @@ unittest
 
     testCronDaily(repositories);
 }
+
+// for "blocked" PRs, the `mergeable` attribute should be preferred
+// if mergeable is true, "needs rebase" should be removed
+unittest
+{
+    setAPIExpectations(
+        "/github/repos/dlang/phobos/issues?state=open&sort=updated&direction=asc", (ref Json j) {
+            // only test one pull request
+            j = Json([j[0]]);
+            j[0]["labels"][0]["name"] = "needs rebase";
+        },
+        "/github/repos/dlang/phobos/pulls/2526", (ref Json j) {
+            j["mergeable"] = true;
+            j["mergeable_state"] = "blocked";
+        },
+        "/github/repos/dlang/phobos/status/a04acd6a2813fb344d3e47369cf7fd64523ece44",
+        "/github/repos/dlang/phobos/issues/2526/comments", &dontTestStalled,
+        "/github/repos/dlang/phobos/pulls/2526/comments",
+        "/github/repos/dlang/phobos/issues/2526/labels",
+        (scope HTTPServerRequest req, scope HTTPServerResponse res){
+            assert(req.method == HTTPMethod.PUT);
+            assert(req.json[].length == 0);
+        },
+    );
+
+    testCronDaily(repositories);
+}