From fb7702a4da5d5b245c5f049de0f1c0c9b55a2275 Mon Sep 17 00:00:00 2001 From: Sebastian Wilzbach Date: Thu, 4 Jan 2018 09:21:36 +0100 Subject: [PATCH 1/2] Move avoid checking for stallement into a separate function --- test/cronjob.d | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/test/cronjob.d b/test/cronjob.d index 9b7f741..5752a71 100644 --- a/test/cronjob.d +++ b/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){ From 5e8691c0e66a5f3d250d6467b6345aef8b321f49 Mon Sep 17 00:00:00 2001 From: Sebastian Wilzbach Date: Thu, 4 Jan 2018 09:37:47 +0100 Subject: [PATCH 2/2] Improve mergeableState recognition --- source/dlangbot/cron.d | 36 ++++++++++++++++++++++++------------ test/cronjob.d | 27 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/source/dlangbot/cron.d b/source/dlangbot/cron.d index b7cc7b4..c90008c 100644 --- a/source/dlangbot/cron.d +++ b/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; } diff --git a/test/cronjob.d b/test/cronjob.d index 5752a71..7ae4502 100644 --- a/test/cronjob.d +++ b/test/cronjob.d @@ -138,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); +}