Conversation
1. testStorageCacheNameCollision — proves that two
CaseInstanceTreeElement instances (one for casedb, one for results)
return the same getStorageCacheName() value ("casedb").
2. testRecordObjectCacheCrossContamination — proves that when a casedb
record is loaded into the RecordObjectCache, the results instance
retrieves that wrong record when it asks for the same row ID, getting
"person" instead of "facility".
getStorageCacheName() needs to return distinct keys for different
storage instances
| assertEquals(casedbInstance.getStorageCacheName(), resultsInstance.getStorageCacheName(), | ||
| "Both instances return the same storageCacheName, causing cache collisions"); |
There was a problem hiding this comment.
Bug: The new test CaseSearchCacheCollisionTest.java asserts known incorrect behavior, verifying that two different storage instances produce the same cache name. This test will fail when the bug is fixed.
Severity: LOW
Suggested Fix
The test should assert the correct behavior, which is that different storage instances have different cache names. Use assertNotEquals instead of assertEquals to validate that casedbInstance.getStorageCacheName() and resultsInstance.getStorageCacheName() are distinct. Alternatively, if this test is intended to be temporary, add an @Disabled or @Ignore annotation and a comment explaining that it should be updated or removed when the underlying bug is fixed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
src/test/java/org/commcare/formplayer/tests/CaseSearchCacheCollisionTest.java#L65-L66
Potential issue: The new test file `CaseSearchCacheCollisionTest.java` is designed to
pass when a known bug (cache key collision) is present. The test asserts that different
storage instances produce the same cache name, for example
`assertEquals(casedbInstance.getStorageCacheName(),
resultsInstance.getStorageCacheName())`. This is the opposite of the desired behavior.
While this test passes now, it will fail when the underlying bug is fixed, creating a
future maintenance burden as the test will need to be inverted or deleted. The test
lacks any `@Disabled` annotations or comments to indicate its temporary nature.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1771 +/- ##
============================================
- Coverage 70.17% 70.09% -0.08%
+ Complexity 2032 2029 -3
============================================
Files 257 257
Lines 8009 8009
Branches 763 763
============================================
- Hits 5620 5614 -6
- Misses 2106 2112 +6
Partials 283 283 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
getStorageCacheName() needs to return distinct keys for different storage instances
Product Description
Technical Summary
Safety Assurance
Safety story
Automated test coverage
QA Plan
Migrations
Special deploy instructions
Rollback instructions
Review