Add edge case tests for NamespaceService.loadNamespaceBO#5574
Add edge case tests for NamespaceService.loadNamespaceBO#5574mergify[bot] merged 3 commits intoapolloconfig:masterfrom
Conversation
…spaceBO covering deleted items, namespace not found, missing release scenarios, and public/private namespace resolution
dosubot
bot
added
the
size:L
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded ~15 new unit tests in NamespaceServiceTest exercising NamespaceService.loadNamespaceBO across deleted-item handling, absence of namespace/releases, empty items, public/private namespace relationships, dirty app namespace states, and modified item count calculations (tests only; no production API changes). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java (1)
46-53:⚠️ Potential issue | 🔴 CriticalMissing imports cause compilation failure.
The new tests use
assertNotNull,assertTrue, andBadRequestExceptionwhich are not imported. Add the following imports to resolve the compilation errors:🔧 Proposed fix to add missing imports
import static org.assertj.core.api.AssertionsForClassTypes.assertThat; import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when;Also add the
BadRequestExceptionclass import:import com.ctrip.framework.apollo.portal.entity.bo.NamespaceBO; import com.ctrip.framework.apollo.portal.entity.bo.UserInfo; +import com.ctrip.framework.apollo.common.exception.BadRequestException; import com.ctrip.framework.apollo.portal.spi.UserInfoHolder;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java` around lines 46 - 53, The test class NamespaceServiceTest is missing imports that cause compilation failures; add static imports for assertNotNull and assertTrue (e.g., import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue;) and add the import for the BadRequestException class used by the test (import the same BadRequestException type referenced in NamespaceServiceTest) so the references to assertNotNull, assertTrue, and BadRequestException resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java`:
- Line 313: The tests call NamespaceService.loadNamespaceBO with an incorrect
5-argument signature; replace those calls with the correct overloads on
NamespaceService.loadNamespaceBO: either the 4-arg version
(loadNamespaceBO(appId, env, clusterName, namespaceName)) or the 6-arg version
(loadNamespaceBO(appId, env, clusterName, namespaceName, fillItemDetail,
includeDeletedItems)). For testLoadNamespaceBOWithDeletedItems pass
includeDeletedItems=true (e.g. fillItemDetail=true, includeDeletedItems=true);
for testLoadNamespaceBOWithoutDeletedItems pass includeDeletedItems=false (e.g.
fillItemDetail=true, includeDeletedItems=false); and for
testLoadNamespaceBOItemModifiedCountCalculation choose the 4-arg or 6-arg
overload as appropriate for that test's expectations.
- Around line 296-458: Spotless formatting violations detected in
NamespaceServiceTest - run the formatter. Fix by running the Spotless formatter
(./mvnw spotless:apply) locally and commit the resulting whitespace/formatting
changes; ensure the test class NamespaceServiceTest and its test methods (e.g.,
testLoadNamespaceBOWithDeletedItems, testLoadNamespaceBOWithoutDeletedItems,
etc.) are formatted according to project rules, then re-run tests and push the
formatted changes.
- Around line 383-387: The test in NamespaceServiceTest assumes a deterministic
order when checking namespaceBO.getItems().get(0).getItem().getKey(); instead,
make the assertion order-independent by inspecting the collection produced by
parseDeletedItems/releaseItems.entrySet(): for example, assert that
namespaceBO.getItems() contains an element whose item.getKey() equals "k1" and
isDeleted() is true (or extract keys into a Set and assert it contains "k1"), or
sort namespaceBO.getItems() by item.getKey() before asserting the first element;
update the assertion that currently references namespaceBO.getItems().get(0)
accordingly.
---
Outside diff comments:
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java`:
- Around line 46-53: The test class NamespaceServiceTest is missing imports that
cause compilation failures; add static imports for assertNotNull and assertTrue
(e.g., import static org.junit.Assert.assertNotNull; import static
org.junit.Assert.assertTrue;) and add the import for the BadRequestException
class used by the test (import the same BadRequestException type referenced in
NamespaceServiceTest) so the references to assertNotNull, assertTrue, and
BadRequestException resolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19ff8c84-2606-4a24-b50d-4c9bac74c91b
📒 Files selected for processing (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java
Show resolved
Hide resolved
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java
Outdated
Show resolved
Hide resolved
| assertNotNull(namespaceBO); | ||
| assertEquals(3, namespaceBO.getItems().size()); | ||
| assertTrue(namespaceBO.getItems().get(0).isDeleted()); | ||
| assertEquals("k1", namespaceBO.getItems().get(0).getItem().getKey()); | ||
| } |
There was a problem hiding this comment.
Fragile assertion depends on non-deterministic iteration order.
The assertion at line 386 (assertEquals("k1", namespaceBO.getItems().get(0).getItem().getKey())) assumes k1 is the first deleted item. However, parseDeletedItems iterates over releaseItems.entrySet() (a HashMap), whose iteration order is not guaranteed. This test may pass or fail depending on JVM/runtime behavior.
🔧 Proposed fix to use order-independent assertion
assertNotNull(namespaceBO);
assertEquals(3, namespaceBO.getItems().size());
- assertTrue(namespaceBO.getItems().get(0).isDeleted());
- assertEquals("k1", namespaceBO.getItems().get(0).getItem().getKey());
+ // All items should be marked as deleted since items list is empty
+ assertTrue(namespaceBO.getItems().stream().allMatch(item -> item.isDeleted()));
+ // Verify expected keys are present (order is non-deterministic)
+ List<String> keys = namespaceBO.getItems().stream()
+ .map(item -> item.getItem().getKey())
+ .collect(Collectors.toList());
+ assertThat(keys).asList().containsExactlyInAnyOrder("k1", "k2", "k3");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertNotNull(namespaceBO); | |
| assertEquals(3, namespaceBO.getItems().size()); | |
| assertTrue(namespaceBO.getItems().get(0).isDeleted()); | |
| assertEquals("k1", namespaceBO.getItems().get(0).getItem().getKey()); | |
| } | |
| assertNotNull(namespaceBO); | |
| assertEquals(3, namespaceBO.getItems().size()); | |
| // All items should be marked as deleted since items list is empty | |
| assertTrue(namespaceBO.getItems().stream().allMatch(item -> item.isDeleted())); | |
| // Verify expected keys are present (order is non-deterministic) | |
| Set<String> keys = namespaceBO.getItems().stream() | |
| .map(item -> item.getItem().getKey()) | |
| .collect(Collectors.toSet()); | |
| assertEquals(Set.of("k1", "k2", "k3"), keys); | |
| } |
🧰 Tools
🪛 GitHub Actions: build
[error] 383-383: Compilation error: cannot find symbol method assertNotNull(com.ctrip.framework.apollo.portal.entity.bo.NamespaceBO).
[error] 385-385: Compilation error: cannot find symbol method assertTrue(boolean).
🪛 GitHub Actions: portal-login-e2e
[error] 383-383: Cannot find symbol: method assertNotNull(NamespaceBO).
[error] 385-385: Cannot find symbol: method assertTrue(boolean).
🪛 GitHub Actions: portal-ui-e2e
[error] 385-385: Cannot resolve symbol: NamespaceBO in assertion context (NamespaceServiceTest).
[error] 385-385: Cannot resolve symbol: assertTrue(boolean) in test code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java`
around lines 383 - 387, The test in NamespaceServiceTest assumes a deterministic
order when checking namespaceBO.getItems().get(0).getItem().getKey(); instead,
make the assertion order-independent by inspecting the collection produced by
parseDeletedItems/releaseItems.entrySet(): for example, assert that
namespaceBO.getItems() contains an element whose item.getKey() equals "k1" and
isDeleted() is true (or extract keys into a Set and assert it contains "k1"), or
sort namespaceBO.getItems() by item.getKey() before asserting the first element;
update the assertion that currently references namespaceBO.getItems().get(0)
accordingly.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java (1)
46-52:⚠️ Potential issue | 🟡 MinorRun Spotless on this file before merging.
spotless:checkis still failing on the updated imports/tests block, so this PR remains red until the formatter is applied.As per coding guidelines, "Run
./mvnw spotless:applyto format code and must be run before opening a PR".Also applies to: 300-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java` around lines 46 - 52, The import/test block in NamespaceServiceTest is not formatted according to Spotless; run the formatter and reapply import ordering to satisfy spotless:apply. Fix by running ./mvnw spotless:apply (or your IDE's Spotless plugin), ensure import ordering and static-import grouping in the NamespaceServiceTest class are corrected, re-run spotless:check, and commit the formatted changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java`:
- Around line 359-365: The tests (testLoadNamespaceBONoLatestRelease,
testLoadNamespaceBOWithPublicNamespace, testLoadNamespaceBOWithPrivateNamespace,
testLoadNamespaceBOWithDirtyAppNamespace) call
NamespaceService.loadNamespaceBO(...) which by default includes deleted items
and causes transformNamespace2BO(...) to call itemService.findDeletedItems(...);
stub that interaction in each affected test to return Collections.emptyList()
(or disable deleted-items behavior) so the mock does not return null; update the
mocked behavior for itemService.findDeletedItems(testAppId, testEnv,
testClusterName, testNamespaceName) in those tests to return an empty list.
---
Duplicate comments:
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java`:
- Around line 46-52: The import/test block in NamespaceServiceTest is not
formatted according to Spotless; run the formatter and reapply import ordering
to satisfy spotless:apply. Fix by running ./mvnw spotless:apply (or your IDE's
Spotless plugin), ensure import ordering and static-import grouping in the
NamespaceServiceTest class are corrected, re-run spotless:check, and commit the
formatted changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b12f9ba2-7306-43b9-ad9e-9a52d737a3f4
📒 Files selected for processing (1)
apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java
| public void testLoadNamespaceBONoLatestRelease() { | ||
| when(namespaceAPI.loadNamespace(testAppId, testEnv, testClusterName, testNamespaceName)).thenReturn(createNamespace(testAppId, testClusterName, testNamespaceName)); | ||
| when(releaseService.loadLatestRelease(testAppId, testEnv, testClusterName, testNamespaceName)).thenReturn(null); | ||
| when(appNamespaceService.findByAppIdAndName(testAppId, testNamespaceName)).thenReturn(createAppNamespace(testAppId, testNamespaceName, false)); | ||
| when(itemService.findItems(testAppId, testEnv, testClusterName, testNamespaceName)).thenReturn(createItems()); | ||
|
|
||
| NamespaceBO namespaceBO = namespaceService.loadNamespaceBO(testAppId, testEnv, testClusterName, testNamespaceName); |
There was a problem hiding this comment.
Stub deleted items or disable them in these tests.
These methods call the 4-arg loadNamespaceBO(...), which defaults includeDeletedItems to true. transformNamespace2BO(...) then does itemService.findDeletedItems(...).stream(), so the unstubbed mock returns null and these tests fail before the assertions run.
🔧 Minimal fix
- NamespaceBO namespaceBO = namespaceService.loadNamespaceBO(testAppId, testEnv, testClusterName, testNamespaceName);
+ NamespaceBO namespaceBO =
+ namespaceService.loadNamespaceBO(
+ testAppId, testEnv, testClusterName, testNamespaceName, true, false);Apply the same change in:
testLoadNamespaceBONoLatestReleasetestLoadNamespaceBOWithPublicNamespacetestLoadNamespaceBOWithPrivateNamespacetestLoadNamespaceBOWithDirtyAppNamespace
If you want to keep exercising the default overload instead, stub findDeletedItems(...) to return Collections.emptyList() in each test.
Also applies to: 394-405, 413-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apollo-portal/src/test/java/com/ctrip/framework/apollo/portal/service/NamespaceServiceTest.java`
around lines 359 - 365, The tests (testLoadNamespaceBONoLatestRelease,
testLoadNamespaceBOWithPublicNamespace, testLoadNamespaceBOWithPrivateNamespace,
testLoadNamespaceBOWithDirtyAppNamespace) call
NamespaceService.loadNamespaceBO(...) which by default includes deleted items
and causes transformNamespace2BO(...) to call itemService.findDeletedItems(...);
stub that interaction in each affected test to return Collections.emptyList()
(or disable deleted-items behavior) so the mock does not return null; update the
mocked behavior for itemService.findDeletedItems(testAppId, testEnv,
testClusterName, testNamespaceName) in those tests to return an empty list.
nobodyiam
left a comment
nobodyiam
left a comment
There was a problem hiding this comment.
LGTM.
I verified the current head 222ca2e. The added tests cover the edge cases described in the PR for deleted items, namespace-not-found, no-latest-release, public/private namespace resolution, dirty app namespace handling, and modified item count, and the checks are green.
Merge Queue Status
This pull request spent 8 seconds in the queue, with no time running CI. Required conditions to merge
|
2 participants
Summary
Add comprehensive edge case tests for NamespaceService.loadNamespaceBO covering deleted items, namespace not found, missing release scenarios, and public/private namespace resolution.
Why
Current tests cover basic functionality but do not explicitly cover boundary cases such as:
These boundary cases are easy to regress (for example, through off-by-one in item counting or grouping changes), so explicit tests improve behavioral stability and prevent future regressions.
Verification
Ran mvn -Dtest=NamespaceServiceTest test
Result: all tests passed
Summary by CodeRabbit