Skip to content

Commit

Permalink
Distinguish between unknown and unmergeable states
Browse files Browse the repository at this point in the history
  • Loading branch information
@bitwiseman
bitwiseman committed Apr 26, 2019
1 parent f291964 commit 894415f
Show file tree
Hide file tree
Showing 7 changed files with 451 additions and 23 deletions.
4 changes: 3 additions & 1 deletion src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1475,7 +1475,9 @@ private PullRequestSCMRevision createPullRequestSCMRevision(GHPullRequest pr, Pu
case MERGE:
Connector.checkApiRateLimit(listener, github);
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
mergeHash = Boolean.TRUE.equals(pr.getMergeable()) ? pr.getMergeCommitSha() : null;
if (pr.getMergeable() != null) {
mergeHash = Boolean.TRUE.equals(pr.getMergeable()) ? pr.getMergeCommitSha() : PullRequestSCMRevision.NOT_MERGEABLE_HASH;
}
break;
default:
break;
Expand Down
14 changes: 11 additions & 3 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
public class PullRequestSCMRevision extends ChangeRequestSCMRevision<PullRequestSCMHead> {

private static final long serialVersionUID = 1L;
static final String NOT_MERGEABLE_HASH = "NOT_MERGEABLE";

private final @NonNull String baseHash;
private final @NonNull String pullHash;
Expand Down Expand Up @@ -108,9 +109,11 @@ public String getMergeHash() {

void validateMergeHash(@NonNull GHRepository repo) throws IOException {
if (!isMerge()) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not a merge head.");
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not a merge head");
} else if (this.mergeHash == null) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable or connection failure " + this.toString());
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Unknown merge state " + this.toString());
} else if (this.mergeHash == NOT_MERGEABLE_HASH) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable " + this.toString());
} else {
List<String> parents = repo.getCommit(this.mergeHash).getParentSHA1s();
if (parents.size() != 2 || !parents.contains(this.getBaseHash()) || !parents.contains(this.getPullHash())) {
Expand All @@ -135,6 +138,11 @@ public int _hashCode() {

@Override
public String toString() {
return getHead() instanceof PullRequestSCMHead && ((PullRequestSCMHead) getHead()).isMerge() ? pullHash + "+" + baseHash + " (" + StringUtils.defaultIfEmpty(mergeHash, "null") + ")" : pullHash;
String result = pullHash;
if (getHead() instanceof PullRequestSCMHead && ((PullRequestSCMHead) getHead()).isMerge()) {
result += "+" + baseHash +
" (" + StringUtils.defaultIfBlank(mergeHash, "UNKNOWN_MERGE_STATE") + ")";
}
return result;
}
}
9 changes: 9 additions & 0 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.instanceOf;
Expand Down Expand Up @@ -165,6 +166,14 @@ public void prepareMockGitHub() throws Exception {
new SingleRootFileSource("src/test/resources/api/__files"));
githubRaw.enableRecordMappings(new SingleRootFileSource("src/test/resources/raw/mappings"),
new SingleRootFileSource("src/test/resources/raw/__files"));

githubApi.stubFor(
get(urlEqualTo("/repos/cloudbeers/yolo/pulls/2"))
.willReturn(
aResponse()
.withHeader("Content-Type", "application/json; charset=utf-8")
.withBodyFile("body-yolo-pulls-2-mergeable-true.json")));

githubApi.stubFor(
get(urlMatching(".*")).atPriority(10).willReturn(aResponse().proxiedFrom("https://api.github.com/")));
githubRaw.stubFor(get(urlMatching(".*")).atPriority(10)
Expand Down
88 changes: 87 additions & 1 deletion src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import com.github.tomakehurst.wiremock.http.Request;
import com.github.tomakehurst.wiremock.http.Response;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import com.github.tomakehurst.wiremock.stubbing.Scenario;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.AbortException;
import hudson.ExtensionList;
Expand Down Expand Up @@ -84,10 +86,12 @@
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
import java.util.EnumSet;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasProperty;
import static org.hamcrest.Matchers.hasSize;
Expand Down Expand Up @@ -156,6 +160,37 @@ public void prepareMockGitHub() throws Exception {
new SingleRootFileSource("src/test/resources/api/__files"));
githubRaw.enableRecordMappings(new SingleRootFileSource("src/test/resources/raw/mappings"),
new SingleRootFileSource("src/test/resources/raw/__files"));

githubApi.stubFor(
get(urlEqualTo("/repos/cloudbeers/yolo/pulls/2"))
.inScenario("Pull Request Merge Hash")
.whenScenarioStateIs(Scenario.STARTED)
.willReturn(
aResponse()
.withHeader("Content-Type", "application/json; charset=utf-8")
.withBodyFile("body-yolo-pulls-2-mergeable-null.json"))
.willSetStateTo("Pull Request Merge Hash - retry 1"));

githubApi.stubFor(
get(urlEqualTo("/repos/cloudbeers/yolo/pulls/2"))
.inScenario("Pull Request Merge Hash")
.whenScenarioStateIs("Pull Request Merge Hash - retry 1")
.willReturn(
aResponse()
.withHeader("Content-Type", "application/json; charset=utf-8")
.withBodyFile("body-yolo-pulls-2-mergeable-null.json"))
.willSetStateTo("Pull Request Merge Hash - retry 2"));

githubApi.stubFor(
get(urlEqualTo("/repos/cloudbeers/yolo/pulls/2"))
.inScenario("Pull Request Merge Hash")
.whenScenarioStateIs("Pull Request Merge Hash - retry 2")
.willReturn(
aResponse()
.withHeader("Content-Type", "application/json; charset=utf-8")
.withBodyFile("body-yolo-pulls-2-mergeable-true.json"))
.willSetStateTo("Pull Request Merge Hash - retry 2"));

githubApi.stubFor(
get(urlMatching(".*")).atPriority(10).willReturn(aResponse().proxiedFrom("https://api.github.com/")));
githubRaw.stubFor(get(urlMatching(".*")).atPriority(10)
Expand Down Expand Up @@ -241,7 +276,8 @@ public boolean isHead(@NonNull Probe probe, @NonNull TaskListener listener) thro
assertThat(((PullRequestSCMHead)byName.get("PR-3")).isMerge(), is(true));
assertThat(revByName.get("PR-3"), is((SCMRevision) new PullRequestSCMRevision((PullRequestSCMHead)(byName.get("PR-3")),
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
"c0e024f89969b976da165eecaa71e09dc60c3da1"
"c0e024f89969b976da165eecaa71e09dc60c3da1",
PullRequestSCMRevision.NOT_MERGEABLE_HASH
)));

// validation should fail for this PR.
Expand All @@ -252,6 +288,7 @@ public boolean isHead(@NonNull Probe probe, @NonNull TaskListener listener) thro
abort = e;
}
assertThat(abort, instanceOf(AbortException.class));
assertThat(abort.getMessage(), containsString("Not mergeable"));

assertThat(byName.get("master"), instanceOf(BranchSCMHead.class));
assertThat(revByName.get("master"),
Expand All @@ -263,6 +300,53 @@ public boolean isHead(@NonNull Probe probe, @NonNull TaskListener listener) thro
));
}

@Test
public void fetchSmokesUnknownMergeable() throws Exception {
// make it so PR-2 always returns mergeable = null
githubApi.stubFor(
get(urlEqualTo("/repos/cloudbeers/yolo/pulls/2"))
.inScenario("Pull Request Merge Hash")
.whenScenarioStateIs(Scenario.STARTED)
.willReturn(
aResponse()
.withHeader("Content-Type", "application/json; charset=utf-8")
.withBodyFile("body-yolo-pulls-2-mergeable-null.json")));

SCMHeadObserver.Collector collector = SCMHeadObserver.collect();
source.fetch(new SCMSourceCriteria() {
@Override
public boolean isHead(@NonNull Probe probe, @NonNull TaskListener listener) throws IOException {
return probe.stat("README.md").getType() == SCMFile.Type.REGULAR_FILE;
}
}, collector, null, null);
Map<String,SCMHead> byName = new HashMap<>();
Map<String,SCMRevision> revByName = new HashMap<>();
for (Map.Entry<SCMHead, SCMRevision> h: collector.result().entrySet()) {
byName.put(h.getKey().getName(), h.getKey());
revByName.put(h.getKey().getName(), h.getValue());
}

assertThat(byName.keySet(), containsInAnyOrder("PR-2", "PR-3", "master", "stephenc-patch-1"));

assertThat(byName.get("PR-2"), instanceOf(PullRequestSCMHead.class));
assertThat(((PullRequestSCMHead)byName.get("PR-2")).isMerge(), is(true));
assertThat(revByName.get("PR-2"), is((SCMRevision) new PullRequestSCMRevision((PullRequestSCMHead)(byName.get("PR-2")),
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
"c0e024f89969b976da165eecaa71e09dc60c3da1",
null
)));

// validation should fail for this PR.
Exception abort = null;
try {
((PullRequestSCMRevision)revByName.get("PR-2")).validateMergeHash(repo);
} catch (Exception e) {
abort = e;
}
assertThat(abort, instanceOf(AbortException.class));
assertThat(abort.getMessage(), containsString("Unknown merge state"));
}

@Test
public void fetchAltConfig() throws Exception {
source.setBuildForkPRMerge(false);
Expand Down Expand Up @@ -298,6 +382,8 @@ public boolean isHead(@NonNull Probe probe, @NonNull TaskListener listener) thro
abort = e;
}
assertThat(abort, instanceOf(AbortException.class));
assertThat(abort.getMessage(), containsString("Not a merge head"));


assertThat(byName.get("PR-3"), instanceOf(PullRequestSCMHead.class));
assertThat(((PullRequestSCMHead)byName.get("PR-3")).isMerge(), is(false));
Expand Down
54 changes: 36 additions & 18 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevisionTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* The MIT License
*
* Copyright (c) 2016 CloudBees, Inc.
* Copyright (c) 2019 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -34,27 +34,14 @@
import com.github.tomakehurst.wiremock.http.Response;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import jenkins.plugins.git.AbstractGitSCMSource;
import jenkins.scm.api.SCMFile;
import jenkins.scm.api.SCMFileSystem;
import jenkins.scm.api.SCMHead;
import jenkins.scm.api.SCMHeadOrigin;
import jenkins.scm.api.SCMRevision;
import jenkins.scm.api.mixin.ChangeRequestCheckoutStrategy;

import org.antlr.v4.runtime.misc.NotNull;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;
import org.hamcrest.Matchers;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.jvnet.hudson.test.JenkinsRule;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GitHub;
Expand All @@ -65,12 +52,11 @@
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;

public class PullRequestSCMRevisionTest {
/**
Expand Down Expand Up @@ -198,7 +184,7 @@ public void createHeadwithMergeRevision() throws Exception {
public void createMergewithNullMergeRevision() throws Exception {
PullRequestSCMRevision prRevision = new PullRequestSCMRevision(
prMerge, "master-revision", "pr-branch-revision");
assertThat(prRevision.toString(), is("pr-branch-revision+master-revision (null)"));
assertThat(prRevision.toString(), is("pr-branch-revision+master-revision (UNKNOWN_MERGE_STATE)"));

// equality
assertThat(prRevision,
Expand All @@ -211,6 +197,34 @@ public void createMergewithNullMergeRevision() throws Exception {
assertThat(prRevision,
not(is(new PullRequestSCMRevision(prHead, "master-revision", "pr-branch-revision", null))));

// validation should fail for this PR.
Exception abort = null;
try {
prRevision.validateMergeHash(repo);
} catch (Exception e) {
abort = e;
}
assertThat(abort, instanceOf(AbortException.class));
assertThat(abort.getMessage(), containsString("Unknown merge state"));
}

@Test
public void createMergewithNotMergeableRevision() throws Exception {
PullRequestSCMRevision prRevision = new PullRequestSCMRevision(
prMerge, "master-revision", "pr-branch-revision", PullRequestSCMRevision.NOT_MERGEABLE_HASH);
assertThat(prRevision.toString(), is("pr-branch-revision+master-revision (NOT_MERGEABLE)"));

// equality
assertThat(prRevision,
not(is(new PullRequestSCMRevision(prMerge, "master-revision", "pr-branch-revision", null))));
assertThat(prRevision,
not(is(new PullRequestSCMRevision(prMerge, "master-revision", "pr-branch-revision", ""))));
assertThat(prRevision,
not(is(new PullRequestSCMRevision(prMerge, "master-revision", "pr-branch-revision", "pr-merge-revision"))));

assertThat(prRevision,
not(is(new PullRequestSCMRevision(prHead, "master-revision", "pr-branch-revision", null))));

// validation should fail for this PR.
Exception abort = null;
try {
Expand Down Expand Up @@ -249,7 +263,11 @@ public void createMergewithMergeRevision_validation_valid() throws Exception {
"38814ca33833ff5583624c29f305be9133f27a40");

assertThat(prRevision.toString(), is("c0e024f89969b976da165eecaa71e09dc60c3da1+8f1314fc3c8284d8c6d5886d473db98f2126071c (38814ca33833ff5583624c29f305be9133f27a40)"));
prRevision.validateMergeHash(repo);
try {
prRevision.validateMergeHash(repo);
} catch (AbortException e) {
fail("Validation should succeed, but: " + e.getMessage());
}
}

@Test
Expand Down

0 comments on commit 894415f

Please sign in to comment.