Refactor API product category test case#13976
Open
sgayangi wants to merge 1 commit intowso2:masterfrom
Open
Conversation
sgayangi
requested review from
AnuGayan,
Arshardh,
chamilaadhi and
tharindu1st
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactored test Changes
Sequence Diagram(s)(omitted — changes are test refactor and do not introduce new multi-component control flow needing a sequence diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java (1)
450-452: Resource cleanup is fragile: category deletion may be skipped on test failure.The category deletion at lines 451-452 is placed inline after the assertions. If any assertion fails between lines 404-449, the test will throw an exception and the category will not be deleted, leaving orphaned test data in the system.
Consider wrapping the test logic in a try-finally block to ensure cleanup always occurs, or use an instance variable with
@AfterMethodcleanup.🧹 Suggested pattern for reliable cleanup
+ private String apiCategoryIdToCleanup; + `@Test`(groups = {"wso2.am"}, description = "Test creation and deployment of API Product with an API " + "category") public void testCreateAndInvokeApiProductWithAPICategoryAdded() throws Exception { // Create the Marketing category first String categoryName = "Marketing-" + UUID.randomUUID().toString().substring(0, 8); String description = "Marketing category for testing"; APICategoryDTO categoryDTO = DtoFactory.createApiCategoryDTO(categoryName, description); //Add the api category ApiResponse<APICategoryDTO> addedApiCategory = restAPIAdmin.addApiCategory(categoryDTO); //Assert the status code and api category ID Assert.assertEquals(addedApiCategory.getStatusCode(), HttpStatus.SC_CREATED); APICategoryDTO addedApiCategoryDTO = addedApiCategory.getData(); String apiCategoryId = addedApiCategoryDTO.getId(); - categoryDTO.setId(apiCategoryId); + apiCategoryIdToCleanup = apiCategoryId; + try { // ... rest of the test logic ... - - ApiResponse<Void> apiResponse = restAPIAdmin.deleteApiCategory(apiCategoryId); - Assert.assertEquals(apiResponse.getStatusCode(), HttpStatus.SC_OK); + } finally { + if (apiCategoryIdToCleanup != null) { + restAPIAdmin.deleteApiCategory(apiCategoryIdToCleanup); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java` around lines 450 - 452, The test currently deletes the category inline after assertions which can be skipped if an assertion fails; ensure cleanup always runs by moving deletion into a finally block or into an `@AfterMethod` that uses an instance variable for apiCategoryId so restAPIAdmin.deleteApiCategory(apiCategoryId) is always invoked; update the test method (where apiCategoryId is set) to assign the id to a field and implement an `@AfterMethod` that calls restAPIAdmin.deleteApiCategory(apiCategoryId) and asserts the ApiResponse status (ApiResponse<Void>, deleteApiCategory) so orphaned categories are removed even on test failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java`:
- Around line 397-409: The test uses DtoFactory.createApiCategoryDTO but the
import for DtoFactory is missing and causes compilation failure; add the import
org.wso2.am.integration.test.impl.DtoFactory to the file. Also remove the dead
assignment categoryDTO.setId(apiCategoryId) (the categoryDTO object is not used
afterwards) to clean up unused code; locate the APICategoryDTO categoryDTO
declaration and the subsequent categoryDTO.setId(...) call and delete that
single assignment line.
---
Nitpick comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java`:
- Around line 450-452: The test currently deletes the category inline after
assertions which can be skipped if an assertion fails; ensure cleanup always
runs by moving deletion into a finally block or into an `@AfterMethod` that uses
an instance variable for apiCategoryId so
restAPIAdmin.deleteApiCategory(apiCategoryId) is always invoked; update the test
method (where apiCategoryId is set) to assign the id to a field and implement an
`@AfterMethod` that calls restAPIAdmin.deleteApiCategory(apiCategoryId) and
asserts the ApiResponse status (ApiResponse<Void>, deleteApiCategory) so
orphaned categories are removed even on test failures.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java
...ckend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java
Outdated
Show resolved
Hide resolved
sgayangi
force-pushed
the
devportal-categories
branch
from
01337ae to
3b9a271
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java (1)
449-451: Consider using try-finally to ensure category cleanup on test failure.The category deletion is placed at the end of the test method. If any assertion fails earlier (e.g., lines 404, 426, 435-436, 441-442, 447-448), the category won't be deleted, leaving orphaned test data.
♻️ Optional: Wrap test body in try-finally for reliable cleanup
public void testCreateAndInvokeApiProductWithAPICategoryAdded() throws Exception { // Create the Marketing category first String categoryName = "Marketing-" + UUID.randomUUID().toString().substring(0, 8); String description = "Marketing category for testing"; APICategoryDTO categoryDTO = DtoFactory.createApiCategoryDTO(categoryName, description); //Add the api category ApiResponse<APICategoryDTO> addedApiCategory = restAPIAdmin.addApiCategory(categoryDTO); //Assert the status code and api category ID Assert.assertEquals(addedApiCategory.getStatusCode(), HttpStatus.SC_CREATED); APICategoryDTO addedApiCategoryDTO = addedApiCategory.getData(); String apiCategoryId = addedApiCategoryDTO.getId(); + try { String provider = user.getUserName(); // ... rest of test logic ... Assert.assertTrue(categoriesInReceivedAPI.contains(categoryName)); + } finally { + ApiResponse<Void> apiResponse = restAPIAdmin.deleteApiCategory(apiCategoryId); + Assert.assertEquals(apiResponse.getStatusCode(), HttpStatus.SC_OK); + } - - ApiResponse<Void> apiResponse = restAPIAdmin.deleteApiCategory(apiCategoryId); - Assert.assertEquals(apiResponse.getStatusCode(), HttpStatus.SC_OK); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java` around lines 449 - 451, Test currently deletes the API category only at the end, risking orphaned data if earlier assertions fail; wrap the test logic in a try-finally so cleanup always runs. Move the body that makes assertions into a try block and perform the call to restAPIAdmin.deleteApiCategory(apiCategoryId) and the Assert on apiResponse.getStatusCode() inside a finally block (or call deleteApiCategory inside finally and assert its status there), referencing the existing apiCategoryId variable and deleteApiCategory / ApiResponse<Void> apiResponse to ensure the category is removed even when the test fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java`:
- Around line 449-451: Test currently deletes the API category only at the end,
risking orphaned data if earlier assertions fail; wrap the test logic in a
try-finally so cleanup always runs. Move the body that makes assertions into a
try block and perform the call to restAPIAdmin.deleteApiCategory(apiCategoryId)
and the Assert on apiResponse.getStatusCode() inside a finally block (or call
deleteApiCategory inside finally and assert its status there), referencing the
existing apiCategoryId variable and deleteApiCategory / ApiResponse<Void>
apiResponse to ensure the category is removed even when the test fails.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
all-in-one-apim/modules/integration/tests-integration/tests-backend/src/test/java/org/wso2/am/integration/tests/apiproduct/APIProductCreationTestCase.java
sgayangi
changed the title
sgayangi
force-pushed
the
devportal-categories
branch
from
3b9a271 to
0c88bdf
Compare
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.
Added refactoring for #13943
Summary by CodeRabbit