[PM-27862] Fix skipped memory storage tests #18168
Conversation
This commit fixes the tests that were skipped as part of #17245.
|
Claude finished @dereknance's task in 2m 8s —— View job Overall Assessment: APPROVE This PR correctly fixes the skipped memory storage tests from #17245 by completing the mock port implementation and using the appropriate Jest cleanup method. Code Review DetailsChanges Reviewed:
Analysis: The root cause of the test failures was that PR #17245 added The change from
No issues found. The changes are minimal, targeted, and correctly address the test failures. 🤖 Generated with Claude Code Co-Authored-By: Claude Sonnet 4.5 [email protected] |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18168 +/- ##
==========================================
+ Coverage 42.27% 42.32% +0.05%
==========================================
Files 3599 3599
Lines 104548 104548
Branches 15780 15780
==========================================
+ Hits 44194 44251 +57
+ Misses 58473 58412 -61
- Partials 1881 1885 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changes SummaryThis PR re-enables previously skipped memory storage interaction tests by fixing the mock port utility. The fix addresses issues introduced in PR #17245 by adding the missing Type: bugfix Components Affected: browser/spec/mock-port.spec-util, browser/platform/storage/memory-storage-service-interactions.spec Files Changed
Risk Areas: Test isolation: Switching from Suggestions
Full review in progress... | Powered by diffray |
| (chrome.runtime.connect as jest.Mock).mockImplementation((portInfo) => { | ||
| const port = mockDeep<chrome.runtime.Port>(); | ||
| port.name = portInfo.name; | ||
| port.sender = { url: chrome.runtime.getURL("") }; | ||
| port.sender = { | ||
| url: chrome.runtime.getURL(""), | ||
| origin: chrome.runtime.getURL(""), | ||
| }; |
There was a problem hiding this comment.
🟠 HIGH - JSDoc parameters do not match function signature
Agent: react
Category: docs
Description:
The JSDoc comment documents @param name and @param immediateOnConnectExecution, but the mockPorts() function takes no parameters. This creates misleading documentation.
Suggestion:
Remove the @param tags from the JSDoc since the function takes no parameters, or add the documented parameters to the function signature if they were intended.
Confidence: 95%
Rule: ts_always_use_jsdoc_for_documentation
Review ID: 17ab85c0-047a-4959-8626-3febe24d50d5
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| }; | ||
|
|
||
| // convert to internal port | ||
| delete (port as any).tab; |
There was a problem hiding this comment.
🟡 MEDIUM - Multiple uses of 'any' type assertion for port deletion
Agent: react
Category: quality
Description:
Lines 21-24 use 'as any' type assertions to delete properties from the port object. This defeats type safety.
Suggestion:
Use a more specific type assertion like 'delete (port as Partial<chrome.runtime.Port>).tab' or create a properly typed test utility.
Confidence: 75%
Rule: ts_prefer_specific_types_over_any_unknown_w
Review ID: 17ab85c0-047a-4959-8626-3febe24d50d5
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| test.each(["has", "get"])( |
There was a problem hiding this comment.
🟠 HIGH - Missing edge case tests for null/undefined values
Agent: testing
Category: quality
Description:
The parametrized test for 'has' and 'get' actions only tests with a simple string value. Missing tests for null, undefined, empty string edge cases.
Suggestion:
Add additional test cases covering edge cases: null values, undefined, empty strings to verify the service handles these appropriately
Confidence: 75%
Rule: test_js_missing_edge_cases
Review ID: 17ab85c0-047a-4959-8626-3febe24d50d5
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
|
|
||
| // These are succeeding individually but failing in a batch run - skipping for now | ||
| describe.skip("foreground background memory storage interaction", () => { | ||
| describe("foreground background memory storage interaction", () => { |
There was a problem hiding this comment.
🟡 MEDIUM - Missing error condition tests
Agent: testing
Category: quality
Description:
The test suite only covers happy paths. No tests for error conditions such as rejected promises or service failures.
Suggestion:
Add tests using mockRejectedValue() to cover error scenarios and verify error propagation
Confidence: 70%
Rule: test_js_missing_edge_cases
Review ID: 17ab85c0-047a-4959-8626-3febe24d50d5
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 10 issues: 5 kept (documentation bug, quality concerns, testing gaps), 5 filtered (low confidence, minor impact, or inaccurate analysis) Issues Found: 5💬 See 4 individual line comment(s) for details. 📊 4 unique issue type(s) across 5 location(s) 📋 Full issue list (click to expand)🟠 HIGH - JSDoc parameters do not match function signatureAgent: react Category: docs File: Description: The JSDoc comment documents @param name and @param immediateOnConnectExecution, but the mockPorts() function takes no parameters. This creates misleading documentation. Suggestion: Remove the @param tags from the JSDoc since the function takes no parameters, or add the documented parameters to the function signature if they were intended. Confidence: 95% Rule: 🟠 HIGH - Missing edge case tests for null/undefined values (2 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Multiple uses of 'any' type assertion for port deletionAgent: react Category: quality File: Description: Lines 21-24 use 'as any' type assertions to delete properties from the port object. This defeats type safety. Suggestion: Use a more specific type assertion like 'delete (port as Partial<chrome.runtime.Port>).tab' or create a properly typed test utility. Confidence: 75% Rule: 🟡 MEDIUM - Private property access breaks encapsulationAgent: quality Category: quality File: Description: The test accesses private _port property using bracket notation (secondForeground['_port']) to bypass TypeScript's access modifiers. This makes the test brittle to internal refactoring. Suggestion: Expose the port as a protected/internal property for testing, or refactor to test behavior only via public APIs. Confidence: 70% Rule: ℹ️ 1 issue(s) outside PR diff (click to expand)
🟡 MEDIUM - Private property access breaks encapsulationAgent: quality Category: quality File: Description: The test accesses private _port property using bracket notation (secondForeground['_port']) to bypass TypeScript's access modifiers. This makes the test brittle to internal refactoring. Suggestion: Expose the port as a protected/internal property for testing, or refactor to test behavior only via public APIs. Confidence: 70% Rule: Review ID: |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27862
📔 Objective
This PR is a follow-up to #17245 for fixing the memory storage tests that were skipped as part of those changes.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes