Merge pull request #1628 from olivergondza/JENKINS-27635

[JENKINS-20989] PeepholePermalink RunListenerImpl oncompleted should be triggered before downstream builds are triggered
jenkinsci · Apr 4, 2015 · ad56599 · ad56599
2 parents f2c2f0f + ca5b9b6
commit ad56599
Show file tree

Hide file tree

Showing 2 changed files with 106 additions and 25 deletions.
diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java
@@ -1783,13 +1783,13 @@ protected final void execute(@Nonnull RunExecution job) {
                 state = State.POST_PRODUCTION;
 
                 if (listener != null) {
+                    RunListener.fireCompleted(this,listener);
                     try {
                         job.cleanUp(listener);
                     } catch (Exception e) {
                         handleFatalBuildProblem(listener,e);
                         // too late to update the result now
                     }
-                    RunListener.fireCompleted(this,listener);
                     listener.finished(result);
                     listener.closeQuietly();
                 }

diff --git a/test/src/test/java/hudson/tasks/BuildTriggerTest.java b/test/src/test/java/hudson/tasks/BuildTriggerTest.java
@@ -24,24 +24,23 @@
 package hudson.tasks;
 
 import static org.junit.Assert.*;
-
-import com.gargoylesoftware.htmlunit.html.HtmlForm;
-import com.gargoylesoftware.htmlunit.html.HtmlPage;
-
 import hudson.Launcher;
 import hudson.maven.MavenModuleSet;
 import hudson.maven.MavenModuleSetBuild;
 import hudson.model.AbstractBuild;
 import hudson.model.AbstractProject;
+import hudson.model.Action;
 import hudson.model.BuildListener;
 import hudson.model.Cause;
 import hudson.model.Computer;
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
+import hudson.model.DependencyGraph;
+import hudson.model.DependencyGraph.Dependency;
 import hudson.model.Item;
 import hudson.model.Result;
-import hudson.model.PermalinkProjectAction.Permalink;
 import hudson.model.Run;
+import hudson.model.TaskListener;
 import hudson.model.User;
 import hudson.security.ACL;
 import hudson.security.AuthorizationMatrixProperty;
@@ -56,6 +55,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.List;
 
 import javax.annotation.CheckForNull;
 
@@ -72,6 +72,7 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.ExtractResourceSCM;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.JenkinsRule.WebClient;
 import org.jvnet.hudson.test.TestBuilder;
@@ -120,10 +121,11 @@ private void assertNoDownstreamBuild(FreeStyleProject dp, Run<?,?> b) throws Exc
         }
     }
 
-    private void assertDownstreamBuild(FreeStyleProject dp, Run<?,?> b) throws Exception {
+    private FreeStyleBuild assertDownstreamBuild(FreeStyleProject dp, Run<?,?> b) throws Exception {
         // Wait for downstream build
         for (int i = 0; dp.getLastBuild()==null && i < 20; i++) Thread.sleep(100);
         assertNotNull("downstream build didn't run.. upstream log: " + b.getLog(), dp.getLastBuild());
+        return dp.getLastBuild();
     }
 
     @Test
@@ -300,9 +302,9 @@ private void assertDoCheck(Authentication auth, @CheckForNull String expectedErr
         }
     }
 
-    @Test
+    @Test @Issue("JENKINS-20989")
     public void downstreamProjectShouldObserveCompletedParent() throws Exception {
-        final WebClient wc = j.createWebClient();
+        j.jenkins.setNumExecutors(2);
 
         final FreeStyleProject us = j.createFreeStyleProject();
         us.getPublishersList().add(new BuildTrigger("downstream", true));
@@ -315,22 +317,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         });
 
         FreeStyleProject ds = createDownstreamProject();
-        ds.getBuildersList().add(new TestBuilder() {
-            @Override
-            public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
-                // Parent should be completed by now
-                FreeStyleBuild upstream = us.getLastBuild();
-                assertNotNull(upstream);
-                assertEquals(1, upstream.getNumber());
-
-                try {
-                    wc.getPage(us, "lastBuild");
-                } catch (SAXException ex) {
-                    throw new AssertionError(ex);
-                }
-                return true;
-            }
-        });
+        ds.getBuildersList().add(new AssertTriggerBuildCompleted(us, j.createWebClient()));
 
         j.jenkins.rebuildDependencyGraph();
 
@@ -340,4 +327,98 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         j.waitForCompletion(dsb);
         j.assertBuildStatusSuccess(dsb);
     }
+
+    @Test @Issue("JENKINS-20989")
+    public void allDownstreamProjectsShouldObserveCompletedParent() throws Exception {
+        j.jenkins.setNumExecutors(3);
+
+        final FreeStyleProject us = j.createFreeStyleProject();
+        us.getPublishersList().add(new SlowTrigger("downstream,downstream2"));
+
+        FreeStyleProject ds = createDownstreamProject();
+        ds.getBuildersList().add(new AssertTriggerBuildCompleted(us, j.createWebClient()));
+        FreeStyleProject ds2 = j.createFreeStyleProject("downstream2");
+        ds2.setQuietPeriod(0);
+        ds2.getBuildersList().add(new AssertTriggerBuildCompleted(us, j.createWebClient()));
+
+        j.jenkins.rebuildDependencyGraph();
+
+        FreeStyleBuild upstream = j.buildAndAssertSuccess(us);
+
+        FreeStyleBuild dsb = assertDownstreamBuild(ds, upstream);
+        j.waitForCompletion(dsb);
+        j.assertBuildStatusSuccess(dsb);
+
+        dsb = assertDownstreamBuild(ds2, upstream);
+        j.waitForCompletion(dsb);
+        j.assertBuildStatusSuccess(dsb);
+    }
+
+    // Trigger that goes through dependencies very slowly
+    private static final class SlowTrigger extends BuildTrigger {
+
+        private static final class Dep extends Dependency {
+            private static boolean block = false;
+            private Dep(AbstractProject upstream, AbstractProject downstream) {
+                super(upstream, downstream);
+            }
+
+            @Override
+            public boolean shouldTriggerBuild(AbstractBuild build, TaskListener listener, List<Action> actions) {
+                if (block) {
+                    try {
+                        Thread.sleep(5000);
+                    } catch (InterruptedException ex) {
+                        throw new AssertionError(ex);
+                    }
+                }
+                block = true;
+                final boolean should = super.shouldTriggerBuild(build, listener, actions);
+                return should;
+            }
+        }
+
+        public SlowTrigger(String childProjects) {
+            super(childProjects, true);
+        }
+
+        @Override @SuppressWarnings("rawtypes")
+        public void buildDependencyGraph(AbstractProject owner, DependencyGraph graph) {
+            for (AbstractProject ch: getChildProjects(owner)) {
+                graph.addDependency(new Dep(owner, ch));
+            }
+        }
+    }
+
+    // Fail downstream build if upstream is not completed yet
+    private static final class AssertTriggerBuildCompleted extends TestBuilder {
+        private final FreeStyleProject us;
+        private final WebClient wc;
+
+        private AssertTriggerBuildCompleted(FreeStyleProject us, WebClient wc) {
+            this.us = us;
+            this.wc = wc;
+        }
+
+        @Override
+        public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
+            FreeStyleBuild success = us.getLastSuccessfulBuild();
+            FreeStyleBuild last = us.getLastBuild();
+            try {
+                assertFalse("Upstream build is not completed after downstream started", last.isBuilding());
+                assertNotNull("Upstream build permalink not correctly updated", success);
+                assertEquals(1, success.getNumber());
+            } catch (AssertionError ex) {
+                System.err.println("Upstream build log: " + last.getLog());
+                throw ex;
+            }
+
+            try {
+                wc.getPage(us, "lastSuccessfulBuild");
+            } catch (SAXException ex) {
+                throw new AssertionError(ex);
+            }
+            return true;
+        }
+    }
 }