Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move tests that depend on MavenModuleSet and MavenModuleSetBuild from Jenkins core to Maven Integration #155

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

basil
Copy link
Member

@basil basil commented Jan 8, 2021

Background

See jenkinsci/jenkins#5118 (comment). The tests in Jenkins core depend on an old version of maven-plugin. This pollutes the dependency tree for the tests in Jenkins core, causing RequireUpperBoundDeps issues and complicating maintenance.

Problem

A number of tests in Jenkins core depend on hudson.maven.MavenModuleSet or hudson.maven.MavenModuleSetBuild. These classes were previously in Jenkins core before they were extracted to maven-plugin. However, the corresponding tests were never moved to maven-plugin. As a result, the tests are no longer in the same repository as the code under test.

Solution

Move the tests in Jenkins core that depend on hudson.maven.MavenModuleSet or hudson.maven.MavenModuleSetBuild to maven-plugin.

Implementation

A handful of these tests were already disabled for several years; we do not make any effort to revive these or move them. Rather we only move the tests that are still running regularly. Below is a detailed description of the change:

  • PluginManagerTest#optionalMavenDependency: This test relies on hudson.maven.agent.AbortException with a comment stating: "this actually doesn't really test what we need." The test seemed pointless, so we deleted it.
  • Operation2174Test#testBuildChains: Moved to maven-plugin.
  • ListJobsCommandTest#getAllJobsFromFolderWithMavenModuleSet: Moved to maven-plugin.
  • GetEnvironmentOutsideBuildTest#testMaven: The entire test class is disabled with a comment stating: "It's unfortunately not working, yet, as whenJenkinsMasterHasNoExecutors is not working as expected." The test seemed pointless, so we deleted it.
  • HelpLinkTest#mavenConfig: The entire test class is disabled with a comment stating: "Excluding test to be able to ship 2.0 beta 1. Jenkins confirms that this test is now taking 45mins to complete." The test seemed pointless, so we deleted it.
  • JobPropertyTest#jobPropertySummaryIsShownInMavenModuleSetIndexPage: Moved to maven-plugin.
  • NodeTest#testGetAssignedLabelWithJobs, NodeTest#testGetAssignedLabelMultipleSlaves, and NodeTest#testGetAssignedLabelWhenLabelRemoveFromProject: Moved to maven-plugin.
  • BuildTriggerTest#mavenBuildTrigger and BuildTriggerTest#mavenTriggerEvenWhenUnstable: These tests are disabled with a comment stating: "Fails on CI due to maven trying to download from maven central on http, which is no longer supported". The tests seemed pointless, so we deleted them.
  • ToolLocationNodePropertyTest#maven: Moved to maven-plugin.
  • ToolLocationNodePropertyTest#nativeMaven: This test is disabled with a comment stating: "Fails on CI due to maven trying to download from maven central on http, which is no longer supported". The test seemed pointless, so we deleted it.
  • JenkinsBuildsAndWorkspacesDirectoriesTest#testItemFullNameExpansion: Moved to maven-plugin.
  • SimpleBuildWrapperTest#disposerWithMaven: Moved to maven-plugin.
  • ListScmBrowsersTest#selectBoxesUnique_MavenProject: Moved to maven-plugin.

@jglick
Copy link
Member

jglick commented Jan 11, 2021

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on!

src/test/java/jenkins/tasks/SimpleBuildWrapperTest.java Outdated Show resolved Hide resolved
src/test/java/jenkins/tasks/SimpleBuildWrapperTest.java Outdated Show resolved Hide resolved
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, assuming tests pass!

@olamy olamy closed this Jan 17, 2021
@olamy olamy reopened this Jan 17, 2021
@olamy olamy merged commit 51afa3c into jenkinsci:master Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants