Skip to content

Commit

Permalink
JENKINS-57583 Ignore merge commmit when comparing revisions
Browse files Browse the repository at this point in the history 
Other plugins assume that there are are only two revisions
involved in PR revision identity. Since the mergeHash is generated
based on the base and source hashes, it is not particularly
interesting to use it and can cause bad behavior if we do.

The only place where the mergeHash is interesting is when
getting Jenkinsfile or doing readTrusted. It is an optimization
that should have as few side effects as possible.
  • Loading branch information
@bitwiseman
bitwiseman committed May 22, 2019
1 parent de83966 commit 9977bd4
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 72 deletions.
8 changes: 6 additions & 2 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 @@ -118,12 +118,16 @@ public boolean equivalent(ChangeRequestSCMRevision<?> o) {
return false;
}
PullRequestSCMRevision other = (PullRequestSCMRevision) o;
return getHead().equals(other.getHead()) && pullHash.equals(other.pullHash) && StringUtils.equals(getMergeHash(), other.getMergeHash());

// JENKINS-57583 - Equivalent is used to make decisiions about when to build.
// mergeHash is an implementation detail of github, generated from base and target
// If only mergeHash changes we do not consider it a different revision
return getHead().equals(other.getHead()) && pullHash.equals(other.pullHash);
}

@Override
public int _hashCode() {
return pullHash.hashCode() + ((mergeHash == null) ? 0 : mergeHash.hashCode());
return pullHash.hashCode();
}

@Override
Expand Down
193 changes: 123 additions & 70 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevisionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import jenkins.scm.api.SCMHeadOrigin;
import jenkins.scm.api.mixin.ChangeRequestCheckoutStrategy;

import org.apache.tools.ant.taskdefs.condition.IsTrue;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Rule;
Expand All @@ -55,7 +56,9 @@
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

public class PullRequestSCMRevisionTest {
Expand Down Expand Up @@ -134,125 +137,175 @@ public void prepareMockGitHub() throws Exception {

@Test
public void createHeadwithNullMergeRevision() throws Exception {
PullRequestSCMRevision prRevision = new PullRequestSCMRevision(
prHead, "master-revision", "pr-branch-revision");
assertThat(prRevision.toString(), is("pr-branch-revision"));
PullRequestSCMHead currentHead = prHead;
PullRequestSCMHead otherHead = prMerge;

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

assertThat(prRevision,
not(is(new PullRequestSCMRevision(prMerge, "master-revision", "pr-branch-revision", null))));
PullRequestSCMRevision currentRevision = new PullRequestSCMRevision(
currentHead, "master-revision", "pr-branch-revision");
assertThat(currentRevision.toString(), is("pr-branch-revision"));

try {
prRevision.validateMergeHash();
currentRevision.validateMergeHash();
} catch (AbortException e) {
fail("Validation should succeed, but: " + e.getMessage());
}

// equivalence
assertTrue(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision-changed", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(otherHead, "master-revision-changed", "pr-branch-revision", "any")));

// equality
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", null)));
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", "any")));
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any")));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision-changed", "any"))));

assertThat(currentRevision,
not(is(new PullRequestSCMRevision(otherHead, "master-revision", "pr-branch-revision", null))));
}

@Test
public void createHeadwithMergeRevision() throws Exception {
PullRequestSCMRevision prRevision = new PullRequestSCMRevision(
prHead, "master-revision", "pr-branch-revision", "pr-merge-revision");
assertThat(prRevision.toString(), is("pr-branch-revision"));

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

assertThat(prRevision,
not(is(new PullRequestSCMRevision(prMerge, "master-revision", "pr-branch-revision", "pr-merge-revision"))));
PullRequestSCMRevision currentRevision = new PullRequestSCMRevision(
currentHead, "master-revision", "pr-branch-revision", "pr-merge-revision");
assertThat(currentRevision.toString(), is("pr-branch-revision"));

try {
prRevision.validateMergeHash();
currentRevision.validateMergeHash();
} catch (AbortException e) {
fail("Validation should succeed, but: " + e.getMessage());
}

// equivalence
assertTrue(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision-changed", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(otherHead, "master-revision-changed", "pr-branch-revision", "any")));

// equality
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", null)));
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", "any")));
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any")));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision-changed", "any"))));

assertThat(currentRevision,
not(is(new PullRequestSCMRevision(otherHead, "master-revision", "pr-branch-revision", null))));
}

@Test
public void createMergewithNullMergeRevision() throws Exception {
PullRequestSCMRevision prRevision = new PullRequestSCMRevision(
prMerge, "master-revision", "pr-branch-revision");
assertThat(prRevision.toString(), is("pr-branch-revision+master-revision (UNKNOWN_MERGE_STATE)"));
PullRequestSCMHead currentHead = prMerge;
PullRequestSCMHead otherHead = prHead;

// equality
assertThat(prRevision,
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))));
PullRequestSCMRevision currentRevision = new PullRequestSCMRevision(
currentHead, "master-revision", "pr-branch-revision");
assertThat(currentRevision.toString(), is("pr-branch-revision+master-revision (UNKNOWN_MERGE_STATE)"));

try {
prRevision.validateMergeHash();
currentRevision.validateMergeHash();
} catch (AbortException e) {
fail("Validation should succeed, but: " + e.getMessage());
}

// equivalence
assertTrue(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision-changed", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(otherHead, "master-revision-changed", "pr-branch-revision", "any")));

// equality
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", null)));
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", "any")));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any"))));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision-changed", "any"))));

assertThat(currentRevision,
not(is(new PullRequestSCMRevision(otherHead, "master-revision", "pr-branch-revision", null))));
}

@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"))));
PullRequestSCMHead currentHead = prMerge;
PullRequestSCMHead otherHead = prHead;

assertThat(prRevision,
not(is(new PullRequestSCMRevision(prHead, "master-revision", "pr-branch-revision", null))));
PullRequestSCMRevision currentRevision = new PullRequestSCMRevision(
currentHead, "master-revision", "pr-branch-revision", PullRequestSCMRevision.NOT_MERGEABLE_HASH);
assertThat(currentRevision.toString(), is("pr-branch-revision+master-revision (NOT_MERGEABLE)"));

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

// equivalence
assertTrue(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision-changed", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(otherHead, "master-revision-changed", "pr-branch-revision", "any")));

// equality
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", null)));
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", "any")));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any"))));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision-changed", "any"))));

assertThat(currentRevision,
not(is(new PullRequestSCMRevision(otherHead, "master-revision", "pr-branch-revision", null))));
}

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

// 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,
is(new PullRequestSCMRevision(prMerge, "master-revision", "pr-branch-revision", "pr-merge-revision")));
PullRequestSCMHead currentHead = prMerge;
PullRequestSCMHead otherHead = prHead;

assertThat(prRevision,
not(is(new PullRequestSCMRevision(prHead, "master-revision", "pr-branch-revision", "pr-merge-revision"))));
PullRequestSCMRevision currentRevision = new PullRequestSCMRevision(
currentHead, "master-revision", "pr-branch-revision", "pr-merge-revision");
assertThat(currentRevision.toString(), is("pr-branch-revision+master-revision (pr-merge-revision)"));

try {
prRevision.validateMergeHash();
currentRevision.validateMergeHash();
} catch (AbortException e) {
fail("Validation should succeed, but: " + e.getMessage());
}

// equivalence
assertTrue(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision-changed", "any")));
assertFalse(currentRevision.equivalent(new PullRequestSCMRevision(otherHead, "master-revision-changed", "pr-branch-revision", "any")));

// equality
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", null)));
assertThat(currentRevision,
is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision", "any")));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision-changed", "pr-branch-revision", "any"))));
assertThat(currentRevision,
not(is(new PullRequestSCMRevision(currentHead, "master-revision", "pr-branch-revision-changed", "any"))));

assertThat(currentRevision,
not(is(new PullRequestSCMRevision(otherHead, "master-revision", "pr-branch-revision", null))));
}
}

0 comments on commit 9977bd4

Please sign in to comment.