refactor: Migrate from JUnit 4 to JUnit 5#276
Open
matyik wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe pull request migrates tests across API and resource modules from JUnit 3/4 to JUnit 5, updates Mockito and WireMock integrations, adjusts assertion signatures, removes legacy JUnit dependencies, and upgrades related Maven tooling and WireMock versions. ChangesJUnit 5 test migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 19901527 | Triggered | JSON Web Token | 66aa5f2 | pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/LoggerReaderInterceptorTest.java | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices. AWS Best Practice
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
|
New Issues (65)Checkmarx found the following issues in this Pull Request
Fixed Issues (4)Great job! The following issues were fixed in this Pull Request
Use @Checkmarx to interact with Checkmarx PR Assistant. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
pic-sure-resources/pic-sure-aggregate-data-sharing-resource/src/test/java/edu/harvard/hms/dbmi/avillach/ObfuscatedCountShapeTest.java (1)
123-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
assertNullinstead ofassertEquals(null, ...)For improved readability and idiomatic JUnit 5 usage, consider using
assertNullrather thanassertEqualswith anullexpected value. (Be sure to statically importassertNullif applying this change).♻️ Proposed refactor
- assertEquals(null, result.variance(), "Exact (authorized) values carry no uncertainty band"); + assertNull(result.variance(), "Exact (authorized) values carry no uncertainty band");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pic-sure-resources/pic-sure-aggregate-data-sharing-resource/src/test/java/edu/harvard/hms/dbmi/avillach/ObfuscatedCountShapeTest.java` at line 123, Replace the null comparison in ObfuscatedCountShapeTest with JUnit 5’s assertNull, preserving the existing variance() assertion message and adding the corresponding static import if needed.pic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/service/ResourceWebClientTest.java (1)
345-345: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat the array in the assertion message.
Implicitly concatenating an array like
mockResultwith a string calls its defaulttoString()method, which outputs its memory reference (e.g.,[B@1f2b3c4) rather than its actual contents. Consider usingjava.util.Arrays.toString()to include the readable values in the failure message.♻️ Proposed refactor
- assertArrayEquals(mockResult, resultContent, "Result should match " + mockResult); + assertArrayEquals(mockResult, resultContent, "Result should match " + java.util.Arrays.toString(mockResult));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/service/ResourceWebClientTest.java` at line 345, Update the assertion message in ResourceWebClientTest around assertArrayEquals to format mockResult with java.util.Arrays.toString() instead of implicit array-to-string concatenation, so failures display the array contents.pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.java (1)
184-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicated comments. It looks like the inline comment was duplicated into the outer scope when the calls were wrapped in
assertThrowslambdas during the JUnit 5 refactoring.
pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.java#L184-L185: Remove the redundant// Test passes if NotAuthorizedException is throwncomment inside the lambda block.pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.java#L197-L198: Remove the redundant// Test passes if NotAuthorizedException is throwncomment inside the lambda block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.java` around lines 184 - 185, Remove the redundant “Test passes if NotAuthorizedException is thrown” inline comment from both assertThrows lambda blocks in JWTFilterTest at the anchor site (lines 184-185) and sibling site (lines 197-198); leave the test behavior unchanged.pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/SystemServiceTest.java (2)
4-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport fully qualified annotations and classes.
The
@BeforeEachannotation and theWireMockclass are used with their fully qualified names. You can import them at the top of the file to improve readability.♻️ Proposed refactor
import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.extension.RegisterExtension; import edu.harvard.dbmi.avillach.PicSureWarInit; import com.github.tomakehurst.wiremock.core.WireMockConfiguration; import com.github.tomakehurst.wiremock.junit5.WireMockExtension; +import com.github.tomakehurst.wiremock.client.WireMock; import edu.harvard.dbmi.avillach.data.entity.Resource; import edu.harvard.dbmi.avillach.data.repository.ResourceRepository; public class SystemServiceTest { `@RegisterExtension` public WireMockExtension wireMockRule = WireMockExtension.newInstance().options(WireMockConfiguration.options().port(0)).build(); private int port; - `@org.junit.jupiter.api.BeforeEach` + `@BeforeEach` public void setup() { this.port = wireMockRule.getPort(); - com.github.tomakehurst.wiremock.client.WireMock.configureFor(this.port); + WireMock.configureFor(this.port); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/SystemServiceTest.java` around lines 4 - 33, Update SystemServiceTest imports to include BeforeEach and WireMock, then replace their fully qualified usages in setup() with the imported symbols while preserving existing behavior.
87-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
assertThrowsovertry-catchwithassertTrue(false, ...)in JUnit 5.Using
assertThrowsis the idiomatic way to test expected exceptions in JUnit 5. It eliminates the need fortry-catchblocks and avoids theassertTrue(false)anti-pattern.♻️ Proposed refactor
Ensure you add the
assertThrowsimport alongside the other assertions at the top of the file:import static org.junit.jupiter.api.Assertions.assertThrows;`@Test` public void testThrowsExceptionOnStartupIfTokenIntrospectionTokenNotConfigured() { SystemService service = basicService(); when(service.picSureWarInit.getToken_introspection_token()).thenReturn(null); - try { - service.token_introspection_token = null; - service.init(); - } catch (Exception e) { - return; - } - assertTrue(false, "Expected an exception to be thrown."); + service.token_introspection_token = null; + assertThrows(Exception.class, service::init, "Expected an exception to be thrown."); } `@Test` public void testThrowsExceptionOnStartupIfTokenIntrospectionUrlNotConfigured() { SystemService service = basicService(); when(service.picSureWarInit.getToken_introspection_url()).thenReturn(null); - try { - service.token_introspection_url = null; - service.init(); - } catch (Exception e) { - return; - } - assertTrue(false, "Expected an exception to be thrown."); + service.token_introspection_url = null; + assertThrows(Exception.class, service::init, "Expected an exception to be thrown."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/SystemServiceTest.java` around lines 87 - 112, Refactor testThrowsExceptionOnStartupIfTokenIntrospectionTokenNotConfigured and testThrowsExceptionOnStartupIfTokenIntrospectionUrlNotConfigured to use JUnit 5 assertThrows around service.init() instead of try-catch with assertTrue(false). Add the corresponding static assertThrows import and preserve each test’s existing setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.java`:
- Around line 184-185: Remove the redundant “Test passes if
NotAuthorizedException is thrown” inline comment from both assertThrows lambda
blocks in JWTFilterTest at the anchor site (lines 184-185) and sibling site
(lines 197-198); leave the test behavior unchanged.
In
`@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/SystemServiceTest.java`:
- Around line 4-33: Update SystemServiceTest imports to include BeforeEach and
WireMock, then replace their fully qualified usages in setup() with the imported
symbols while preserving existing behavior.
- Around line 87-112: Refactor
testThrowsExceptionOnStartupIfTokenIntrospectionTokenNotConfigured and
testThrowsExceptionOnStartupIfTokenIntrospectionUrlNotConfigured to use JUnit 5
assertThrows around service.init() instead of try-catch with assertTrue(false).
Add the corresponding static assertThrows import and preserve each test’s
existing setup.
In
`@pic-sure-resources/pic-sure-aggregate-data-sharing-resource/src/test/java/edu/harvard/hms/dbmi/avillach/ObfuscatedCountShapeTest.java`:
- Line 123: Replace the null comparison in ObfuscatedCountShapeTest with JUnit
5’s assertNull, preserving the existing variance() assertion message and adding
the corresponding static import if needed.
In
`@pic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/service/ResourceWebClientTest.java`:
- Line 345: Update the assertion message in ResourceWebClientTest around
assertArrayEquals to format mockResult with java.util.Arrays.toString() instead
of implicit array-to-string concatenation, so failures display the array
contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df5a5eab-4039-4350-88c6-2d4b573eb4bd
📒 Files selected for processing (30)
pic-sure-api-data/pom.xmlpic-sure-api-data/src/test/java/edu/harvard/dbmi/avillach/DataTest.javapic-sure-api-war/pom.xmlpic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/BaseServiceTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/ConfigurationRSTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/LoggerReaderInterceptorTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/PicsureInfoServiceTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/PicsureQueryServiceTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/PicsureSearchServiceTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/AuditLoggingFilterTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/ConfigurationServiceTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/LoggingClientProducerTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/NamedDatasetServiceTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/SiteParsingServiceTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/SystemServiceTest.javapic-sure-resources/pic-sure-aggregate-data-sharing-resource/pom.xmlpic-sure-resources/pic-sure-aggregate-data-sharing-resource/src/test/java/edu/harvard/hms/dbmi/avillach/AggregateDataSharingResourceRSAcceptanceTests.javapic-sure-resources/pic-sure-aggregate-data-sharing-resource/src/test/java/edu/harvard/hms/dbmi/avillach/ObfuscatedCountShapeTest.javapic-sure-resources/pic-sure-aggregate-data-sharing-resource/src/test/java/edu/harvard/hms/dbmi/avillach/ProxyPostEndpointMocker.javapic-sure-resources/pic-sure-passthrough-resource/src/test/java/edu/harvard/hms/dbmi/avillach/resource/passthru/PassThroughResourceRSTest.javapic-sure-resources/pic-sure-resource-api/pom.xmlpic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/AppTest.javapic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/domain/PaginatedSearchResultTest.javapic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/domain/QueryRequestTest.javapic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/domain/SignedUrlResponseTest.javapic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/service/HttpClientUtilTest.javapic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/service/ProxyWebClientTest.javapic-sure-resources/pic-sure-resource-api/src/test/java/edu/harvard/dbmi/avillach/service/ResourceWebClientTest.javapom.xml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrading to JUnit 5 offers a more standard architecture while not requiring a Java version bump like JUnit 6. This refactor will make future development cleaner without intruding on the project's original compatibility.
Removed
junit:junit(JUnit 4) dependencies. Addedorg.junit.jupiter:junit-jupiter(JUnit 5). Replaced JUnit 4 annotations with JUnit 5 annotations accordingly (@BeforeClass,@Before->@BeforeAll,@BeforeEach)Upgraded
wiremock-standalonetowiremock-jre8to support modern JUnit 5 extensions. This introduced some minimal changes to the matching syntax inJWTFilterTest.java.Upgraded
mockito-coreand addedmockito-junit-jupiterfor JUnit 5 mockito integrationMigrated
@Rule WireMockClassRuleusages to@RegisterExtension WireMockExtensionSwapped
.port()to.getPort()Summary by CodeRabbit