Migrate tests to JUnit5 #509

strangelookingnerd · 2025-04-09T12:37:00Z

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

src/test/java/integration/MigrationTest.java

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

strangelookingnerd · 2025-06-26T10:49:07Z

@jenkinsci/branch-api-plugin-developers Kindly requesting a review.

# Conflicts: # src/test/java/jenkins/branch/WorkspaceLocatorImplTest.java

jglick

-0 I guess, given the degraded assertions in a couple places.

jglick · 2025-11-17T17:05:08Z

pom.xml

-    <dependency>
-      <groupId>org.jenkins-ci.plugins</groupId>
-      <artifactId>junit</artifactId>
-      <scope>test</scope>
-    </dependency>


FTR, added in #84 for no apparent reason.

jglick · 2025-11-17T17:07:19Z

src/test/java/integration/EventsTest.java

-            assumeThat("We have no master branch", master, nullValue());
+            assumeTrue(master == null, "We have no master branch");


What is the purpose of this sort of change? It seems like a regression in the quality of test assertions: before, you would get the actual value of master if it existed.

There does not seem to be an equivalent in the latest JUnit / Hamcrest. I don't recall the history but I think it got dropped by both libraries in a major version and was not picked up again. There was a issue about it on their GitHub but I can't seem to find it right now.

So you cannot use matchers?!

Oh I see, this was assume not assert. I think it should have been assert. There is no apparent reason why this would be sensitive to the test environment.

Mistake in #104 I think. given_multibranchWithMergeableChangeRequests_when_reindexing_then_mergableChangeRequestsRebuilt passes, it is not skipped.

jglick · 2025-11-17T17:07:49Z

src/test/java/integration/EventsTest.java

-            assumeThat("The head change request was built", crHead.getLastBuild(), notNullValue());
-            assumeThat("The head change request was built", crHead.getLastBuild().getNumber(), is(1));
+            assumeTrue(crMerge.getLastBuild() != null, "The merge change request was built");
+            assumeTrue(crMerge.getLastBuild().getNumber() == 1, "The merge change request was built");


Similarly, here you would have gotten a number other than 1. Now you will just get a failure.

jglick · 2025-11-17T17:08:27Z

src/test/java/integration/MigrationTest.java

-                assertDataMigrated(foo);
+    void createdFromScratch() throws Throwable {
+        story.then(j -> {
+            OrganizationFolder foo = j.createProject(OrganizationFolder.class, "foo");


(ignore WS)

jglick · 2025-11-17T17:10:06Z

src/test/java/integration/UpdatingFromXmlTest.java

            c.addFile("foo", "feature", "add new feature", "FEATURE", "new".getBytes());
            String configXml = IOUtils.toString(getClass().getResourceAsStream("UpdatingFromXmlTest/config.xml"), StandardCharsets.UTF_8).replace("fixme", c.getId());
-            BasicMultiBranchProject prj = (BasicMultiBranchProject) r.jenkins.createProjectFromXML("foo", new ReaderInputStream(new StringReader(configXml), StandardCharsets.UTF_8));
+            BasicMultiBranchProject prj = (BasicMultiBranchProject) r.jenkins.createProjectFromXML("foo", ReaderInputStream.builder().setReader(new StringReader(configXml)).setCharset(StandardCharsets.UTF_8).get());


What does this have to do with JUnit 5? If you want to propose random code improvements, great, but please file them separately.

(just feedback for the future)

jglick · 2025-11-17T17:12:00Z

src/test/java/jenkins/branch/BuildRetentionBranchPropertyTest.java

-        instance.jobDecorator((Class) Job.class).project(job);
-        assertTrue("The IOException was caught and ignored", true);
+        assertDoesNotThrow(() -> instance.jobDecorator((Class) Job.class).project(job));


Or just delete the assertTrue; assertDoesNotThrow seems useless.

I'd usually agree since I oftentimes don't see a reason to use assertDoesNotThrow. In this instance however I feel it makes the intention clearer and gives the call to #project a purpose. It's more elegant than using assertTrue(true). Removing assertions should not be my goal here.

strangelookingnerd requested a review from a team as a code owner April 9, 2025 12:37

strangelookingnerd commented Apr 9, 2025

View reviewed changes

src/test/java/integration/MigrationTest.java Show resolved Hide resolved

Migrate tests to JUnit5

e53c08b

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

strangelookingnerd force-pushed the migrate_to_junit5 branch from b9f2b99 to e53c08b Compare April 10, 2025 12:45

strangelookingnerd added 3 commits April 10, 2025 21:56

Merge branch 'master' into migrate_to_junit5

22725f0

Merge branch 'master' into migrate_to_junit5

a7d7274

Merge branch 'master' into migrate_to_junit5

d89bc0a

coderabbitai bot mentioned this pull request Jul 9, 2025

Update PR data Daily collection (2025-07-09 01:43 UTC) gounthar/alpha-omega-stats#117

Merged

strangelookingnerd added 4 commits July 15, 2025 14:03

Merge branch 'master' into migrate_to_junit5

507cb76

# Conflicts: # src/test/java/jenkins/branch/WorkspaceLocatorImplTest.java

Merge branch 'master' into migrate_to_junit5

2e18f0b

Use latest JTH features

8342f3e

Merge branch 'master' into migrate_to_junit5

a9805f2

# Conflicts: # src/test/java/jenkins/branch/WorkspaceLocatorImplTest.java

jglick added the tests label Nov 17, 2025

jglick reviewed Nov 17, 2025

View reviewed changes

strangelookingnerd added 2 commits November 18, 2025 21:04

Merge branch 'master' into migrate_to_junit5

1162e7d

Merge branch 'master' into migrate_to_junit5

01e8a19

		assumeThat("We have no master branch", master, nullValue());
		assumeTrue(master == null, "We have no master branch");

Migrate tests to JUnit5 #509

Are you sure you want to change the base?

Migrate tests to JUnit5 #509

Uh oh!

Conversation

strangelookingnerd commented Apr 9, 2025

Uh oh!

Uh oh!

strangelookingnerd commented Jun 26, 2025

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strangelookingnerd Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

strangelookingnerd Nov 17, 2025 •

edited

Loading