From 2e6fbe0ae343c8d95f4ffa4b9827925a5fa05ade Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Fri, 7 Jul 2023 20:58:43 -0600 Subject: [PATCH 1/7] Fix random GitStatusTest failures Test already had code that waits for jobs to complete on Windows. The improved job leak detection that was implemented in the Jenkins test harness will sometimes show the same conditions on Unix builds. Wait for jobs to complete on Windows and on Unix. --- src/test/java/hudson/plugins/git/GitStatusTest.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitStatusTest.java b/src/test/java/hudson/plugins/git/GitStatusTest.java index 7be2eaf5c2..63b9166609 100644 --- a/src/test/java/hudson/plugins/git/GitStatusTest.java +++ b/src/test/java/hudson/plugins/git/GitStatusTest.java @@ -72,9 +72,8 @@ public void resetAllowNotifyCommitParameters() throws Exception { @After public void waitForAllJobsToComplete() throws Exception { - // Put JenkinsRule into shutdown state, trying to reduce Windows cleanup exceptions - if (r != null && r.jenkins != null) { - r.jenkins.doQuietDown(); + if (r == null || r.jenkins == null) { + return; // No jobs if not running with a JenkinsRule } // JenkinsRule cleanup throws exceptions during tearDown. // Reduce exceptions by a random delay from 0.5 to 0.9 seconds. @@ -82,14 +81,12 @@ public void waitForAllJobsToComplete() throws Exception { // for fewer exceptions and for better Windows job cleanup. java.util.Random random = new java.util.Random(); Thread.sleep(500L + random.nextInt(400)); + /* Windows job cleanup fails to delete build logs in some of these tests. * Wait for the jobs to complete before exiting the test so that the * build logs will not be active when the cleanup process tries to * delete them. */ - if (!isWindows() || r == null || r.jenkins == null) { - return; - } View allView = r.jenkins.getView("All"); if (allView == null) { return; From 16a359755a978794219c1859992c29f0b0b1ea60 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Fri, 7 Jul 2023 21:50:21 -0600 Subject: [PATCH 2/7] Remove non JenkinsRule tests to simplify cleanup --- .../hudson/plugins/git/GitStatusTest.java | 79 +------------------ 1 file changed, 3 insertions(+), 76 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitStatusTest.java b/src/test/java/hudson/plugins/git/GitStatusTest.java index 63b9166609..4dc04ace87 100644 --- a/src/test/java/hudson/plugins/git/GitStatusTest.java +++ b/src/test/java/hudson/plugins/git/GitStatusTest.java @@ -32,7 +32,6 @@ import org.junit.Before; import org.junit.Test; import org.jvnet.hudson.test.Issue; -import org.jvnet.hudson.test.WithoutJenkins; import javax.servlet.http.HttpServletRequest; import org.kohsuke.stapler.HttpResponse; @@ -72,16 +71,14 @@ public void resetAllowNotifyCommitParameters() throws Exception { @After public void waitForAllJobsToComplete() throws Exception { - if (r == null || r.jenkins == null) { - return; // No jobs if not running with a JenkinsRule - } + // Put JenkinsRule into shutdown state, trying to reduce cleanup exceptions + r.jenkins.doQuietDown(); // JenkinsRule cleanup throws exceptions during tearDown. // Reduce exceptions by a random delay from 0.5 to 0.9 seconds. // Adding roughly 0.7 seconds to these JenkinsRule tests is a small price // for fewer exceptions and for better Windows job cleanup. java.util.Random random = new java.util.Random(); Thread.sleep(500L + random.nextInt(400)); - /* Windows job cleanup fails to delete build logs in some of these tests. * Wait for the jobs to complete before exiting the test so that the * build logs will not be active when the cleanup process tries to @@ -89,6 +86,7 @@ public void waitForAllJobsToComplete() throws Exception { */ View allView = r.jenkins.getView("All"); if (allView == null) { + assertTrue(false); // unexpected return; } RunList runList = allView.getBuilds(); @@ -105,36 +103,6 @@ public void waitForAllJobsToComplete() throws Exception { }); } - @WithoutJenkins - @Test - public void testGetDisplayName() { - assertEquals("Git", this.gitStatus.getDisplayName()); - } - - @WithoutJenkins - @Test - public void testGetIconFileName() { - assertNull(this.gitStatus.getIconFileName()); - } - - @WithoutJenkins - @Test - public void testGetUrlName() { - assertEquals("git", this.gitStatus.getUrlName()); - } - - @WithoutJenkins - @Test - public void testAllowNotifyCommitParametersDisabled() { - assertFalse("SECURITY-275: ignore arbitrary notifyCommit parameters", GitStatus.ALLOW_NOTIFY_COMMIT_PARAMETERS); - } - - @WithoutJenkins - @Test - public void testSafeParametersEmpty() { - assertEquals("SECURITY-275: Safe notifyCommit parameters", "", GitStatus.SAFE_PARAMETERS); - } - @Test public void testDoNotifyCommitWithNoBranches() throws Exception { SCMTrigger aMasterTrigger = setupProjectWithTrigger("a", "master", false); @@ -368,47 +336,6 @@ private void setupProject(String url, String branchString, SCMTrigger trigger) t if (trigger != null) project.addTrigger(trigger); } - @WithoutJenkins - @Test - public void testLooselyMatches() throws URISyntaxException { - String[] equivalentRepoURLs = new String[]{ - "https://example.com/jenkinsci/git-plugin", - "https://example.com/jenkinsci/git-plugin/", - "https://example.com/jenkinsci/git-plugin.git", - "https://example.com/jenkinsci/git-plugin.git/", - "https://someone@example.com/jenkinsci/git-plugin.git", - "https://someone:somepassword@example.com/jenkinsci/git-plugin/", - "git://example.com/jenkinsci/git-plugin", - "git://example.com/jenkinsci/git-plugin/", - "git://example.com/jenkinsci/git-plugin.git", - "git://example.com/jenkinsci/git-plugin.git/", - "ssh://git@example.com/jenkinsci/git-plugin", - "ssh://example.com/jenkinsci/git-plugin.git", - "git@example.com:jenkinsci/git-plugin/", - "git@example.com:jenkinsci/git-plugin.git", - "git@example.com:jenkinsci/git-plugin.git/" - }; - List uris = new ArrayList<>(); - for (String testURL : equivalentRepoURLs) { - uris.add(new URIish(testURL)); - } - - /* Extra slashes on end of URL probably should be considered equivalent, - * but current implementation does not consider them as loose matches - */ - URIish badURLTrailingSlashes = new URIish(equivalentRepoURLs[0] + "///"); - /* Different hostname should always fail match check */ - URIish badURLHostname = new URIish(equivalentRepoURLs[0].replace("example.com", "bitbucket.org")); - - for (URIish lhs : uris) { - assertFalse(lhs + " matches trailing slashes " + badURLTrailingSlashes, GitStatus.looselyMatches(lhs, badURLTrailingSlashes)); - assertFalse(lhs + " matches bad hostname " + badURLHostname, GitStatus.looselyMatches(lhs, badURLHostname)); - for (URIish rhs : uris) { - assertTrue(lhs + " and " + rhs + " didn't match", GitStatus.looselyMatches(lhs, rhs)); - } - } - } - private FreeStyleProject setupNotifyProject() throws Exception { FreeStyleProject project = r.createFreeStyleProject(); project.setQuietPeriod(0); From 85e1bdf323b712a489702997b577bd468299ac23 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 8 Jul 2023 04:16:42 -0600 Subject: [PATCH 3/7] Move simple GitStatusTest items to new class No need to complicate GitStatusTest with the test methods that do not require a JenkinsRule. Use a simpler test structure and a simpler test file for the GitStatusTest assertions that do not require a JenkinsRule. --- .../plugins/git/GitStatusSimpleTest.java | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 src/test/java/hudson/plugins/git/GitStatusSimpleTest.java diff --git a/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java b/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java new file mode 100644 index 0000000000..27fe8ceff1 --- /dev/null +++ b/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java @@ -0,0 +1,88 @@ +package hudson.plugins.git; + +import static org.junit.Assert.*; + +import java.net.URISyntaxException; +import org.eclipse.jgit.transport.URIish; +import java.util.ArrayList; +import java.util.List; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +public class GitStatusSimpleTest { + + private GitStatus gitStatus; + + @Before + public void setUp() throws Exception { + this.gitStatus = new GitStatus(); + } + + @Test + public void testGetDisplayName() { + assertEquals("Git", this.gitStatus.getDisplayName()); + } + + @Test + public void testGetIconFileName() { + assertNull(this.gitStatus.getIconFileName()); + } + + @Test + public void testGetUrlName() { + assertEquals("git", this.gitStatus.getUrlName()); + } + + @Test + public void testAllowNotifyCommitParametersDisabled() { + assertFalse("SECURITY-275: ignore arbitrary notifyCommit parameters", GitStatus.ALLOW_NOTIFY_COMMIT_PARAMETERS); + } + + @Test + public void testSafeParametersEmpty() { + assertEquals("SECURITY-275: Safe notifyCommit parameters", "", GitStatus.SAFE_PARAMETERS); + } + + @Test + public void testLooselyMatches() throws URISyntaxException { + String[] equivalentRepoURLs = new String[] { + "https://example.com/jenkinsci/git-plugin", + "https://example.com/jenkinsci/git-plugin/", + "https://example.com/jenkinsci/git-plugin.git", + "https://example.com/jenkinsci/git-plugin.git/", + "https://someone@example.com/jenkinsci/git-plugin.git", + "https://someone:somepassword@example.com/jenkinsci/git-plugin/", + "git://example.com/jenkinsci/git-plugin", + "git://example.com/jenkinsci/git-plugin/", + "git://example.com/jenkinsci/git-plugin.git", + "git://example.com/jenkinsci/git-plugin.git/", + "ssh://git@example.com/jenkinsci/git-plugin", + "ssh://example.com/jenkinsci/git-plugin.git", + "git@example.com:jenkinsci/git-plugin/", + "git@example.com:jenkinsci/git-plugin.git", + "git@example.com:jenkinsci/git-plugin.git/" + }; + List uris = new ArrayList<>(); + for (String testURL : equivalentRepoURLs) { + uris.add(new URIish(testURL)); + } + + /* Extra slashes on end of URL probably should be considered equivalent, + * but current implementation does not consider them as loose matches + */ + URIish badURLTrailingSlashes = new URIish(equivalentRepoURLs[0] + "///"); + /* Different hostname should always fail match check */ + URIish badURLHostname = new URIish(equivalentRepoURLs[0].replace("example.com", "bitbucket.org")); + + for (URIish lhs : uris) { + assertFalse( + lhs + " matches trailing slashes " + badURLTrailingSlashes, + GitStatus.looselyMatches(lhs, badURLTrailingSlashes)); + assertFalse(lhs + " matches bad hostname " + badURLHostname, GitStatus.looselyMatches(lhs, badURLHostname)); + for (URIish rhs : uris) { + assertTrue(lhs + " and " + rhs + " didn't match", GitStatus.looselyMatches(lhs, rhs)); + } + } + } +} From b6b385197d5d2743e6ef42423d8f5990fe638607 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 8 Jul 2023 05:05:53 -0600 Subject: [PATCH 4/7] Assert that the "all" view must exist --- src/test/java/hudson/plugins/git/GitStatusTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/hudson/plugins/git/GitStatusTest.java b/src/test/java/hudson/plugins/git/GitStatusTest.java index 4dc04ace87..35997df6c3 100644 --- a/src/test/java/hudson/plugins/git/GitStatusTest.java +++ b/src/test/java/hudson/plugins/git/GitStatusTest.java @@ -86,7 +86,7 @@ public void waitForAllJobsToComplete() throws Exception { */ View allView = r.jenkins.getView("All"); if (allView == null) { - assertTrue(false); // unexpected + fail("All view was not found when it should always be available"); return; } RunList runList = allView.getBuilds(); From 5e398aca18cab26d907703bb73d40df59318a6f7 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 8 Jul 2023 05:06:09 -0600 Subject: [PATCH 5/7] Report a message if no jobs found for wait loop Not an error, but surprising based on my testing. None of my test machines are fast enough to complete all the jobs without needing any time for waiting. --- src/test/java/hudson/plugins/git/GitStatusTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/hudson/plugins/git/GitStatusTest.java b/src/test/java/hudson/plugins/git/GitStatusTest.java index 35997df6c3..74561ba0ce 100644 --- a/src/test/java/hudson/plugins/git/GitStatusTest.java +++ b/src/test/java/hudson/plugins/git/GitStatusTest.java @@ -91,6 +91,7 @@ public void waitForAllJobsToComplete() throws Exception { } RunList runList = allView.getBuilds(); if (runList == null) { + Logger.getLogger(GitStatusTest.class.getName()).log(Level.INFO, "No waiting, no entries in the runList"); return; } runList.forEach((Run run) -> { From 37788ba14e66fe10ab25921bc3ac59569adb4d21 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 8 Jul 2023 05:35:43 -0600 Subject: [PATCH 6/7] Replace Hudson with Jenkins in comments --- src/main/java/hudson/plugins/git/GitChangeSet.java | 2 +- src/main/java/hudson/plugins/git/GitSCM.java | 2 +- src/main/java/hudson/plugins/git/GitStatus.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/hudson/plugins/git/GitChangeSet.java b/src/main/java/hudson/plugins/git/GitChangeSet.java index 710d17a8c7..ecb542446b 100644 --- a/src/main/java/hudson/plugins/git/GitChangeSet.java +++ b/src/main/java/hudson/plugins/git/GitChangeSet.java @@ -590,7 +590,7 @@ public User getAuthor() { /** * Gets the author name for this changeset - note that this is mainly here * so that we can test authorOrCommitter without needing a fully instantiated - * Hudson (which is needed for User.get in getAuthor()). + * Jenkins (which is needed for User.get in getAuthor()). * * @return author name */ diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index 410961919e..a26f51363b 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -696,7 +696,7 @@ public PollingResult compareRemoteRevisionWith(Job project, Launcher launc public static final Pattern GIT_REF = Pattern.compile("^(refs/[^/]+)/(.+)"); private PollingResult compareRemoteRevisionWithImpl(Job project, Launcher launcher, FilePath workspace, final @NonNull TaskListener listener) throws IOException, InterruptedException { - // Poll for changes. Are there any unbuilt revisions that Hudson ought to build ? + // Poll for changes. Are there any unbuilt revisions that Jenkins ought to build ? listener.getLogger().println("Using strategy: " + getBuildChooser().getDisplayName()); diff --git a/src/main/java/hudson/plugins/git/GitStatus.java b/src/main/java/hudson/plugins/git/GitStatus.java index ef948f0a26..29e3b824cb 100644 --- a/src/main/java/hudson/plugins/git/GitStatus.java +++ b/src/main/java/hudson/plugins/git/GitStatus.java @@ -34,7 +34,7 @@ import org.kohsuke.stapler.*; /** - * Information screen for the use of Git in Hudson. + * Root action that requests the plugin to poll for changes in remote repositories. */ @Extension public class GitStatus implements UnprotectedRootAction { From 01dd5feef0d1ed0dfac3226fa8c2ee76d11b8c5f Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 8 Jul 2023 05:36:03 -0600 Subject: [PATCH 7/7] Remove unused imports --- src/test/java/hudson/plugins/git/GitStatusSimpleTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java b/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java index 27fe8ceff1..298490ea05 100644 --- a/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java +++ b/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java @@ -2,11 +2,9 @@ import static org.junit.Assert.*; -import java.net.URISyntaxException; import org.eclipse.jgit.transport.URIish; import java.util.ArrayList; import java.util.List; -import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -45,7 +43,7 @@ public void testSafeParametersEmpty() { } @Test - public void testLooselyMatches() throws URISyntaxException { + public void testLooselyMatches() throws Exception { String[] equivalentRepoURLs = new String[] { "https://example.com/jenkinsci/git-plugin", "https://example.com/jenkinsci/git-plugin/",