[JENKINS-43507] Fresh eyes find and fix fresh bugs

- Need to upgrade to structs 1.9 to pick up JENKINS-45130 - Two legacy setters were not purging useless traits, and we were missing tests to verify same - Pick up git's -alpha-4
jenkinsci · Jun 26, 2017 · b35932383d8d4cd7185dfd1bd8e3c41c03d31b23 · b359323
1 parent 471e5b7
commit b35932383d8d4cd7185dfd1bd8e3c41c03d31b23
diff --git a/pom.xml b/pom.xml
@@ -36,7 +36,7 @@
        <dependency>
            <groupId>org.jenkins-ci.plugins</groupId>
            <artifactId>structs</artifactId>
-  
-            <version>1.8</version>
+  
+            <version>1.9</version>
        </dependency>
        <dependency>
            <groupId>org.jenkins-ci.plugins</groupId>
@@ -46,7 +46,7 @@
        <dependency>
            <groupId>org.jenkins-ci.plugins</groupId>
            <artifactId>git</artifactId>
-  
-            <version>3.4.0-alpha-1</version>
+  
+            <version>3.4.0-alpha-4</version>
        </dependency>
        <dependency>
            <groupId>org.jenkins-ci.plugins</groupId>

diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java
@@ -529,13 +529,18 @@ public void setBuildOriginBranch(boolean buildOriginBranch) {
        for (int i = 0; i < traits.size(); i++) {
            SCMTrait<?> trait = traits.get(i);
            if (trait instanceof BranchDiscoveryTrait) {
-  
-                traits.set(i, new BranchDiscoveryTrait(
-  
-                        buildOriginBranch, ((BranchDiscoveryTrait) trait).isBuildBranchesWithPR()
-  
-                ));
+  
+                BranchDiscoveryTrait previous = (BranchDiscoveryTrait) trait;
+  
+                if (buildOriginBranch || previous.isBuildBranchesWithPR()) {
+  
+                    traits.set(i, new BranchDiscoveryTrait(buildOriginBranch, previous.isBuildBranchesWithPR()));
+  
+                } else {
+  
+                    traits.remove(i);
+  
+                }
                return;
            }
        }
-  
-        traits.add(new BranchDiscoveryTrait(buildOriginBranch, false));
+  
+        if (buildOriginBranch) {
+  
+            traits.add(new BranchDiscoveryTrait(buildOriginBranch, false));
+  
+        }
    }
    /**
@@ -570,13 +575,18 @@ public void setBuildOriginBranchWithPR(boolean buildOriginBranchWithPR) {
        for (int i = 0; i < traits.size(); i++) {
            SCMTrait<?> trait = traits.get(i);
            if (trait instanceof BranchDiscoveryTrait) {
-  
-                traits.set(i, new BranchDiscoveryTrait(
-  
-                        ((BranchDiscoveryTrait) trait).isBuildBranch(), buildOriginBranchWithPR
-  
-                ));
+  
+                BranchDiscoveryTrait previous = (BranchDiscoveryTrait) trait;
+  
+                if (buildOriginBranchWithPR || previous.isBuildBranch()) {
+  
+                    traits.set(i, new BranchDiscoveryTrait(previous.isBuildBranch(), buildOriginBranchWithPR));
+  
+                } else {
+  
+                    traits.remove(i);
+  
+                }
                return;
            }
        }
-  
-        traits.add(new BranchDiscoveryTrait(false, buildOriginBranchWithPR));
+  
+        if (buildOriginBranchWithPR) {
+  
+            traits.add(new BranchDiscoveryTrait(false, buildOriginBranchWithPR));
+  
+        }
    }
    /**

diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
@@ -289,7 +289,7 @@ public GitHubSCMSource(@NonNull String repoOwner, @NonNull String repository) {
        this.repository = repository;
        pullRequestMetadataCache = new ConcurrentHashMap<>();
        pullRequestContributorCache = new ConcurrentHashMap<>();
-  
-        this.traits = new ArrayList<>(((DescriptorImpl) getDescriptor()).getTraitsDefaults());
+  
+        this.traits = new ArrayList<>();
    }
    /**
@@ -606,13 +606,18 @@ public void setBuildOriginBranch(boolean buildOriginBranch) {
        for (int i = 0; i < traits.size(); i++) {
            SCMTrait<?> trait = traits.get(i);
            if (trait instanceof BranchDiscoveryTrait) {
-  
-                traits.set(i, new BranchDiscoveryTrait(
-  
-                        buildOriginBranch, ((BranchDiscoveryTrait) trait).isBuildBranchesWithPR()
-  
-                ));
+  
+                BranchDiscoveryTrait previous = (BranchDiscoveryTrait) trait;
+  
+                if (buildOriginBranch || previous.isBuildBranchesWithPR()) {
+  
+                    traits.set(i, new BranchDiscoveryTrait(buildOriginBranch, previous.isBuildBranchesWithPR()));
+  
+                } else {
+  
+                    traits.remove(i);
+  
+                }
                return;
            }
        }
-  
-        traits.add(new BranchDiscoveryTrait(buildOriginBranch, false));
+  
+        if (buildOriginBranch) {
+  
+            traits.add(new BranchDiscoveryTrait(buildOriginBranch, false));
+  
+        }
    }
    @Deprecated
@@ -635,13 +640,18 @@ public void setBuildOriginBranchWithPR(boolean buildOriginBranchWithPR) {
        for (int i = 0; i < traits.size(); i++) {
            SCMTrait<?> trait = traits.get(i);
            if (trait instanceof BranchDiscoveryTrait) {
-  
-                traits.set(i, new BranchDiscoveryTrait(
-  
-                        ((BranchDiscoveryTrait) trait).isBuildBranch(), buildOriginBranchWithPR
-  
-                ));
+  
+                BranchDiscoveryTrait previous = (BranchDiscoveryTrait) trait;
+  
+                if (buildOriginBranchWithPR || previous.isBuildBranch()) {
+  
+                    traits.set(i, new BranchDiscoveryTrait(previous.isBuildBranch(), buildOriginBranchWithPR));
+  
+                } else {
+  
+                    traits.remove(i);
+  
+                }
                return;
            }
        }
-  
-        traits.add(new BranchDiscoveryTrait(false, buildOriginBranchWithPR));
+  
+        if (buildOriginBranchWithPR) {
+  
+            traits.add(new BranchDiscoveryTrait(false, buildOriginBranchWithPR));
+  
+        }
    }
    @Deprecated

diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTraitsTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTraitsTest.java
@@ -5,6 +5,7 @@
 import java.util.EnumSet;
 import jenkins.model.Jenkins;
 import jenkins.scm.api.mixin.ChangeRequestCheckoutStrategy;
+  
+import jenkins.scm.api.trait.SCMSourceTrait;
 import jenkins.scm.api.trait.SCMTrait;
 import jenkins.scm.impl.trait.RegexSCMSourceFilterTrait;
 import jenkins.scm.impl.trait.WildcardSCMHeadFilterTrait;
@@ -1972,4 +1973,36 @@ public void given__legacyCode_withIncludes__when__setExcludes_value__then__trait
        )));
    }
+  
+    @Test
+  
+    public void given__legacyCode__when__setBuildOriginBranch__then__traitsMaintained() {
+  
+        GitHubSCMNavigator instance = new GitHubSCMNavigator("test");
+  
+        instance.setTraits(Collections.<SCMTrait<?>>emptyList());
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMTrait<?>>emptyList()));
+  
+        instance.setBuildOriginBranch(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranch(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMTrait<?>>emptyList()));
+  
+
+  
+        instance.setBuildOriginBranchWithPR(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranchWithPR(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMTrait<?>>emptyList()));
+  
+
+  
+        instance.setBuildOriginBranchWithPR(true);
+  
+        instance.setBuildOriginBranch(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranchWithPR(false);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranch(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMTrait<?>>emptyList()));
+  
+
+  
+        instance.setBuildOriginBranchWithPR(true);
+  
+        instance.setBuildOriginBranch(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranch(false);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranchWithPR(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMTrait<?>>emptyList()));
+  
+    }
+  
+
 }
diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTraitsTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTraitsTest.java
@@ -73,7 +73,7 @@ public void given__configuredInstance__when__uninstantiating__then__deprecatedFi
                        + "repoOwner=repo-owner,"
                        + "repository=repo,"
                        + "traits=["
-  
-                        + "$BranchDiscoveryTrait(strategyId=1), "
+  
+                        + "$org.jenkinsci.plugins.github_branch_source.BranchDiscoveryTrait(strategyId=1), "
                        + "$OriginPullRequestDiscoveryTrait(strategyId=1), "
                        + "$ForkPullRequestDiscoveryTrait("
                        + "strategyId=2,"
@@ -433,6 +433,38 @@ public void given__instance__when__setTraits_empty__then__traitsEmpty() {
        assertThat(instance.getTraits(), is(Collections.<SCMSourceTrait>emptyList()));
    }
+  
+    @Test
+  
+    public void given__legacyCode__when__setBuildOriginBranch__then__traitsMaintained() {
+  
+        GitHubSCMSource instance = new GitHubSCMSource("testing", "test-repo");
+  
+        instance.setTraits(Collections.<SCMSourceTrait>emptyList());
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMSourceTrait>emptyList()));
+  
+        instance.setBuildOriginBranch(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranch(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMSourceTrait>emptyList()));
+  
+
+  
+        instance.setBuildOriginBranchWithPR(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranchWithPR(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMSourceTrait>emptyList()));
+  
+
+  
+        instance.setBuildOriginBranchWithPR(true);
+  
+        instance.setBuildOriginBranch(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranchWithPR(false);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranch(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMSourceTrait>emptyList()));
+  
+
+  
+        instance.setBuildOriginBranchWithPR(true);
+  
+        instance.setBuildOriginBranch(true);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranch(false);
+  
+        assertThat(instance.getTraits(), contains(instanceOf(BranchDiscoveryTrait.class)));
+  
+        instance.setBuildOriginBranchWithPR(false);
+  
+        assertThat(instance.getTraits(), is(Collections.<SCMSourceTrait>emptyList()));
+  
+    }
+  
+
    @Test
    public void given__instance__when__setTraits__then__traitsSet() {
        GitHubSCMSource instance = new GitHubSCMSource("testing", "test-repo");