From 18b9a4422ba5d345d1d3750974b1ad3b0680389c Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 16 Jun 2023 14:09:23 -0400 Subject: [PATCH 1/3] [JENKINS-71406] Flake in `ExecutorTest.apiPermissions` caused by builds left running --- test/src/test/java/hudson/model/ExecutorTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/src/test/java/hudson/model/ExecutorTest.java b/test/src/test/java/hudson/model/ExecutorTest.java index 0f83110f52e9..eab3022e316e 100644 --- a/test/src/test/java/hudson/model/ExecutorTest.java +++ b/test/src/test/java/hudson/model/ExecutorTest.java @@ -21,6 +21,7 @@ import jenkins.model.CauseOfInterruption.UserInterruption; import jenkins.model.InterruptedBuildAction; import jenkins.model.Jenkins; +import org.junit.After; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; @@ -34,6 +35,20 @@ public class ExecutorTest { @Rule public JenkinsRule j = new JenkinsRule(); + @After + public void stopRunningBuilds() throws InterruptedException { + for (Job job : j.jenkins.allItems(Job.class)) { + for (Run build : job.getBuilds()) { + Executor executor = build.getExecutor(); + if (executor != null) { + System.out.println("Stopping " + build); + executor.interrupt(); + j.waitForCompletion(build); + } + } + } + } + @Test @Issue("JENKINS-4756") public void whenAnExecutorDiesHardANewExecutorTakesItsPlace() throws Exception { From f2b481b254815377766e521e9487b24e451f526c Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 16 Jun 2023 14:49:21 -0400 Subject: [PATCH 2/3] `ComputerTest` & `ComputerSetTest` also use `startBlockingBuild` --- test/src/test/java/hudson/model/ComputerSetTest.java | 6 ++++++ test/src/test/java/hudson/model/ComputerTest.java | 6 ++++++ test/src/test/java/hudson/model/ExecutorTest.java | 8 ++++++-- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/test/src/test/java/hudson/model/ComputerSetTest.java b/test/src/test/java/hudson/model/ComputerSetTest.java index 703376b19f08..9c51afef1921 100644 --- a/test/src/test/java/hudson/model/ComputerSetTest.java +++ b/test/src/test/java/hudson/model/ComputerSetTest.java @@ -44,6 +44,7 @@ import org.htmlunit.Page; import org.htmlunit.html.HtmlForm; import org.htmlunit.html.HtmlPage; +import org.junit.After; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -59,6 +60,11 @@ public class ComputerSetTest { @Rule public JenkinsRule j = new JenkinsRule(); + @After + public void stopRunningBuilds() throws InterruptedException { + ExecutorTest.stopRunningBuilds(j); + } + @Test @Issue("JENKINS-2821") public void pageRendering() throws Exception { diff --git a/test/src/test/java/hudson/model/ComputerTest.java b/test/src/test/java/hudson/model/ComputerTest.java index 6d5b9a558561..6f8676125771 100644 --- a/test/src/test/java/hudson/model/ComputerTest.java +++ b/test/src/test/java/hudson/model/ComputerTest.java @@ -60,6 +60,7 @@ import org.htmlunit.WebRequest; import org.htmlunit.html.HtmlForm; import org.htmlunit.xml.XmlPage; +import org.junit.After; import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -78,6 +79,11 @@ public class ComputerTest { @Rule public JenkinsRule j = new JenkinsRule(); @Rule public LoggerRule logging = new LoggerRule(); + @After + public void stopRunningBuilds() throws InterruptedException { + ExecutorTest.stopRunningBuilds(j); + } + @Test public void discardLogsAfterDeletion() throws Exception { DumbSlave delete = j.createOnlineSlave(Jenkins.get().getLabelAtom("delete")); diff --git a/test/src/test/java/hudson/model/ExecutorTest.java b/test/src/test/java/hudson/model/ExecutorTest.java index eab3022e316e..81f3501a8ba1 100644 --- a/test/src/test/java/hudson/model/ExecutorTest.java +++ b/test/src/test/java/hudson/model/ExecutorTest.java @@ -37,6 +37,10 @@ public class ExecutorTest { @After public void stopRunningBuilds() throws InterruptedException { + stopRunningBuilds(j); + } + + public static void stopRunningBuilds(JenkinsRule j) throws InterruptedException { for (Job job : j.jenkins.allItems(Job.class)) { for (Run build : job.getBuilds()) { Executor executor = build.getExecutor(); @@ -187,8 +191,8 @@ public void disconnectCause_WithoutTrace() throws Exception { } /** - * Start a project with an infinite build step - * + * Start a project with an infinite build step. + * You must clean up, for example with {@link #stopRunningBuilds(JenkinsRule)}. * @param project {@link FreeStyleProject} to start * @return A {@link Future} object represents the started build * @throws Exception if somethink wrong happened From 9a9e8dcd00dc3219b8fc897767284d55427d30cf Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 28 Jun 2023 13:58:19 -0400 Subject: [PATCH 3/3] `finally` blocks rather than `@After` https://github.com/jenkinsci/jenkins/pull/8157#pullrequestreview-1484888758 --- .../java/hudson/model/ComputerSetTest.java | 52 +++---- .../test/java/hudson/model/ComputerTest.java | 52 +++---- .../test/java/hudson/model/ExecutorTest.java | 134 ++++++++++-------- 3 files changed, 123 insertions(+), 115 deletions(-) diff --git a/test/src/test/java/hudson/model/ComputerSetTest.java b/test/src/test/java/hudson/model/ComputerSetTest.java index 9c51afef1921..a128c7e5cd51 100644 --- a/test/src/test/java/hudson/model/ComputerSetTest.java +++ b/test/src/test/java/hudson/model/ComputerSetTest.java @@ -44,7 +44,6 @@ import org.htmlunit.Page; import org.htmlunit.html.HtmlForm; import org.htmlunit.html.HtmlPage; -import org.junit.After; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -60,11 +59,6 @@ public class ComputerSetTest { @Rule public JenkinsRule j = new JenkinsRule(); - @After - public void stopRunningBuilds() throws InterruptedException { - ExecutorTest.stopRunningBuilds(j); - } - @Test @Issue("JENKINS-2821") public void pageRendering() throws Exception { @@ -153,16 +147,19 @@ public void testTerminatedNodeStatusPageDoesNotShowTrace() throws Exception { p.setAssignedNode(agent); Future r = ExecutorTest.startBlockingBuild(p); - - String message = "It went away"; - p.getLastBuild().getBuiltOn().toComputer().disconnect( - new OfflineCause.ChannelTermination(new RuntimeException(message)) - ); - - WebClient wc = j.createWebClient(); - Page page = wc.getPage(wc.createCrumbedUrl(agent.toComputer().getUrl())); - String content = page.getWebResponse().getContentAsString(); - assertThat(content, not(containsString(message))); + try { + String message = "It went away"; + p.getLastBuild().getBuiltOn().toComputer().disconnect( + new OfflineCause.ChannelTermination(new RuntimeException(message)) + ); + + WebClient wc = j.createWebClient(); + Page page = wc.getPage(wc.createCrumbedUrl(agent.toComputer().getUrl())); + String content = page.getWebResponse().getContentAsString(); + assertThat(content, not(containsString(message))); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } @Test @@ -173,15 +170,18 @@ public void testTerminatedNodeAjaxExecutorsDoesNotShowTrace() throws Exception { p.setAssignedNode(agent); Future r = ExecutorTest.startBlockingBuild(p); - - String message = "It went away"; - p.getLastBuild().getBuiltOn().toComputer().disconnect( - new OfflineCause.ChannelTermination(new RuntimeException(message)) - ); - - WebClient wc = j.createWebClient(); - Page page = wc.getPage(wc.createCrumbedUrl(HasWidgetHelper.getWidget(j.jenkins.getComputer(), ExecutorsWidget.class).orElseThrow().getUrl() + "ajax")); - String content = page.getWebResponse().getContentAsString(); - assertThat(content, not(containsString(message))); + try { + String message = "It went away"; + p.getLastBuild().getBuiltOn().toComputer().disconnect( + new OfflineCause.ChannelTermination(new RuntimeException(message)) + ); + + WebClient wc = j.createWebClient(); + Page page = wc.getPage(wc.createCrumbedUrl(HasWidgetHelper.getWidget(j.jenkins.getComputer(), ExecutorsWidget.class).orElseThrow().getUrl() + "ajax")); + String content = page.getWebResponse().getContentAsString(); + assertThat(content, not(containsString(message))); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } } diff --git a/test/src/test/java/hudson/model/ComputerTest.java b/test/src/test/java/hudson/model/ComputerTest.java index 6f8676125771..49e451da5e9d 100644 --- a/test/src/test/java/hudson/model/ComputerTest.java +++ b/test/src/test/java/hudson/model/ComputerTest.java @@ -60,7 +60,6 @@ import org.htmlunit.WebRequest; import org.htmlunit.html.HtmlForm; import org.htmlunit.xml.XmlPage; -import org.junit.After; import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -79,11 +78,6 @@ public class ComputerTest { @Rule public JenkinsRule j = new JenkinsRule(); @Rule public LoggerRule logging = new LoggerRule(); - @After - public void stopRunningBuilds() throws InterruptedException { - ExecutorTest.stopRunningBuilds(j); - } - @Test public void discardLogsAfterDeletion() throws Exception { DumbSlave delete = j.createOnlineSlave(Jenkins.get().getLabelAtom("delete")); @@ -235,16 +229,19 @@ public void testTerminatedNodeStatusPageDoesNotShowTrace() throws Exception { p.setAssignedNode(agent); Future r = ExecutorTest.startBlockingBuild(p); - - String message = "It went away"; - p.getLastBuild().getBuiltOn().toComputer().disconnect( - new OfflineCause.ChannelTermination(new RuntimeException(message)) - ); - - WebClient wc = j.createWebClient(); - Page page = wc.getPage(wc.createCrumbedUrl(agent.toComputer().getUrl())); - String content = page.getWebResponse().getContentAsString(); - assertThat(content, not(containsString(message))); + try { + String message = "It went away"; + p.getLastBuild().getBuiltOn().toComputer().disconnect( + new OfflineCause.ChannelTermination(new RuntimeException(message)) + ); + + WebClient wc = j.createWebClient(); + Page page = wc.getPage(wc.createCrumbedUrl(agent.toComputer().getUrl())); + String content = page.getWebResponse().getContentAsString(); + assertThat(content, not(containsString(message))); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } @Test @@ -254,16 +251,19 @@ public void testTerminatedNodeAjaxExecutorsDoesNotShowTrace() throws Exception { p.setAssignedNode(agent); Future r = ExecutorTest.startBlockingBuild(p); - - String message = "It went away"; - p.getLastBuild().getBuiltOn().toComputer().disconnect( - new OfflineCause.ChannelTermination(new RuntimeException(message)) - ); - - WebClient wc = j.createWebClient(); - Page page = wc.getPage(wc.createCrumbedUrl(HasWidgetHelper.getWidget(agent.toComputer(), ExecutorsWidget.class).orElseThrow().getUrl() + "ajax")); - String content = page.getWebResponse().getContentAsString(); - assertThat(content, not(containsString(message))); + try { + String message = "It went away"; + p.getLastBuild().getBuiltOn().toComputer().disconnect( + new OfflineCause.ChannelTermination(new RuntimeException(message)) + ); + + WebClient wc = j.createWebClient(); + Page page = wc.getPage(wc.createCrumbedUrl(HasWidgetHelper.getWidget(agent.toComputer(), ExecutorsWidget.class).orElseThrow().getUrl() + "ajax")); + String content = page.getWebResponse().getContentAsString(); + assertThat(content, not(containsString(message))); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } } diff --git a/test/src/test/java/hudson/model/ExecutorTest.java b/test/src/test/java/hudson/model/ExecutorTest.java index 81f3501a8ba1..b624557ef137 100644 --- a/test/src/test/java/hudson/model/ExecutorTest.java +++ b/test/src/test/java/hudson/model/ExecutorTest.java @@ -21,7 +21,6 @@ import jenkins.model.CauseOfInterruption.UserInterruption; import jenkins.model.InterruptedBuildAction; import jenkins.model.Jenkins; -import org.junit.After; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; @@ -35,11 +34,6 @@ public class ExecutorTest { @Rule public JenkinsRule j = new JenkinsRule(); - @After - public void stopRunningBuilds() throws InterruptedException { - stopRunningBuilds(j); - } - public static void stopRunningBuilds(JenkinsRule j) throws InterruptedException { for (Job job : j.jenkins.allItems(Job.class)) { for (Run build : job.getBuilds()) { @@ -101,22 +95,25 @@ public void abortCause() throws Exception { FreeStyleProject p = j.createFreeStyleProject(); Future r = startBlockingBuild(p); - - User johnny = User.getOrCreateByIdOrFullName("Johnny"); - p.getLastBuild().getExecutor().interrupt(Result.FAILURE, - new UserInterruption(johnny), // test the merge semantics - new UserInterruption(johnny)); - - FreeStyleBuild b = r.get(); - - // make sure this information is recorded - j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b)); - InterruptedBuildAction iba = b.getAction(InterruptedBuildAction.class); - assertEquals(1, iba.getCauses().size()); - assertEquals(((UserInterruption) iba.getCauses().get(0)).getUser(), johnny); - - // make sure it shows up in the log - j.assertLogContains(johnny.getId(), b); + try { + User johnny = User.getOrCreateByIdOrFullName("Johnny"); + p.getLastBuild().getExecutor().interrupt(Result.FAILURE, + new UserInterruption(johnny), // test the merge semantics + new UserInterruption(johnny)); + + FreeStyleBuild b = r.get(); + + // make sure this information is recorded + j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b)); + InterruptedBuildAction iba = b.getAction(InterruptedBuildAction.class); + assertEquals(1, iba.getCauses().size()); + assertEquals(((UserInterruption) iba.getCauses().get(0)).getUser(), johnny); + + // make sure it shows up in the log + j.assertLogContains(johnny.getId(), b); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } @Test @@ -126,19 +123,23 @@ public void disconnectCause() throws Exception { p.setAssignedNode(slave); Future r = startBlockingBuild(p); - User johnny = User.getOrCreateByIdOrFullName("Johnny"); - - p.getLastBuild().getBuiltOn().toComputer().disconnect( - new OfflineCause.UserCause(johnny, "Taking offline to break your build") - ); - - FreeStyleBuild b = r.get(); - - j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b)); - j.assertLogContains("Finished: FAILURE", b); - j.assertLogContains("Build step 'BlockingBuilder' marked build as failure", b); - j.assertLogContains("Agent went offline during the build", b); - j.assertLogContains("Disconnected by Johnny : Taking offline to break your build", b); + try { + User johnny = User.getOrCreateByIdOrFullName("Johnny"); + + p.getLastBuild().getBuiltOn().toComputer().disconnect( + new OfflineCause.UserCause(johnny, "Taking offline to break your build") + ); + + FreeStyleBuild b = r.get(); + + j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b)); + j.assertLogContains("Finished: FAILURE", b); + j.assertLogContains("Build step 'BlockingBuilder' marked build as failure", b); + j.assertLogContains("Agent went offline during the build", b); + j.assertLogContains("Disconnected by Johnny : Taking offline to break your build", b); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } @Issue("SECURITY-611") @@ -150,26 +151,30 @@ public void apiPermissions() throws Exception { FreeStyleProject publicProject = j.createFreeStyleProject("public-project"); publicProject.setAssignedNode(slave); startBlockingBuild(publicProject); - FreeStyleProject secretProject = j.createFreeStyleProject("secret-project"); - secretProject.setAssignedNode(slave); - startBlockingBuild(secretProject); - j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); - j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy(). - grant(Jenkins.READ).everywhere().toEveryone(). - grant(Item.READ).onItems(publicProject).toEveryone(). - grant(Item.READ).onItems(secretProject).to("has-security-clearance")); - - JenkinsRule.WebClient wc = j.createWebClient(); - wc.withBasicCredentials("has-security-clearance"); - String api = wc.goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString(); - System.out.println(api); - assertThat(api, allOf(containsString("public-project"), containsString("secret-project"))); - - wc = j.createWebClient(); - wc.withBasicCredentials("regular-joe"); - api = wc.goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString(); - System.out.println(api); - assertThat(api, allOf(containsString("public-project"), not(containsString("secret-project")))); + try { + FreeStyleProject secretProject = j.createFreeStyleProject("secret-project"); + secretProject.setAssignedNode(slave); + startBlockingBuild(secretProject); + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy(). + grant(Jenkins.READ).everywhere().toEveryone(). + grant(Item.READ).onItems(publicProject).toEveryone(). + grant(Item.READ).onItems(secretProject).to("has-security-clearance")); + + JenkinsRule.WebClient wc = j.createWebClient(); + wc.withBasicCredentials("has-security-clearance"); + String api = wc.goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString(); + System.out.println(api); + assertThat(api, allOf(containsString("public-project"), containsString("secret-project"))); + + wc = j.createWebClient(); + wc.withBasicCredentials("regular-joe"); + api = wc.goTo(slave.toComputer().getUrl() + "api/json?pretty&depth=1", null).getWebResponse().getContentAsString(); + System.out.println(api); + assertThat(api, allOf(containsString("public-project"), not(containsString("secret-project")))); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } @Test @@ -180,14 +185,17 @@ public void disconnectCause_WithoutTrace() throws Exception { p.setAssignedNode(slave); Future r = startBlockingBuild(p); - - String message = "It went away"; - p.getLastBuild().getBuiltOn().toComputer().disconnect( - new OfflineCause.ChannelTermination(new RuntimeException(message)) - ); - - OfflineCause offlineCause = p.getLastBuild().getBuiltOn().toComputer().getOfflineCause(); - Assert.assertThat(offlineCause.toString(), not(containsString(message))); + try { + String message = "It went away"; + p.getLastBuild().getBuiltOn().toComputer().disconnect( + new OfflineCause.ChannelTermination(new RuntimeException(message)) + ); + + OfflineCause offlineCause = p.getLastBuild().getBuiltOn().toComputer().getOfflineCause(); + Assert.assertThat(offlineCause.toString(), not(containsString(message))); + } finally { + ExecutorTest.stopRunningBuilds(j); + } } /**