Skip to content

Conversation

@strangelookingnerd
Copy link
Contributor

@strangelookingnerd strangelookingnerd commented Jun 27, 2025

In the light of jenkinsci/throttle-concurrent-builds-plugin#302 (review) and jenkinsci/text-finder-plugin#255 (comment) I want to create JUnit5 equivalents for the most commonly used Rule implementations:

BuildWatcher -> BuildWatcherExtension
InboundAgentRule -> InboundAgentExtension
JenkinsSessionRule -> JenkinsSessionExtension
RealJenkinsRule -> RealJenkinsExtension

Feedback would be highly appreciated (ping @basil @timja @jglick)

The current state mostly copies the existing *Rule into an *Extension, implementing the corresponding wrapper-methods of the respective framework.

I don't like that this will likely cause the implementations to diverge at some point. However I failed to come up with a better solution that does not force me to refactor the entire thing. Something that would already help is to allow the classes to be compared more easily. Right now the formatting is all over the place - spotless could help.

Overall there is some very questionable coding, hacks and workarounds in both implementations, existing and new. I already removed some deprecations as well as TODO that are no longer relevant. Another pair of eyes would however help here.

As always, give me a ping if there any questions.

Testing done

I copied the existing tests for the rules above and adapted them to use JUnit5 and my *Extension implementations.
My main concern right now is that even the existing tests are only covering a small percentage of the functionallity provided, meaning there is a certain risk that some things simply do not work (yet).

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

From a cursory glance it looks fine, best would be to try migrate a few plugins and see how it does.

@strangelookingnerd strangelookingnerd marked this pull request as ready for review July 3, 2025 08:43
@strangelookingnerd strangelookingnerd force-pushed the junit5rules branch 2 times, most recently from 76576cc to b1dd1c2 Compare July 3, 2025 12:38
@strangelookingnerd strangelookingnerd requested a review from timja July 25, 2025 12:15
@timja
Copy link
Member

timja commented Jul 25, 2025

From a cursory glance it looks fine, best would be to try migrate a few plugins and see how it does.

What plugins have you migrated for each extension?

@strangelookingnerd
Copy link
Contributor Author

strangelookingnerd commented Jul 25, 2025

What plugins have you migrated for each extension?

I have some prepared, will create & link the PR.

jenkinsci/throttle-concurrent-builds-plugin#302 -> JenkinsSessionExtension and BuildWatcherExtension
jenkinsci/matrix-auth-plugin#175 -> RealJenkinsExtension and JenkinsSessionExtension
jenkinsci/workflow-durable-task-step-plugin#465 -> RealJenkinsExtension, JenkinsSessionExtension, BuildWatcherExtension and InboundAgentExtension
jenkinsci/workflow-api-plugin#420 -> BuildWatcherExtension, JenkinsSessionExtension, and InboundAgentExtension

@strangelookingnerd
Copy link
Contributor Author

strangelookingnerd commented Jul 28, 2025

Overall my approach seems to work just fine.

I found a tiny issue with meta-annotations such as LocalData in JenkinsSessionExtension that was fixed in d9a5fe4.

Should I add some more migrations or would the four linked above suffice?

@timja timja merged commit 665ccad into jenkinsci:master Jul 28, 2025
15 checks passed
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