Skip to content

Migrate tests to JUnit5#290

Merged
MarkEWaite merged 1 commit intojenkinsci:masterfrom
strangelookingnerd:migrate_to_junit5
Apr 9, 2025
Merged

Migrate tests to JUnit5#290
MarkEWaite merged 1 commit intojenkinsci:masterfrom
strangelookingnerd:migrate_to_junit5

Conversation

@strangelookingnerd
Copy link
Copy Markdown
Contributor

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 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.

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

@strangelookingnerd strangelookingnerd requested a review from a team as a code owner March 4, 2025 14:52
@MarkEWaite
Copy link
Copy Markdown
Contributor

I was surprised to see that test coverage decreased with this change as reported by the "Indirect changes" page of https://ci.jenkins.io/job/Plugins/job/github-oauth-plugin/job/PR-290/1/coverage/ . The GithubAuthenticationToken class reduced its statement coverage and its branch coverage.

Is there a compelling reason that we can't retain comparable coverage with JUnit 5 as we had with JUnit 4?

@strangelookingnerd
Copy link
Copy Markdown
Contributor Author

strangelookingnerd commented Mar 25, 2025

I was surprised to see that test coverage decreased with this change as reported by the "Indirect changes" page of https://ci.jenkins.io/job/Plugins/job/github-oauth-plugin/job/PR-290/1/coverage/ . The GithubAuthenticationToken class reduced its statement coverage and its branch coverage.

Is there a compelling reason that we can't retain comparable coverage with JUnit 5 as we had with JUnit 4?

I double checked my changes and found two additional assertions that were checking the contract of equals that I seem to have removed by accident. With these back in place the Coverage has improved a little but is still short compared to master.

I double checked the jacoco reports locally and found some oddities: According to the Jacoco Report for JUnit5 GithubAuthenticationToken#clearCaches() is not covered at all (= never called). Which I can not confirm, as it is definitly called multiple times during tests. Same goes for #getGithubServer() and some other methods / fields.

I could imagine that switching to MockitoExtension over MockitoJUnitRunner could be involved here, but I would not know how to verify that.

Edit: Ignore this...

@strangelookingnerd
Copy link
Copy Markdown
Contributor Author

I gave it another go and found the culprit to be in GithubRequireOrganizationMembershipACLTest. I accidentally changed one of the mocks which caused the overall coverage to decrease. I fixed it now. Coverage is back to normal. Sorry for the fuss.

* Migrate annotations and imports
* Migrate assertions
* Remove public visibility for test classes and methods
* Minor code cleanup
Copy link
Copy Markdown
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarkEWaite MarkEWaite merged commit b2c94d3 into jenkinsci:master Apr 9, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants