Skip to content

Conversation

@strangelookingnerd
Copy link
Contributor

@strangelookingnerd strangelookingnerd commented Mar 6, 2025

This PR aims to migrate all tests to JUnit5. Changes include:

  • Migrate annotations and imports
  • Migrate assertions
  • Remove public visibility for test classes and methods
  • Minor clean up

I also took the freedom of re-enabling ThrottleConcurrentTest and refactor it to no longer rely on jgiven.
IMHO there is no need for this test in particualar to use it and its better to streamline it with other tests in this plugin.

I am well aware that this is a quite large changeset however I hope that there is still interest in this PR and it will be reviewed.
If there are any questions, please do not hesitate to ping me.

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

* Migrate annotations and imports
* Migrate assertions
* Remove public visibility for test classes and methods
* Minor code cleanup
@strangelookingnerd strangelookingnerd requested a review from a team as a code owner March 6, 2025 11:10
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Incomplete:

There is one exception where a migration was not possible as the tests rely on Rule implementations that have not (yet) been migrated to JUnit5: ThrottleJobPropertyPipelineRestartTest using JenkinsSessionRule (in combination with SemaphoreStep)

@strangelookingnerd
Copy link
Contributor Author

Incomplete:

While it is possible to use JenkinsRule#restart for most cases it does not yet work with SemaphoreStep. It seems like a Semaphore is not properly restored when restarting Jenkins and thus a restarted build is waiting forever. I was looking into solving that but found some roadblocks on the way. In regards to jenkinsci/oidc-provider-plugin#68 (comment) I’m not sure if those rare cases (as I rarely encountered them so far when migrating other plugins to JUnit5) make it worthwhile to investigate further. WDYT?

@strangelookingnerd
Copy link
Contributor Author

strangelookingnerd commented Mar 12, 2025

From what I understood https://github.com/jenkinsci/workflow-support-plugin/blob/a600813d04aac835e7e2bf62a8aaa775c4cd4437/src/test/java/org/jenkinsci/plugins/workflow/test/steps/SemaphoreStep.java#L60 uses the JENKINS_HOME as a File object for the key when restoring / checking the Semaphore. SinceJenkinsRule#restart makes a copy of the current home folder and thereby has a different path when being restarted , breaking the logic described above.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Incomplete:

There is one exception where a migration was not possible as the tests rely on Rule implementations that have not (yet) been migrated to JUnit5: ThrottleJobPropertyPipelineRestartTest using JenkinsSessionRule (in combination with SemaphoreStep)

@strangelookingnerd strangelookingnerd marked this pull request as draft July 28, 2025 10:44
@strangelookingnerd strangelookingnerd marked this pull request as ready for review August 11, 2025 12:46
@strangelookingnerd
Copy link
Contributor Author

strangelookingnerd commented Aug 11, 2025

@basil Now all tests are migrated using the latest JTH version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants