Migrate tests to JUnit5#420
Conversation
* Migrate annotations and imports * Migrate assertions * Remove public visibility for test classes and methods * Minor code cleanup
# Conflicts: # src/test/java/org/jenkinsci/plugins/workflow/log/FileLogStorageTest.java
Bumps [org.jenkins-ci.plugins:plugin](https://github.com/jenkinsci/plugin-pom) from 5.24 to 5.26. - [Release notes](https://github.com/jenkinsci/plugin-pom/releases) - [Changelog](https://github.com/jenkinsci/plugin-pom/blob/master/CHANGELOG.md) - [Commits](jenkinsci/plugin-pom@plugin-5.24...plugin-5.26) --- updated-dependencies: - dependency-name: org.jenkins-ci.plugins:plugin dependency-version: '5.26' dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [org.awaitility:awaitility](https://github.com/awaitility/awaitility) from 4.2.2 to 4.3.0. - [Changelog](https://github.com/awaitility/awaitility/blob/master/changelog.txt) - [Commits](awaitility/awaitility@awaitility-4.2.2...awaitility-4.3.0) --- updated-dependencies: - dependency-name: org.awaitility:awaitility dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
# Conflicts: # src/test/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionListTest.java
| public class ArtifactManagerTest { | ||
| @WithJenkins | ||
| class ArtifactManagerTest { |
There was a problem hiding this comment.
Will break https://github.com/jenkinsci/artifact-manager-s3-plugin/blob/74e888e4e3f275675814d7f809db634720730a8f/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/JCloudsArtifactManagerTest.java#L43 https://github.com/jenkinsci/artifact-manager-s3-plugin/blob/74e888e4e3f275675814d7f809db634720730a8f/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStoreTest.java#L27 https://github.com/jenkinsci/compress-artifacts-plugin/blob/a630439048b97242497f939b39094383277b6551/src/test/java/org/jenkinsci/plugins/compress_artifacts/CompressingArtifactManagerFactoryTest.java#L27 etc.
There was a problem hiding this comment.
I'll look into it. On a side-note: Is there a clever way for me to find these kind of dependencies or to you just happen to know them?
There was a problem hiding this comment.
In this case I wrote the class and some of its subclasses so I just happened to remember that this was being used as a test utility, not only a suite here. And I have had to make matching changes in the past for various reasons, like migrating away from docker-fixtures.
It is not immediately apparently that just restoring public would suffice; you would need to try updating at least one subclass to use the Jupiter version and verify that it still runs as a test (and, of course, passes).
There was a problem hiding this comment.
https://github.com/search?type=code&q=org%3Ajenkinsci+NOT+is%3Aarchived+ArtifactManagerTest FTR (I use a Firefox bookmarklet for this sort of search).
| * All {@link FlowNode}s will this result in calling {@link SimpleChunkVisitor#atomNode(FlowNode, FlowNode, FlowNode, ForkScanner)} | ||
| */ | ||
| public class NoOpChunkFinder implements ChunkFinder { | ||
|
|
There was a problem hiding this comment.
In general I would prefer to avoid gratuitous formatting changes as part of a PR which is not just formatting changes (which should rather be one to enable Spotless).
jglick
dismissed
their stale review
addressed (still TBD whether the subclassing works though)
|
I validated the changes in jenkinsci/artifact-manager-s3-plugin#679 (JUnit 4) as well as in jenkinsci/artifact-manager-s3-plugin#664 (JUnit Jupiter). Changes appear to be working fine in both variants. As always I'd of course be available to fix any issues that may come up regardless. |
3 participants
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