From fd87a1c03189034238f4230008fcf5168f8db757 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 8 Jul 2023 06:09:04 -0600 Subject: [PATCH] Fix erratic GitStatusTest (#1480) * 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. * Remove non JenkinsRule tests to simplify cleanup * 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. * Assert that the "all" view must exist * 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. * Replace Hudson with Jenkins in comments * Remove unused imports --- .../java/hudson/plugins/git/GitChangeSet.java | 2 +- src/main/java/hudson/plugins/git/GitSCM.java | 2 +- .../java/hudson/plugins/git/GitStatus.java | 2 +- .../plugins/git/GitStatusSimpleTest.java | 86 +++++++++++++++++++ .../hudson/plugins/git/GitStatusTest.java | 83 +----------------- 5 files changed, 93 insertions(+), 82 deletions(-) create mode 100644 src/test/java/hudson/plugins/git/GitStatusSimpleTest.java 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 { 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..298490ea05 --- /dev/null +++ b/src/test/java/hudson/plugins/git/GitStatusSimpleTest.java @@ -0,0 +1,86 @@ +package hudson.plugins.git; + +import static org.junit.Assert.*; + +import org.eclipse.jgit.transport.URIish; +import java.util.ArrayList; +import java.util.List; +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 Exception { + 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)); + } + } + } +} diff --git a/src/test/java/hudson/plugins/git/GitStatusTest.java b/src/test/java/hudson/plugins/git/GitStatusTest.java index 7be2eaf5c2..74561ba0ce 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,10 +71,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(); - } + // 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 @@ -87,15 +84,14 @@ public void waitForAllJobsToComplete() throws Exception { * 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) { + fail("All view was not found when it should always be available"); return; } 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) -> { @@ -108,36 +104,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); @@ -371,47 +337,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);