[OB4] [Test] Adding internal consent update API tests#917
Merged
[OB4] [Test] Adding internal consent update API tests#917
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds internal consent-update support: new constants and payload builders, FSConnectorTest fields and a PUT consent update method, and a comprehensive test class plus testng registration validating internal consent flows and negative scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Framework as FSConnectorTest
participant IS as IdentityServer(API)
participant Store as ConsentStore/DB
Test->>Framework: build consent create payload
Framework->>IS: POST /consent
IS->>Store: persist consent
IS-->>Framework: 201 Created (consentID)
Test->>Framework: build consent update payload
Framework->>IS: PUT /consent/{consentID} with `x-wso2-internal-request`
IS->>Store: validate & update consent, create/update authorizations
IS-->>Framework: 200 OK (updated consent)
Test->>Framework: GET /consent/{consentID} (internal)
Framework->>IS: GET with internal header
IS->>Store: fetch consent
IS-->>Framework: 200 OK (consent state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy (1)
41-41: Avoid shadowingconsentRequestBuilderfromFSConnectorTest.Line 41 redeclares a field already defined in the base class, which can drift behavior between inherited helper methods and local request calls.
♻️ Proposed fix
- ConsentRequestBuilder consentRequestBuilder = new ConsentRequestBuilder()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy` at line 41, The test redeclares consentRequestBuilder which shadows the field on the base class FSConnectorTest; remove the local declaration "ConsentRequestBuilder consentRequestBuilder = new ConsentRequestBuilder()" from InternalConsentManageFlowValidationTest so the class uses the inherited consentRequestBuilder instance, and update any local usages to rely on the base-class field (ensure no other local initializations remain and imports/constructors are unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@fs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovy`:
- Around line 163-192: The receipt field in getAccountConsentUpdatePayload
currently contains hardcoded ISO timestamps causing brittle tests; replace those
literal dates with dynamically generated ISO-8601 strings (e.g., use
ConnectorTestConstants.expirationInstant or Instant.now() /
ConnectorTestConstants helper timestamps formatted to the same pattern) and
inject them into the embedded receipt JSON string (keeping the escaped quotes)
so the receipt's "ExpirationDateTime", "TransactionFromDateTime" and
"TransactionToDateTime" are computed at runtime; update
getAccountConsentUpdatePayload to build those timestamp strings and interpolate
them into the receipt value while preserving existing escaping/JSON structure.
In
`@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy`:
- Around line 78-80: The tests call extractListFromResponse(...) to obtain
authorizationResources and then immediately use authorizationResources.get(0)
(e.g., to set authIdBeforeUpdate) without asserting the list is non-empty; add
an explicit assertion (e.g., assert authorizationResources != null &&
!authorizationResources.isEmpty()) immediately after each
extractListFromResponse call (locations around authorizationResources variable
usage and before any authorizationResources.get(0) access in
InternalConsentManageFlowValidationTest) to fail with a clear message instead of
throwing IndexOutOfBoundsException. Ensure the assertion message clarifies which
response was empty (authorizationResources) and apply this fix to all
occurrences where .get(0) is used (the blocks that set authIdBeforeUpdate and
similar usages).
In
`@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/resources/testng.xml`:
- Around line 67-68: The two new test classes
ConsentManageEndpointValidationTest and InternalConsentManageFlowValidationTest
are defined inside a disabled TestNG <test enabled="false"> block so they won't
run; fix by either removing or setting enabled="true" on that <test> block or
move the class entry for InternalConsentManageFlowValidationTest (and/or
ConsentManageEndpointValidationTest if needed) into an existing enabled <test>
section in testng.xml so the classes are executed during the test run.
---
Nitpick comments:
In
`@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy`:
- Line 41: The test redeclares consentRequestBuilder which shadows the field on
the base class FSConnectorTest; remove the local declaration
"ConsentRequestBuilder consentRequestBuilder = new ConsentRequestBuilder()" from
InternalConsentManageFlowValidationTest so the class uses the inherited
consentRequestBuilder instance, and update any local usages to rely on the
base-class field (ensure no other local initializations remain and
imports/constructors are unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4ffce32-54ea-4218-9275-728fd2f7ba6b
📒 Files selected for processing (5)
fs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/FSConnectorTest.groovyfs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovyfs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/ConnectorTestConstants.groovyfs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovyfs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/resources/testng.xml
...g/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovy
Show resolved
Hide resolved
Comment on lines
+78
to
+80
| List<JSONObject> authorizationResources = extractListFromResponse(consentResponse, "authorizationResources") | ||
| authIdBeforeUpdate = authorizationResources.get(0).get("authorizationID") | ||
| } |
There was a problem hiding this comment.
Assert list non-emptiness before indexing into authorizationResources/resources.
At Line 79/93/118/139 you index with .get(0) after only null checks. If the API returns an empty list, the test fails with IndexOutOfBoundsException instead of a meaningful assertion.
✅ Proposed fix pattern
List<JSONObject> authorizationResources = extractListFromResponse(consentResponse, "authorizationResources")
+ Assert.assertFalse(authorizationResources.isEmpty(), "authorizationResources is empty")
authIdBeforeUpdate = authorizationResources.get(0).get("authorizationID") List<JSONObject> resources = authorizationResources.get(0).get("resources") as List<JSONObject>
Assert.assertNotNull(resources)
+ Assert.assertFalse(resources.isEmpty(), "resources is empty")
mappingIdBeforeUpdate = resources.get(0).get("mappingID")Also applies to: 92-94, 117-121, 134-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy`
around lines 78 - 80, The tests call extractListFromResponse(...) to obtain
authorizationResources and then immediately use authorizationResources.get(0)
(e.g., to set authIdBeforeUpdate) without asserting the list is non-empty; add
an explicit assertion (e.g., assert authorizationResources != null &&
!authorizationResources.isEmpty()) immediately after each
extractListFromResponse call (locations around authorizationResources variable
usage and before any authorizationResources.get(0) access in
InternalConsentManageFlowValidationTest) to fail with a clear message instead of
throwing IndexOutOfBoundsException. Ensure the assertion message clarifies which
response was empty (authorizationResources) and apply this fix to all
occurrences where .get(0) is used (the blocks that set authIdBeforeUpdate and
similar usages).
Comment on lines
+67
to
+68
| <class name="org.wso2.financial.services.accelerator.is.test.consent.management.ConsentOperationsFlowTests.ConsentManageEndpointValidationTest"/> | ||
| <class name="org.wso2.financial.services.accelerator.is.test.consent.management.ConsentOperationsFlowTests.InternalConsentManageFlowValidationTest"/> |
There was a problem hiding this comment.
New test classes are added under a disabled TestNG block, so they won’t execute.
Line 67 and Line 68 are inside a <test ... enabled="false"> section (Line 64). This prevents the new internal consent update tests from running.
🛠️ Proposed fix options
- <test name="Consent Flow Types Validation Flow" parallel="tests" enabled="false" group-by-instances="true">
+ <test name="Consent Flow Types Validation Flow" parallel="tests" enabled="true" group-by-instances="true">Or move InternalConsentManageFlowValidationTest under an already enabled <test> block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/resources/testng.xml`
around lines 67 - 68, The two new test classes
ConsentManageEndpointValidationTest and InternalConsentManageFlowValidationTest
are defined inside a disabled TestNG <test enabled="false"> block so they won't
run; fix by either removing or setting enabled="true" on that <test> block or
move the class entry for InternalConsentManageFlowValidationTest (and/or
ConsentManageEndpointValidationTest if needed) into an existing enabled <test>
section in testng.xml so the classes are executed during the test run.
malshaniS
reviewed
Mar 18, 2026
| "validityPeriod": ${ConnectorTestConstants.expirationInstant.toEpochSecond()}, | ||
| "recurringIndicator": true, | ||
| "consentFrequency": 0, | ||
| "receipt": "{\\"Data\\": {\\"Permissions\\": [\\"ReadAccountsBasic\\",\\"ReadAccountsDetail\\",\\"ReadBalances\\"],\\"ExpirationDateTime\\": \\"2026-03-17T15:43:35.946770+05:30\\",\\"TransactionFromDateTime\\": \\"2026-03-12T15:43:35.947399+05:30\\",\\"TransactionToDateTime\\": \\"2026-03-15T15:43:35.947514+05:30\\"},\\"Risk\\": { }}", |
Contributor
There was a problem hiding this comment.
Shall we use the datetime variables defined at the beginning of the class without hard coding them. This may fail after sometime.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy (1)
78-79:⚠️ Potential issue | 🟡 MinorAssert list non-emptiness before indexing into
authorizationResources/resources.
Assert.assertNotNull(...)is present, but.get(0)is used without checking emptiness. If the API returns[], these tests fail withIndexOutOfBoundsExceptioninstead of a meaningful assertion failure.✅ Fix pattern to apply at each occurrence
List<JSONObject> authorizationResources = extractListFromResponse(consentResponse, "authorizationResources") +Assert.assertFalse(authorizationResources.isEmpty(), "authorizationResources is empty") authIdBeforeUpdate = authorizationResources.get(0).get("authorizationID") List<JSONObject> resources = authorizationResources.get(0).get("resources") as List<JSONObject> Assert.assertNotNull(resources) +Assert.assertFalse(resources.isEmpty(), "resources is empty") mappingIdBeforeUpdate = resources.get(0).get("mappingID")Also applies to: 92-93, 117-121, 134-139, 320-321, 333-334, 354-355, 367-368, 388-389, 401-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy` around lines 78 - 79, The test indexes into authorizationResources (produced by extractListFromResponse(consentResponse, "authorizationResources")) with .get(0) without asserting the list is non-empty; update each occurrence (e.g., where authIdBeforeUpdate is assigned) to first assert the list size > 0 (or !isEmpty()) using Assert.assertFalse/Assert.assertTrue with a clear message, and only then call .get(0) to avoid IndexOutOfBoundsException and produce a meaningful assertion failure.
🧹 Nitpick comments (2)
fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy (1)
70-77: Extract common consent-response assertions into a helper.The same assertion set is repeated across many tests (
updatedTime,consentID,clientID,authorizationResources, status). A shared helper will reduce maintenance overhead and keep failures more consistent.Also applies to: 88-95, 111-116, 130-137, 327-335, 361-369, 395-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy` around lines 70 - 77, Multiple tests duplicate the same set of assertions on consentResponse; extract them into a reusable helper to reduce duplication. Add a private helper method in InternalConsentManageFlowValidationTest (e.g., assertCommonConsentResponse(Response consentResponse, String expectedStatus)) that checks non-null for updatedTime, consentID, clientID, authorizationResources and verifies status equals expectedStatus and status code 200 and consentId presence; then replace each repeated block (including the occurrences around lines 70–77, 88–95, 111–116, 130–137, 327–335, 361–369, 395–403) with a single call to this new helper, passing the appropriate expected status like "AwaitingAuthorisation".fs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovy (1)
171-171: CentralizereceiptJSON construction to reduce escaping errors and drift.The same escaped
receiptstring is duplicated across methods. A shared builder (Map +JsonOutput) would make these payloads safer and easier to maintain.Also applies to: 211-211, 224-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovy` at line 171, Replace the repeated escaped JSON string for the "receipt" field with a single builder method in AccountsRequestPayloads that creates a Map (with keys "Data" -> ["Permissions", "ExpirationDateTime", "TransactionFromDateTime", "TransactionToDateTime"] and "Risk" -> [:]) and serializes it using groovy.json.JsonOutput.toJson, injecting ConnectorTestConstants.expirationInstant / fromInstant / toInstant; update all places that currently embed the escaped string (including the occurrences at lines similar to 171, 211, 224) to call this new method so the payloads are built programmatically and avoid manual escaping/drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@fs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovy`:
- Around line 194-200: The JSON payload strings returned by
getAccountConsentStatusUpdatePayload(String consentId) and
getAccountBasicConsentUpdatePayload(...) include trailing commas before the
closing brace which produces invalid JSON; update both methods (remove the
trailing comma after the last property in each triple-quoted JSON string—e.g.,
remove the comma after "status": "Rejected" in
getAccountConsentStatusUpdatePayload and remove the trailing comma after the
final field in getAccountBasicConsentUpdatePayload) so the returned strings are
valid JSON.
---
Duplicate comments:
In
`@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy`:
- Around line 78-79: The test indexes into authorizationResources (produced by
extractListFromResponse(consentResponse, "authorizationResources")) with .get(0)
without asserting the list is non-empty; update each occurrence (e.g., where
authIdBeforeUpdate is assigned) to first assert the list size > 0 (or
!isEmpty()) using Assert.assertFalse/Assert.assertTrue with a clear message, and
only then call .get(0) to avoid IndexOutOfBoundsException and produce a
meaningful assertion failure.
---
Nitpick comments:
In
`@fs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovy`:
- Line 171: Replace the repeated escaped JSON string for the "receipt" field
with a single builder method in AccountsRequestPayloads that creates a Map (with
keys "Data" -> ["Permissions", "ExpirationDateTime", "TransactionFromDateTime",
"TransactionToDateTime"] and "Risk" -> [:]) and serializes it using
groovy.json.JsonOutput.toJson, injecting
ConnectorTestConstants.expirationInstant / fromInstant / toInstant; update all
places that currently embed the escaped string (including the occurrences at
lines similar to 171, 211, 224) to call this new method so the payloads are
built programmatically and avoid manual escaping/drift.
In
`@fs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy`:
- Around line 70-77: Multiple tests duplicate the same set of assertions on
consentResponse; extract them into a reusable helper to reduce duplication. Add
a private helper method in InternalConsentManageFlowValidationTest (e.g.,
assertCommonConsentResponse(Response consentResponse, String expectedStatus))
that checks non-null for updatedTime, consentID, clientID,
authorizationResources and verifies status equals expectedStatus and status code
200 and consentId presence; then replace each repeated block (including the
occurrences around lines 70–77, 88–95, 111–116, 130–137, 327–335, 361–369,
395–403) with a single call to this new helper, passing the appropriate expected
status like "AwaitingAuthorisation".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a898d390-9883-4703-bba0-d3869b0aa540
📒 Files selected for processing (2)
fs-integration-test-suite/accelerator-test-framework/src/main/groovy/org/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovyfs-integration-test-suite/accelerator-tests/is-tests/consent-management/src/test/groovy/org/wso2/financial/services/accelerator/is/test/consent/management/ConsentOperationsFlowTests/InternalConsentManageFlowValidationTest.groovy
...g/wso2/financial/services/accelerator/test/framework/constant/AccountsRequestPayloads.groovy
Show resolved
Hide resolved
malshaniS
approved these changes
Mar 19, 2026
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.
Adding internal consent update API tests
Issue link: required
Doc Issue: Optional, link issue from documentation repository
Applicable Labels: Spec, product, version, type (specify requested labels)
Development Checklist
Testing Checklist
Summary by CodeRabbit
New Features
Tests