Restructure Tests to distinguish between UnitTests and IntegrationTests #2258
Conversation
WalkthroughTests are reorganized into unit and integration test suites with improved separation of concerns. OAuth2 B2C tests are refactored with a shared test setup class. Integration tests are relocated from core packages to a new Changes
Sequence Diagram(s)sequenceDiagram
participant UnitTests
participant IntegrationTests
participant OAuth2ResourceB2CTestSetup
participant OAuth2ResourceB2CUnitTest
participant OAuth2ResourceB2CIntegrationTest
participant InMemB2CResourceTest
participant InMemB2CResourceIntegrationTest
UnitTests->>UnitTests: `@ExcludePackages`("com.predic8.membrane.integration")
Note over UnitTests: Excludes integration package from unit discovery
IntegrationTests->>IntegrationTests: `@SelectClasses`({IntegrationTestsWithoutInternet,<br/>IntegrationTestsWithInternet})
Note over IntegrationTests: Aggregates all integration tests
OAuth2ResourceB2CTestSetup->>OAuth2ResourceB2CTestSetup: `@BeforeEach`: init mocks,<br/>auth server, B2C membrane
OAuth2ResourceB2CTestSetup->>OAuth2ResourceB2CTestSetup: `@AfterEach`: cleanup resources
Note over OAuth2ResourceB2CTestSetup: Shared test infrastructure
OAuth2ResourceB2CUnitTest->>OAuth2ResourceB2CTestSetup: extends
Note over OAuth2ResourceB2CUnitTest: Unit test base class
OAuth2ResourceB2CIntegrationTest->>OAuth2ResourceB2CTestSetup: extends
OAuth2ResourceB2CIntegrationTest->>OAuth2ResourceB2CIntegrationTest: useRefreshTokenOnTokenExpiration()<br/>parallel token refresh test
Note over OAuth2ResourceB2CIntegrationTest: Integration test base with<br/>concurrency scenarios
InMemB2CResourceTest->>OAuth2ResourceB2CUnitTest: extends
Note over InMemB2CResourceTest: Unit test variant
InMemB2CResourceIntegrationTest->>OAuth2ResourceB2CIntegrationTest: extends
InMemB2CResourceIntegrationTest->>InMemB2CResourceIntegrationTest: createSessionManager()<br/>returns InMemorySessionManager
Note over InMemB2CResourceIntegrationTest: Integration test variant
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2ResourceB2CIntegrationTest.java (1)
47-64: Fail fast on worker exceptions for clearer test signalCatching
Exceptionand just printing the stack trace can let thread-level failures slip by without immediately failing the test, obscuring the root cause. Please rethrow (or callfail()), so we get a deterministic test failure when a worker hits an error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/InMemB2CResourceTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/JwtB2CResourceTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTestSetup.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/SyncB2CResourceTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/IntegrationTests.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/InMemB2CResourceIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/JwtB2CResourceIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2ResourceB2CIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/SyncB2CResourceIntegrationTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.787Z
Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters.
Applied to files:
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java
🧬 Code graph analysis (4)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/SyncB2CResourceIntegrationTest.java (1)
core/src/test/java/com/predic8/membrane/core/interceptor/session/FakeSyncSessionStoreManager.java (1)
FakeSyncSessionStoreManager(25-47)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java (3)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
ProblemDetails(36-414)core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-238)core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.java (1)
AbstractInterceptor(29-148)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTestSetup.java (1)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/BrowserMock.java (1)
BrowserMock(47-440)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2ResourceB2CIntegrationTest.java (1)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTestSetup.java (1)
OAuth2ResourceB2CTestSetup(26-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
core/src/test/java/com/predic8/membrane/integration/IntegrationTests.java (2)
1-4: LGTM!The package declaration and imports are correct for a JUnit Platform test suite.
6-12: Test suite references are valid.Both
IntegrationTestsWithoutInternetandIntegrationTestsWithInternetclasses exist in the codebase and are correctly referenced by the@SelectClassesannotation. The test suite structure is correct.core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTestSetup.java (2)
36-43: Good test lifecycle management.The
@BeforeEachmethod properly resets test state flags and initializes test resources, ensuring test isolation.
45-49: Proper resource cleanup.The
@AfterEachmethod ensures that test resources (mock server and membrane) are properly stopped after each test.core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java (2)
35-35: Well-designed test hierarchy.Making this class abstract while extending
OAuth2ResourceB2CTestSetupprovides a clean separation: the base setup handles lifecycle and common scaffolding, this class provides reusable test methods, and concrete subclasses provide specificSessionManagerimplementations. This enables testing the same OAuth2 B2C flows with different session storage backends.
37-463: Comprehensive test coverage.The test suite covers key OAuth2 B2C scenarios including authentication flows, logout, state attack prevention, session management, multiple user flows, error handling, and various edge cases. The tests are well-structured with clear assertions.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/InMemB2CResourceIntegrationTest.java (1)
6-11: Clean integration test implementation.This concrete test class correctly implements the template method pattern by providing an
InMemorySessionManagerinstance. The implementation is focused and follows the established pattern for session manager variants.
...java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTestSetup.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2Test.java (1)
61-75: Use dynamic port allocation to avoid conflicts.Hardcoded ports (2000, 3000) can cause conflicts when tests run in parallel or when these ports are already in use. Consider using port 0 to let the OS assign available ports dynamically, then retrieve the actual port from the transport after starting.
Apply this pattern:
-oAuth2ServerProxy = new ServiceProxy(new ServiceProxyKey("localhost", "*", ".*", 2000), null, 0); +oAuth2ServerProxy = new ServiceProxy(new ServiceProxyKey("localhost", "*", ".*", 0), null, 0);After
router.start(), retrieve the actual port:int oauthPort = router.getTransport().getPort();Then use this port when constructing URLs in the test methods.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorOpenidTest.java (1)
105-107: Fix lambda parameter name mismatch.The lambda parameter is named
exchangebut the body referencesexc, which will either fail to compile or reference the wrong variable from an outer scope.Apply this diff to fix the variable reference:
private static ExceptionThrowingConsumer<Exchange> userinfoOpenidRequestPostprocessing() { - return exchange -> assertEquals("[email protected]", parseSimpleJSONResponse(exc.getResponse()).get("email")); + return exchange -> assertEquals("[email protected]", parseSimpleJSONResponse(exchange.getResponse()).get("email")); }
🧹 Nitpick comments (7)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/AcmeRenewTest.java (2)
97-99: Consider adding a timeout to prevent indefinite hanging.The busy-wait loop could hang indefinitely if
isReady()never returns true. Consider adding a timeout mechanism or using a countdown latch pattern.Example with timeout:
-while (!acmeSSLContext.isReady()) { - Thread.sleep(100); -} +long timeout = System.currentTimeMillis() + 30000; // 30 second timeout +while (!acmeSSLContext.isReady()) { + if (System.currentTimeMillis() > timeout) { + fail("ACME SSL context did not become ready within timeout"); + } + Thread.sleep(100); +}
115-115: TODO comment suggests using @afterall for cleanup.The TODO indicates that router cleanup should be moved to an
@AfterAllmethod instead of the test method's finally block. This would provide cleaner lifecycle management.Would you like me to help implement this improvement or open an issue to track it?
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2Test.java (2)
175-182: Add timeouts to HTTP connections for test reliability.The HTTP connections lack timeout configurations, which could cause tests to hang indefinitely if the server becomes unresponsive.
Add timeouts after creating the connection:
HttpURLConnection connection = (HttpURLConnection) new URL("http://localhost:3000").openConnection(); +connection.setConnectTimeout(5000); +connection.setReadTimeout(5000);Also applies to: 195-202
184-193: Consider using JSONObject for consistency and robustness.The manual string parsing with
Pattern.quoteandsplitis more brittle than usingJSONObject, which is already used elsewhere in this test class.Simplify with proper JSON parsing:
-private static String parseTokenRequestResponse(String key, String tokenRequestResponse) { - - String temp = tokenRequestResponse.replaceFirst(Pattern.quote("{\""+key+"\":\""),""); - String token = temp.split(Pattern.quote("\""))[0]; - - temp = temp.replaceFirst(Pattern.quote(token + "\",\"token_type\":\""),""); - String tokenType = temp.split(Pattern.quote("\""))[0]; - - return tokenType + " " + token; -} +private static String parseTokenRequestResponse(String key, String tokenRequestResponse) { + JSONObject json = new JSONObject(tokenRequestResponse); + String token = json.getString(key); + String tokenType = json.getString("token_type"); + return tokenType + " " + token; +}core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UtilTest.java (1)
18-18: Use consistent assertion style.The test mixes qualified (
Assertions.assertTrueon line 29) and unqualified (assertFalseon line 36) assertion calls. Choose one approach consistently: either use the static import throughout or qualify all assertions with theAssertionsclass.Option 1 (recommended): Use static imports consistently
-import com.predic8.membrane.core.interceptor.oauth2.OAuth2Util; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.params.*; import org.junit.jupiter.params.provider.*; +import com.predic8.membrane.core.interceptor.oauth2.OAuth2Util; import static org.junit.jupiter.api.Assertions.*;- Assertions.assertTrue(OAuth2Util.isOpenIdScope(scope)); + assertTrue(OAuth2Util.isOpenIdScope(scope));Option 2: Remove static import and qualify all assertions
-import static org.junit.jupiter.api.Assertions.*;assertFalse(OAuth2Util.isOpenIdScope(scope)); + Assertions.assertFalse(OAuth2Util.isOpenIdScope(scope));Also applies to: 22-22, 29-29
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/ClaimsParameterTest.java (1)
25-70: Verify the test classification as an integration test.This test uses mock data (
OAuth2TestUtil.getMockClaims()) and appears to test theClaimsParameterclass in isolation without external services, network calls, or infrastructure setup. These characteristics typically align with unit tests rather than integration tests.Please confirm that this test belongs in the
integration.withoutinternetpackage according to your project's test classification criteria, or consider whether it should be classified as a unit test instead.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointTest.java (1)
50-78: Consider refactoring anonymous inner classes to lambdas.The three anonymous inner classes implementing
Callable<Object>could be more concisely expressed as lambdas.Example refactor:
private static Callable<Object> modifySessionToCodeResponseType() { - return new Callable<>() { - @Override - public Object call() { - modifySessionAttributes("response_type", "code"); - return this; - } - }; + return () -> { + modifySessionAttributes("response_type", "code"); + return null; // or appropriate return value + }; }Apply similar changes to
modifySessionToTokenResponseType()andmodifySessionToUnsupportedType().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/AcmeRenewTest.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithSessionRequestJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithSessionRequestTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionOpenidRequestJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionOpenidRequestTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionRequestJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionRequestTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/ClaimsParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointOpenidJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointOpenidTest.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorNormalJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorNormalTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorOpenidJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorOpenidTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2Test.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UtilTest.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordGrantJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordGrantTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/RequestParameterizedTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/RevocationRequestTest.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/TokenRequestJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/TokenRequestTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/UserinfoRequestJWTTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/UserinfoRequestTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/TokenRequestTest.java
- core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordGrantTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.931Z
Learning: The mediaType method in the Request class throws jakarta.mail.internet.ParseException, not java.text.ParseException, making the jakarta.mail.internet.ParseException import necessary and correct in JsonSchemaTestSuiteTests.java.
Applied to files:
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/RequestParameterizedTest.java
🧬 Code graph analysis (16)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/TokenRequestJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointOpenidJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionOpenidRequestJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/AcmeRenewTest.java (1)
core/src/test/java/com/predic8/membrane/core/transport/ssl/acme/AcmeServerSimulator.java (1)
AcmeServerSimulator(50-329)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2Util.java (1)
OAuth2Util(30-89)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorNormalJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordGrantJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionOpenidRequestTest.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ParamNames.java (1)
ParamNames(16-48)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/UserinfoRequestJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorOpenidJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionRequestJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithSessionRequestJWTTest.java (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
OAuth2TestUtil(24-59)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (2)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-238)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/BufferedJsonGenerator.java (1)
BufferedJsonGenerator(22-60)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorOpenidTest.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2Util.java (1)
OAuth2Util(30-89)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ParamNames.java (1)
ParamNames(16-48)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java (2)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/RevocationRequestJWTTest.java (1)
RevocationRequestJWTTest(17-22)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/WellknownFileTest.java (1)
WellknownFileTest(25-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (37)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/AcmeRenewTest.java (1)
14-14: LGTM! Package restructuring is appropriate.The move to the
integration.withoutinternet.interceptorpackage correctly categorizes this as an integration test, as it starts actual server instances, uses network communication, and tests end-to-end ACME functionality. The added import forAcmeServerSimulatoris necessary and correct.Also applies to: 27-27
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2Test.java (2)
14-14: LGTM! Package restructuring aligns with PR objectives.The move from core test package to integration test package is appropriate for this test, which starts HTTP servers and performs end-to-end OAuth2 authentication flows.
21-24: LGTM! Explicit imports are correct.The explicit imports for OAuth2 classes are appropriate following the package restructuring.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointJWTTest.java (2)
13-17: LGTM! Package relocation and imports correctly updated.The package declaration and imports have been properly updated to reflect the test restructuring. The import of
OAuth2AuthorizationServerInterceptorfrom the original package is correct, and the static import correctly referencesOAuth2TestUtilin its new location.
19-24: Code structure is correct.
EmptyEndpointTestis confirmed to be in the same package (com.predic8.membrane.integration.withoutinternet.interceptor.oauth2), so the class extension without an explicit import is valid. The implementation correctly follows the template method pattern for test configuration.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/RevocationRequestTest.java (2)
14-14: LGTM! Package relocation aligns with test restructuring objectives.The package move to
integration.withoutinternetcorrectly categorizes this as an integration test.
23-29: All test dependencies are accessible and properly configured.The verification confirms all referenced classes are located in the same package:
RequestParameterizedTestOAuth2AuthorizationServerInterceptorBaseOAuth2AuthorizationServerInterceptorNormalTestSince they're co-located in the same package as
RevocationRequestTest, they remain accessible without explicit imports. The test framework configuration is properly set up in the build files, and the test should be discovered and executed correctly.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithSessionRequestTest.java (1)
14-14: LGTM: Package relocation verified and complete.Verification confirms all dependencies are properly resolved in the new package location:
RequestParameterizedTestis in the same package and correctly extended byAuthWithSessionRequestTestOAuth2AuthorizationServerInterceptorBaseis in the same package and correctly referenced- No stale references to the old package path remain
The package move aligns with the PR objective to distinguish integration tests from unit tests.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionOpenidRequestTest.java (3)
14-14: LGTM! Package restructuring aligns with PR objectives.The package relocation to
com.predic8.membrane.integration.withoutinternet.interceptor.oauth2correctly categorizes this as an integration test that runs without internet connectivity.
17-17: LGTM! Import correctly added for cross-package access.The explicit import of
ParamNamesis necessary now that the test has moved to a different package.
25-53: All dependencies are properly accessible; review comment should be dismissed.Verification confirms that all referenced classes and dependencies are co-located in the same package (
com.predic8.membrane.integration.withoutinternet.interceptor.oauth2) and properly configured:
RequestParameterizedTestdefinesprotected static Exchange exc(line 34)OAuth2AuthorizationServerInterceptorBasedefinesstatic OAuth2AuthorizationServerInterceptor oasi(line 44)OAuth2AuthorizationServerInterceptorOpenidTestprovidespublic static Callable<Exchange> getMockAuthOpenidRequestExchange()(line 77)- All required imports are present in
AuthWithoutSessionOpenidRequestTestNo accessibility issues exist. The test will compile and run successfully.
Likely an incorrect or invalid review comment.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/ClaimsParameterTest.java (1)
14-14: No action required—OAuth2TestUtil is in the same package.Both
OAuth2TestUtilandClaimsParameterTestare in the packagecom.predic8.membrane.integration.withoutinternet.interceptor.oauth2. In Java, classes in the same package can reference each other without explicit imports, so the code is correct as-is and will compile successfully.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorOpenidTest.java (2)
14-20: LGTM! Package relocation and imports are appropriate.The package move to the integration namespace correctly separates integration tests from unit tests. The new imports for
OAuth2UtilandParamNamesprovide necessary utilities for OpenID Connect test flows.
64-75: Proper use of imported constants and utilities.The method correctly uses the newly imported
ParamNamesconstants and integrates well with the relocatedOAuth2TestUtil.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/TokenRequestJWTTest.java (1)
13-17: LGTM! Package and import updates are correct.The package relocation and import updates properly align with the integration test restructuring. The production code import from
core.interceptor.oauth2and test utility import from the new integration package are both correct.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionOpenidRequestJWTTest.java (1)
13-17: LGTM! Consistent package restructuring.The package and import changes align with the integration test reorganization. All references are correctly updated.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/UserinfoRequestJWTTest.java (1)
13-17: LGTM! Package relocation correctly implemented.The changes follow the established pattern for moving JWT-variant tests to the integration package. All imports are properly updated.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/RequestParameterizedTest.java (1)
14-18: LGTM! Base test class properly relocated.The package move and import addition are consistent with the integration test restructuring. The abstract test class now correctly resides in the integration namespace.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionRequestTest.java (1)
14-21: LGTM! Clean package relocation.The package move properly relocates this integration test. The minor formatting adjustment at line 21 is inconsequential.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2TestUtil.java (1)
14-18: LGTM! Test utility properly relocated with correct imports.The package move places this test utility in the integration namespace where it's now referenced by other integration tests. The added imports from the core package are appropriate for production code dependencies.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithSessionRequestJWTTest.java (1)
13-17: LGTM! Final JWT test variant properly relocated.The package and import changes complete the consistent restructuring of OAuth2 JWT test variants into the integration namespace.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointOpenidJWTTest.java (2)
13-17: LGTM! Package restructuring is correctly implemented.The package and import changes properly distinguish integration tests from core code. Production code (
OAuth2AuthorizationServerInterceptor) correctly remains in the core package, while test utilities (OAuth2TestUtil) have moved to the integration package.
19-24: No issues found.The parent class
EmptyEndpointOpenidTestis located in the same package as the child class (com.predic8.membrane.integration.withoutinternet.interceptor.oauth2), ensuring full accessibility. The package structure is correct and consistent.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordGrantJWTTest.java (2)
13-17: LGTM! Consistent package restructuring.Package and import changes are correctly implemented and consistent with the PR's integration test reorganization.
19-24: No accessibility issues found.
PasswordGrantTestis already located in the integration package at the same location (com.predic8.membrane.integration.withoutinternet.interceptor.oauth2), making it fully accessible toPasswordGrantJWTTest. Both classes reside in the same package andPasswordGrantTestis public, so inheritance works without any issues.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorOpenidJWTTest.java (2)
13-17: LGTM! Package restructuring is correct.Changes properly separate integration tests from core package while maintaining correct dependencies.
19-24: No issues found; parent class is properly accessible.The parent class
OAuth2AuthorizationServerInterceptorOpenidTestis located in the integration package and is in the same package as the child class, ensuring full accessibility. The inheritance structure is correct.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthWithoutSessionRequestJWTTest.java (2)
13-17: LGTM! Package restructuring is correctly implemented.Package and import changes align with the integration test reorganization and maintain proper separation of concerns.
19-24: Parent test class is accessible and properly located.
AuthWithoutSessionRequestTestis in the same integration package and is a public class, making it accessible toAuthWithoutSessionRequestJWTTest. No accessibility issues found.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointOpenidTest.java (1)
14-14: Package relocation verified—all referenced classes are co-located.All three referenced classes (
RequestParameterizedTest,OAuth2AuthorizationServerInterceptorBase, andOAuth2AuthorizationServerInterceptorNormalTest) are located in the same package asEmptyEndpointOpenidTest, making them accessible without explicit imports. The package move is complete and correct.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/EmptyEndpointTest.java (1)
14-14: Package restructuring verified successfully.The move from
com.predic8.membrane.core.interceptor.oauth2tocom.predic8.membrane.integration.withoutinternet.interceptor.oauth2correctly categorizes this test as an integration test. All referenced classes (RequestParameterizedTest,OAuth2AuthorizationServerInterceptorBase,OAuth2AuthorizationServerInterceptorNormalTest) are co-located in the same package, ensuring accessibility without import issues. The package migration is coordinated and complete.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/UserinfoRequestTest.java (2)
17-17: LGTM! Import correctly added for cross-package dependency.The
TokenAuthorizationHeaderimport is necessary since the test has moved to a different package but still needs to reference this class from the core package (used on line 65).
14-14: Package restructuring complete and correct.The test file has been successfully relocated with all necessary imports properly maintained. Verification confirms:
- Package correctly moved to
com.predic8.membrane.integration.withoutinternet.interceptor.oauth2- Cross-package import of
TokenAuthorizationHeaderis present and actively used (line 65)- Base class
RequestParameterizedTestis in the same relocated package- No stale references to the old package path exist
- All imports are syntactically correct
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorNormalTest.java (1)
14-14: LGTM! Package relocation is clean.The package declaration has been correctly updated to the integration namespace. Since the base class
OAuth2AuthorizationServerInterceptorBaseis also being moved in this PR, no additional import changes are needed.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java (2)
14-14: LGTM! Package relocation is correct.The package has been successfully moved to the integration namespace, aligning with the PR's objective to separate integration tests from unit tests.
20-23: LGTM! Necessary imports added after package relocation.These import statements are now required because the test classes have been moved to the
integration.withoutinternet.interceptor.oauth2package while the classes under test (ClaimList,Client,OAuth2AuthorizationServerInterceptor,StaticClientList) remain in thecore.interceptor.oauth2package. Previously, when tests and code were in the same package, these explicit imports were unnecessary.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorNormalJWTTest.java (1)
13-17: Package relocation verified successfully.The relocated test class is properly referenced in
OAuth2UnitTests.java(line 41) within the same package using the bare class name in the@SelectClassesannotation. No references to the old package path remain in the codebase. The import updates and package structure changes are correct.
...ava/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java
Show resolved
Hide resolved
...ava/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java
Show resolved
Hide resolved
...ava/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java
Outdated
Show resolved
Hide resolved
...java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UtilTest.java
Outdated
Show resolved
Hide resolved
…tests from OAuth2UnitTests suite
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java (2)
17-17: Naming inconsistency: "UnitTests" class in integration package.The class
OAuth2UnitTestsis located in thecom.predic8.membrane.integrationpackage hierarchy, which contradicts the PR's goal of distinguishing between unit and integration tests. Either rename the class toOAuth2IntegrationTestsor move it to a non-integration package structure.
45-50: Remove duplicate test class entries.Three test classes appear twice in the
@SelectClassesannotation:
InMemSMOAuth2R2Test.class(lines 45 and 48)JwtSMOAuth2R2Test.class(lines 46 and 49)SyncSMOAuth2R2Test.class(lines 47 and 50)These duplicates will cause the same tests to run multiple times.
Apply this diff to remove the duplicates:
OAuth2Test.class, InMemSMOAuth2R2Test.class, JwtSMOAuth2R2Test.class, SyncSMOAuth2R2Test.class, - InMemSMOAuth2R2Test.class, - JwtSMOAuth2R2Test.class, - SyncSMOAuth2R2Test.class, PasswordGrantTest.class,
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/transport/ssl/OAuth2UtilTest.java (1)
18-18: Inconsistent assertion usage.Line 29 uses
Assertions.assertTrue(...)with explicit qualification, while line 36 uses the statically importedassertFalse(...). For consistency and readability, use one approach throughout the file.Apply this diff to use static imports consistently:
-import org.junit.jupiter.api.Assertions; import org.junit.jupiter.params.*; import org.junit.jupiter.params.provider.*; import static org.junit.jupiter.api.Assertions.*;- Assertions.assertTrue(OAuth2Util.isOpenIdScope(scope)); + assertTrue(OAuth2Util.isOpenIdScope(scope));Also applies to: 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/test/java/com/predic8/membrane/core/transport/ssl/OAuth2UtilTest.java(2 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/transport/ssl/OAuth2UtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2Util.java (1)
OAuth2Util(30-89)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java (2)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/RevocationRequestJWTTest.java (1)
RevocationRequestJWTTest(17-22)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/WellknownFileTest.java (1)
WellknownFileTest(25-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2UnitTests.java (1)
19-20: Imports are syntactically correct.The imports reference unit tests from the core package. The logical placement issue is already addressed by the package/class naming inconsistency comment.
core/src/test/java/com/predic8/membrane/core/transport/ssl/OAuth2UtilTest.java
Show resolved
Hide resolved
|
/ok-to-test |
Summary by CodeRabbit
Release Notes
This release focuses on internal test infrastructure improvements with no end-user-visible changes.