-
-
Notifications
You must be signed in to change notification settings - Fork 107
Update plugin parent POM and forward compatibility for tests #200
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
Conversation
basil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmMeessen This PR supersedes #199, #195, and #184. See the PR description for the motivation and my self-review for an explanation of each change. A timely merge and release of this PR would enable me to move forward with Java 17 compatibility testing in jenkinsci/bom#1097 and, ultimately, moving jenkinsci/bom to JENKINS-45047 in another future PR.
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>plugin</artifactId> | ||
| <version>4.38</version> | ||
| <version>4.40</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to allow testing with Java 17 in jenkinsci/bom.
| <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo> | ||
| <maven.javadoc.skip>true</maven.javadoc.skip> | ||
| <jenkins.version>2.303.3</jenkins.version> | ||
| <java.level>8</java.level> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of the update to parent POM 4.40, java.level is deprecated and inferred from the Jenkins core version. So this is no longer needed. I am deleting it to silence the deprecation warning.
| <revision>3.10.0</revision> | ||
| <changelist>-SNAPSHOT</changelist> | ||
| <gitHubRepo>jenkinsci/config-file-provider-plugin</gitHubRepo> | ||
| <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was here, improving this bit of logic to be consistent with the canonical format in jenkinsci/archetypes.
| <gitHubRepo>jenkinsci/config-file-provider-plugin</gitHubRepo> | ||
| <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo> | ||
| <maven.javadoc.skip>true</maven.javadoc.skip> | ||
| <jenkins.version>2.303.3</jenkins.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we are not bumping the baseline here. This is to support my efforts to switch jenkinsci/bom to JENKINS-45047. This also is friendlier to users by allowing new releases to be adopted by older consumers.
|
|
||
| HtmlPage configFiles = wc.goTo("configfiles"); | ||
| HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@onclick, 'removeConfig?id=" + CONFIG_ID + "')]"); | ||
| String attribute = j.jenkins.getVersion().isOlderThan(new VersionNumber("2.324")) ? "onclick" : "data-url"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and elsewhere, we make the test compatible with both old and new versions of Jenkins by choosing the correct attribute based on the Jenkins version of the code under test.
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>email-ext</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for the email-ext PCT fix (see below).
| Assert.assertEquals(1, getProvider(JellyTemplateConfig.JellyTemplateConfigProvider.class).getAllConfigs().size()); | ||
|
|
||
| Assert.assertEquals(5, GlobalConfigFiles.get().getConfigs().size()); | ||
| Assert.assertEquals(6, GlobalConfigFiles.get().getConfigs().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing in PCT with email-ext on the classpath, this test fails with:
[ERROR] org.jenkinsci.lib.configprovider.SystemConfigFilesManagementTest.testLoadAndMergeOldData Time elapsed: 1.824 s <<< FAILURE!
java.lang.AssertionError: expected:<5> but was:<6>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.junit.Assert.assertEquals(Assert.java:633)
at org.jenkinsci.lib.configprovider.SystemConfigFilesManagementTest.testLoadAndMergeOldData(SystemConfigFilesManagementTest.java:49)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:606)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.lang.Thread.run(Thread.java:833)
Debugging this, I see that the presence of email-ext adds JellyTemplateConfig to the list of global configuration files. This test appears to have been designed for this scenario (see the commented line above) but it was never implemented, presumably due to the lack (at that time) of an easy way to add email-ext via jenkinsci/bom. Now that this is a solved problem, I can add email-ext to the classpath, with its version managed by jenkinsci/bom, and uncomment the above line to get this test to pass with email-ext. And this also enables PCT testing with the full managed set of the 100 plugins in the OSS BOM.
|
Thanks @jmMeessen! I have tested this change on the |
|
@alecharp Are you interested in doing a release of this PR? As mentioned in the previous comment, this would help me move ahead with jenkinsci/bom#1097. |
The underlying motivation for the parent POM update is that the new parent POM supports Java 17 and I want to do Java 17 testing of this plugin in
jenkinsci/bom.The underlying compatibility for the forward compatibility for tests is to support my ongoing efforts to switch plugin compatibility testing in
jenkinsci/bomto use JENKINS-45047.This change supersedes #184 (thank you @mikecirioli for the UI updates!). It contains the UI portions of that change without the controversial Jenkins baseline bump. The 4.40 parent POM update is unique to this PR.