test(java-sdk): improve tests on liststores #642
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds new Java test methods to validate ListStores filtering by name across API and client layers, including integration and unit tests. Updates OpenFgaApiTest with Mockito stubs for configuration override and validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant C as Client
participant API as OpenFGA API
Note over T,C: ListStores with optional name filter
T->>C: listStores(options{name, pageSize, token})
C->>API: GET /stores?name={name}&page_size={n}&continuation_token={t}
API-->>C: 200 OK + stores[]
C-->>T: stores[]
rect rgba(230,245,255,0.5)
Note right of C: If ConfigurationOverride provided
T->>C: listStores(options, override)
C->>C: apply override
C->>API: GET with override-config
API-->>C: 200 OK
C-->>T: stores[]
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (6)
config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache (1)
309-335: Good coverage; watch out for brittle URL ordering and encodingExact URL matching can flake if query param order changes or values need encoding. Consider making the verification order‑agnostic (if the mock supports it) or encoding values in getUrl to match production.
config/clients/java/template/src/test/api/OpenFgaApiTest.java.mustache (5)
7-7: Redundant Mockito importsYou import both static any and ArgumentMatchers class. Prefer one style for consistency (use static any everywhere or qualify with ArgumentMatchers.any).
25-25: Avoid mixed usage of ArgumentMatchersSince you already statically import any, you can drop the direct ArgumentMatchers import and call any(ConfigurationOverride.class) directly.
- import org.mockito.ArgumentMatchers; + // use static any(...) already imported
69-72: Good stubs; consider verifying interactionsYou stub override(...) and assertValid(). In the test that uses a ConfigurationOverride, add verifies to assert these are called, guarding against regressions.
// in listStoresTest_withConfigurationOverride THEN block + verify(mockConfiguration).override(configOverride); + verify(mockConfiguration).assertValid();
129-153: Strengthen assertion to match filtered nameYou filter by storeName but assert DEFAULT_STORE_NAME from the stubbed body. Align the stub and expectation with storeName to validate end‑to‑end.
- String responseBody = - String.format("{\"stores\":[{\"id\":\"%s\",\"name\":\"%s\"}]}", DEFAULT_STORE_ID, DEFAULT_STORE_NAME); + String responseBody = + String.format("{\"stores\":[{\"id\":\"%s\",\"name\":\"%s\"}]}", DEFAULT_STORE_ID, storeName); ... - assertEquals(DEFAULT_STORE_NAME, response.getData().getStores().get(0).getName()); + assertEquals(storeName, response.getData().getStores().get(0).getName());
154-179: Verify override path and consider URL brittlenessTest looks good. Add verify(mockConfiguration).override(configOverride) (and assertValid) to ensure the override path executes. Also, exact URL matching can break if query param order changes; keep values URL‑safe or adjust matcher if possible.
// Then mockHttpClient.verify().get("https://api.fga.example/stores").called(1); + verify(mockConfiguration).override(configOverride); + verify(mockConfiguration).assertValid();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/clients/java/template/src/test-integration/api/OpenFgaApiIntegrationTest.java.mustache(1 hunks)config/clients/java/template/src/test-integration/api/client/OpenFgaClientIntegrationTest.java.mustache(1 hunks)config/clients/java/template/src/test/api/OpenFgaApiTest.java.mustache(4 hunks)config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
config/**/*.mustache
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Validate mustache syntax and variable references across all template files, including CHANGELOG.md.mustache
Files:
config/clients/java/template/src/test-integration/api/client/OpenFgaClientIntegrationTest.java.mustacheconfig/clients/java/template/src/test-integration/api/OpenFgaApiIntegrationTest.java.mustacheconfig/clients/java/template/src/test/api/OpenFgaApiTest.java.mustacheconfig/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache
config/**/*.{json,mustache}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never hardcode API keys or credentials in configuration or template files
Files:
config/clients/java/template/src/test-integration/api/client/OpenFgaClientIntegrationTest.java.mustacheconfig/clients/java/template/src/test-integration/api/OpenFgaApiIntegrationTest.java.mustacheconfig/clients/java/template/src/test/api/OpenFgaApiTest.java.mustacheconfig/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache
⏰ 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). (3)
- GitHub Check: build-and-test-go-sdk
- GitHub Check: build-and-test-java-sdk
- GitHub Check: build-and-test-dotnet-sdk
🔇 Additional comments (3)
config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache (1)
11-11: Import is correct and usedanyInt is needed for mocking Executors; good addition.
config/clients/java/template/src/test-integration/api/OpenFgaApiIntegrationTest.java.mustache (1)
118-141: Nice integration coverage for name filterCreates multiple stores and validates exact‑name filtering returns a single match; looks good.
config/clients/java/template/src/test-integration/api/client/OpenFgaClientIntegrationTest.java.mustache (1)
134-159: Solid client-level test for name filterMirrors API integ path; assertions are correct and concise.
ewanharris
force-pushed
the
test/list-store-tests
branch
from
b5d7d26 to
052b8a8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enhances test coverage for the listStores functionality in the Java SDK by adding tests for name-based filtering and pagination parameters. The changes validate that store listing correctly supports exact name matching, returns only matching stores, and properly handles various parameter combinations.
Key Changes:
- Added integration tests confirming name-based filtering returns only the target store
- Extended unit tests to cover all parameter combinations (pageSize, continuationToken, name)
- Added tests verifying configuration override behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
OpenFgaClientTest.java.mustache |
Added unit test for listStores with all parameters (pageSize, continuationToken, name) |
OpenFgaApiTest.java.mustache |
Added unit tests for name-only filtering and configuration override scenarios |
OpenFgaClientIntegrationTest.java.mustache |
Added integration test verifying exact name filtering returns single matching store |
OpenFgaApiIntegrationTest.java.mustache |
Added integration test validating name-based filtering at API layer |
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.Mockito.*; | ||
| import static org.mockito.ArgumentMatchers.anyInt; |
There was a problem hiding this comment.
The import anyInt is added but never used in this file. Consider removing unused imports to keep the code clean.
| import static org.mockito.ArgumentMatchers.anyInt; |
|
|
||
| import static org.hamcrest.Matchers.*; | ||
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.mockito.ArgumentMatchers.any; |
There was a problem hiding this comment.
The static import for any is added but appears unused in the test methods. The code uses ArgumentMatchers.any() with the full qualifier instead. Either use the static import or remove it.
| import static org.mockito.ArgumentMatchers.any; |
|
Closing because #638 makes this unnecessary 🎉 |
Description
Copies across some extra tests for the list stores by name in Java
References
openfga/java-sdk#195 / openfga/java-sdk#237
Review Checklist
mainSummary by CodeRabbit