Add the session management test to the TestNG XML configuration#27362
Open
DilshanSenarath wants to merge 2 commits intowso2:masterfrom
Open
Add the session management test to the TestNG XML configuration#27362DilshanSenarath wants to merge 2 commits intowso2:masterfrom
DilshanSenarath wants to merge 2 commits intowso2:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRe-enabled the TestNG session-management Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
DilshanSenarath
force-pushed
the
session-mgt-test
branch
2 times, most recently
from
3787fe5 to
61ff3c3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTestBase.java (1)
420-433: Refactor the array filtering logic and add null safety.The current pattern of reassigning
configswithin a for-each loop is confusing and inefficient (creates a new array on each removal). While it technically works because Java captures the array reference at the start of for-each, a cleaner approach using streams would be more readable and efficient.Additionally, there are potential null pointer issues:
getFederatedAuthenticatorConfigs()may returnnullcfg.getName()may returnnull♻️ Proposed refactor using streams with null safety
protected void resetResidentIDPCache() throws Exception { IdentityProviderMgtServiceClient idpMgtClient = new IdentityProviderMgtServiceClient(sessionCookie, backendURL); IdentityProvider residentIdp = idpMgtClient.getResidentIdP(); FederatedAuthenticatorConfig[] configs = residentIdp.getFederatedAuthenticatorConfigs(); - for (FederatedAuthenticatorConfig cfg : configs) { - if (!cfg.getName().equalsIgnoreCase("samlsso")) { - configs = (FederatedAuthenticatorConfig[]) ArrayUtils.removeElement(configs, cfg); - } + if (configs != null) { + FederatedAuthenticatorConfig[] filteredConfigs = java.util.Arrays.stream(configs) + .filter(cfg -> cfg != null && "samlsso".equalsIgnoreCase(cfg.getName())) + .toArray(FederatedAuthenticatorConfig[]::new); + residentIdp.setFederatedAuthenticatorConfigs(filteredConfigs); } - residentIdp.setFederatedAuthenticatorConfigs(configs); idpMgtClient.updateResidentIdP(residentIdp); }Note: If using streams, the
ArrayUtilsimport on line 24 can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTestBase.java` around lines 420 - 433, The loop in resetResidentIDPCache currently mutates the configs array inside a for-each and lacks null checks; refactor to safely handle nulls by retrieving the resident IDP via IdentityProviderMgtServiceClient.getResidentIdP(), then call getFederatedAuthenticatorConfigs() and if it is non-null, filter the FederatedAuthenticatorConfig[] to only keep entries whose getName() is non-null and equalsIgnoreCase("samlsso") (use Arrays.stream(...).filter(...).toArray(FederatedAuthenticatorConfig[]::new) or equivalent), set the filtered array back on residentIdp via setFederatedAuthenticatorConfigs(...) and call updateResidentIdP(residentIdp); remove the ArrayUtils-based removal logic and add null-safety for both the configs array and cfg.getName().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTestBase.java`:
- Around line 420-433: The loop in resetResidentIDPCache currently mutates the
configs array inside a for-each and lacks null checks; refactor to safely handle
nulls by retrieving the resident IDP via
IdentityProviderMgtServiceClient.getResidentIdP(), then call
getFederatedAuthenticatorConfigs() and if it is non-null, filter the
FederatedAuthenticatorConfig[] to only keep entries whose getName() is non-null
and equalsIgnoreCase("samlsso") (use
Arrays.stream(...).filter(...).toArray(FederatedAuthenticatorConfig[]::new) or
equivalent), set the filtered array back on residentIdp via
setFederatedAuthenticatorConfigs(...) and call updateResidentIdP(residentIdp);
remove the ArrayUtils-based removal logic and add null-safety for both the
configs array and cfg.getName().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43bec908-820f-4dc7-9765-90b544c0655f
📒 Files selected for processing (2)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTestBase.java
✅ Files skipped from review due to trivial changes (1)
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTest.java
DilshanSenarath
force-pushed
the
session-mgt-test
branch
from
61ff3c3 to
eafdbd1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTest.java (2)
35-38: Consider importingjava.time.Durationfor consistency.
Instantis imported, butjava.time.Durationis used with its fully qualified name on lines 121, 132, and 143. Import it alongsideInstantfor consistency.Suggested fix
import java.time.Instant; +import java.time.Duration; import java.util.ArrayList; import java.util.Collections; import java.util.logging.Logger;Then update the usages:
- + " | elapsed since login=" + java.time.Duration.between(loginCompletedAt, check1At).toMillis() + "ms"); + + " | elapsed since login=" + Duration.between(loginCompletedAt, check1At).toMillis() + "ms");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTest.java` around lines 35 - 38, The test class SessionMgtTest imports java.time.Instant but uses java.time.Duration with fully qualified names; add an import for java.time.Duration and replace the fully qualified usages (e.g., java.time.Duration.ofSeconds(...) calls found in the test methods) with the simple Duration identifier to keep imports consistent and shorten the calls.
156-173: Consider adding similar logging to other session timeout tests.Only
testSessionExtensionWithIdleTimeouthas detailed timestamped logging. If these tests are also prone to timing-related flakiness, consider adding consistent logging totestSessionWithRememberMe,testMaximumSessionTimeoutWithIdleTimeout, andtestMaximumSessionTimeoutWithRememberMefor easier debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTest.java` around lines 156 - 173, The other timing-sensitive tests (testSessionWithRememberMe, testMaximumSessionTimeoutWithIdleTimeout, testMaximumSessionTimeoutWithRememberMe) lack the timestamped logging present in testSessionExtensionWithIdleTimeout; add similar detailed, timestamped debug/log statements before and after sleeps and before each checkSessionActive call in these methods so test runs record when login, waits, and session checks occur (use the same logging format and logger used in testSessionExtensionWithIdleTimeout to keep output consistent for debugging).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTest.java`:
- Around line 35-38: The test class SessionMgtTest imports java.time.Instant but
uses java.time.Duration with fully qualified names; add an import for
java.time.Duration and replace the fully qualified usages (e.g.,
java.time.Duration.ofSeconds(...) calls found in the test methods) with the
simple Duration identifier to keep imports consistent and shorten the calls.
- Around line 156-173: The other timing-sensitive tests
(testSessionWithRememberMe, testMaximumSessionTimeoutWithIdleTimeout,
testMaximumSessionTimeoutWithRememberMe) lack the timestamped logging present in
testSessionExtensionWithIdleTimeout; add similar detailed, timestamped debug/log
statements before and after sleeps and before each checkSessionActive call in
these methods so test runs record when login, waits, and session checks occur
(use the same logging format and logger used in
testSessionExtensionWithIdleTimeout to keep output consistent for debugging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90285e20-36fd-4fc5-bf1c-d3924ea2e76f
📒 Files selected for processing (2)
modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTest.javamodules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTestBase.java
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/identity/integration/test/rest/api/server/session/SessionMgtTestBase.java
DilshanSenarath
force-pushed
the
session-mgt-test
branch
3 times, most recently
from
80d4108 to
bbd4ea9
Compare
DilshanSenarath
force-pushed
the
session-mgt-test
branch
from
bbd4ea9 to
62b4a38
Compare
|
Contributor
Author
|
|
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
1 participant
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.
$subject
Summary by CodeRabbit