-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Migrate tests to JUnit5 #1818
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
base: master
Are you sure you want to change the base?
Migrate tests to JUnit5 #1818
Conversation
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.
Previously it was not possible to have more than one GitSampleRepoRule
per test when using this extension.
As there are several examples of that being required in tests, I added it here.
I already noticed this requirement in some other plugins that could also benefit from this enhancement.
Even though the changeset is already huge, there is still plenty more room for improvements in code quality. |
* Migrate annotations and imports * Migrate assertions * Remove public visibility for test classes and methods * Minor code cleanup
22aaea3
to
177b645
Compare
There is definitely interest! Improvements to the test classes would be much appreciated. |
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.
The test suite in the git plugin is used by other plugins. They consume the GitSampleRepoRule
(as one example). As far as I can tell, this change will break those consumers.
Can the plugin still provide the GitSampleRepoRule and other dependencies that are needed for plugins that use it like these?
What change exactly? I don't think there is any breaking change hidden.
Since I did not remove the GitSampleRepoRule or change it in an incompatible way I would not see why not. My change to the GitSampleRepoExtension is also not problematic for consumers, quite the opposite should be the case. I am unaware of other classes that may be used by consumers e.g. as base classes for their tests. However I'd be willing to fix them in case we find any. |
Thanks very much for your willingness! When I apply the following patch to the copyartifact plugin it fails the LegacyJobConfigMigrationMonitorMigrationTest:
If I switch the git plugin version from the incremental the version provided by the plugin BOM, then the tests pass. |
I'll look into it and see what change may be causing this 👍 |
Seems to be an issue with the JUnit version pulled by the copyartifact plugin vs. the git plugin. While that should be resolved when using the latest pom / bom for the time being reverting to the deprecated method call should be fine. Fixed in 2dbe72f. |
This PR aims to migrate all tests to JUnit5. Changes include:
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