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

Require a minimum version of the Jenkins Test Harness #489

Closed
wants to merge 5 commits into from

Conversation

basil
Copy link
Member

@basil basil commented Mar 14, 2023

We often see the need to update the test harness to a newer version when testing new core versions, so enforce a minimum version. This PR refactors the existing code to be more reusable. I tested this by running the jenkinsci/bom test suite, with the only two failures being fixed by jenkinsci/workflow-scm-step-plugin#94 and jenkinsci/pipeline-model-definition-plugin#595.

@basil
Copy link
Member Author

basil commented Mar 22, 2023

Passing jenkinsci/bom testing pending the release of jenkinsci/workflow-scm-step-plugin#94 and the merge/release of jenkinsci/pipeline-model-definition-plugin#595.

@basil basil force-pushed the jenkins-test-harness branch 2 times, most recently from d628f28 to 5ad3916 Compare March 23, 2023 03:39
basil added a commit to basil/bom that referenced this pull request Mar 23, 2023
@basil
Copy link
Member Author

basil commented Mar 23, 2023

BOM test running at jenkinsci/bom#1860 with new releases of workflow-scm-step and pipeline-model-definition.

@basil
Copy link
Member Author

basil commented Mar 23, 2023

jenkinsci/bom#1860 passed.

@basil
Copy link
Member Author

basil commented Mar 28, 2023

@jtnord ?

@jtnord
Copy link
Member

jtnord commented Mar 28, 2023

@jtnord ?

As the linked PR shows the build fails. Most likely as the new version of the test harness brings in a new dependency - junit5 and then junit4 tests are not run unless the legacy provider is also on the classpath which it may not be.

Additionally some plugins have issues due to the non backward compatible nature of the j-t-h as noticed in the OSS build requiring changes and our internal build.
the internal build is java.lang.NoSuchMethodError: 'void org.jvnet.hudson.test.RealJenkinsRule.then(org.jvnet.hudson.test.RealJenkinsRule$Step)' , org.jvnet.hudson.test.RealJenkinsRule.runRemotely(org.jvnet.hudson.test.RealJenkinsRule$Step)'

Is there any references to the breaking builds caused by the jth and Jenkins incompatibility? I am aware HTMLUnit has caused some issues in the past but have not seen reports about the j-t-h itself. Is the update just to get the updated htmlunit or something else?

Given the j-t-h is not required to keep binary compatibility, nor are reviewers looking at it (rather expecting plugins to adopt as an when they upgrade) doing this in the PCT seems likely to introduce as many errors as it may fix

@basil
Copy link
Member Author

basil commented Mar 28, 2023

Part of it is to get HTMLUnit fixes, but part of it is also to have a consistent and up-to-date build environment for running tests. For example, recent test harnesses test with Jetty 10 which is also used in recent core releases. Assuming a relatively stable parent POM, a consistent and up-to-date test harness seems more of a benefit than a hindrance to me.

Anyway, I am not going to wrestle with you about this. I would like this in jenkinsci/bom, and the testing passed there, so are you OK if I merge this PR and then if you do not want it on the CloudBees side you can just exclude the hook with --exclude-hook?

@jtnord
Copy link
Member

jtnord commented Mar 28, 2023

are you OK if I merge this PR and then if you do not want it on the CloudBees side you can just exclude the hook with --exclude-hook?

sure.

@basil
Copy link
Member Author

basil commented Mar 28, 2023

As the linked PR shows the build fails. Most likely as the new version of the test harness brings in a new dependency - junit5 and then junit4 tests are not run unless the legacy provider is also on the classpath which it may not be.

This is a good point and I think highlights another angle to PropertyVersionHelper: that some property versions themselves depend on plugin parent POM versions. For example the test harness that requires JUnit 5 depends on plugin parent POM 4.44 or later. I'll expand the scope to cover this before merging this.

@basil basil marked this pull request as draft March 28, 2023 16:48
@basil basil marked this pull request as ready for review March 28, 2023 17:34
@basil
Copy link
Member Author

basil commented Mar 28, 2023

@jtnord Take a look at the latest commit; this only upgrades the test harness to a JUnit 5 requiring version if the plugin parent POM supports it. Does this bring the number of CloudBees internal failures down to a number low enough that you might reconsider adopting this?

@basil
Copy link
Member Author

basil commented Mar 28, 2023

Does this bring the number of CloudBees internal failures down to a number low enough that you might reconsider adopting this?

PS: If it doesn't, I can keep raising the minimum plugin parent POM version that this applies to until you get zero failures. The point is this design should be able to get you to zero failures on your side, at which point whether or not to exclude the hook is only a question of whether the benefit (which I identified) is worth the cost (which you identified).

@basil
Copy link
Member Author

basil commented Mar 29, 2023

@jtnord of the 6 failures in the internal test run, pipeline-model-definition has been fixed in the latest release and suppress-stack-trace is slated for removal.

I just bumped the minimum plugin parent POM version to 4.48 in commit 4f0db2e. That should eliminate the failures in cloudbees-platform-common, cloudbees-unified-ui, and user-activity-monitoring from your next test run.

That just leaves cloudbees-cyberark-credentials. Refreshing this should be low-effort and would get your internal builds green, I think. Are you interested in trying it out?

@basil
Copy link
Member Author

basil commented Mar 30, 2023

I give up.

@basil basil closed this Mar 30, 2023
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.

2 participants