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

[JENKINS-24064] No more war-for-test #2634

Closed
wants to merge 5 commits into from

Conversation

6 participants
@jglick
Copy link
Member

commented Nov 14, 2016

JENKINS-24064

Requires some follow-up PRs for use from plugins.

@reviewbybees

Since jenkins.war is not a classpath dependency, and tests need acces…
…s to *-core-assets.jar, better to just make them be dependencies of jenkins-core.jar.
@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2016

rememberMeAutoLoginFailure(hudson.security.TokenBasedRememberMeServices2Test)  Time elapsed: 10.271 sec  <<< FAILURE!
Assertion failed: 

assert logs.find { it.message.contains("contained username 'alice' but was not found")}!=null
       |    |                                                                          |
       []   null                                                                       false
@rsandell

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

🐝

slf4j-jdk14 must also be treated as a dependency of core.
Noticed due to TokenBasedRememberMeServices2Test, which otherwise fails because TokenBasedRememberMeServices.logger does not work.
@daniel-beck

This comment has been minimized.

Copy link
Member

commented Nov 15, 2016

Will this new dependency be newly added to plugin class paths?

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

Will this new dependency be newly added to plugin class paths?

Yes, see jenkinsci/jenkins-test-harness#43 and jenkinsci/plugin-pom#40.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

Random-looking failure in Security232Test.commonsCollections1 on one system only.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

The main problem is that it is tricky to track which runtime dependencies are actually also needed by functional tests. Perhaps there needs to be another module in this repository which has jar packaging, no sources, and merely serves to depend on jenkins-core plus all the various modules, assets, and runtime libraries. Then jenkins-war would depend on this (dumping all those dependencies in WEB-INF/lib/*.jar, I hope), and tests would depend on it as well, giving effectively the same behavior as a war-for-test dep.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

Given the fun we already have with core dependencies exposed to plugins, I'm not sure we want to do more of that if we have an alternative.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2016

another module in this repository

Scratch that idea—it would not allow the plugin parent POM to pick a dependency on core in a way that would work for existing releases. We would need some Gradle-like

if (new VersionNumber(props['jenkins.version']) >= new VersionNumber('2.34')) {
  test "org.jenkins-ci.main:jenkins-war-deps:${props['jenkins.version']}"
} else {
  test "org.jenkins-ci.main:jenkins-war:war-for-test:${props['jenkins.version']}"
}

Given the fun we already have with core dependencies exposed to plugins, I'm not sure we want to do more of that if we have an alternative.

Not sure what you are getting at. Plugins already pick up transitive dependencies from jenkins-war. The current PR actually removes some of those.

Anyway I think the “fun” you refer to involves runtime dependencies. Here we are only talking about test-scoped dependencies.

@reviewbybees

This comment has been minimized.

Copy link

commented Nov 21, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@stephenc
Copy link
Member

left a comment

First round of review looks ok

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

Not sure what you are getting at. Plugins already pick up transitive dependencies from jenkins-war. The current PR actually removes some of those. … Here we are only talking about test-scoped dependencies.

How will this impact the JTH (not ATH)? Do we need guidance for plugin developers for when they upgrade the Jenkins version they develop against, e.g. will they need to add some test dependencies explicitly, or will things break in other ways?

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2016

How will this impact the JTH (not ATH)? Do we need guidance for plugin developers for when they upgrade the Jenkins version they develop against

They just need to use a parent POM including jenkinsci/plugin-pom#40; and either specify no particular jenkins-test-harness.version, or specify one which includes jenkinsci/jenkins-test-harness#43.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2016

@stephenc suggested an alternative approach in jenkinsci/plugin-pom#40, TBD.

@jglick

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

Not using this approach.

@jglick jglick closed this May 24, 2017

@jglick jglick deleted the jglick:war-for-test-JENKINS-24064 branch May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.