-
Notifications
You must be signed in to change notification settings - Fork 86
[PM-25906] Add account switcher visibility tests #2168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[PM-25906] Add account switcher visibility tests #2168
Conversation
|
Claude finished @LRNcardozoWDF's task in 1m 45s —— View job Code Review CompleteReview posted by bitwarden-code-reviewer agent. Summary: Comprehensive test coverage successfully added for iOS 26 account switcher visibility across 5 processor test files (49 new test cases). Tests follow proper patterns with iOS version gating and appropriate assertions. |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2168 +/- ##
==========================================
+ Coverage 85.39% 85.57% +0.18%
==========================================
Files 1731 1731
Lines 145718 146774 +1056
==========================================
+ Hits 124433 125606 +1173
+ Misses 21285 21168 -117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| subject.state.profileSwitcherState.isVisible = true | ||
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Test setup ordering issue: Mock data is configured AFTER the action is performed (line 173), but these values should be set up BEFORE calling perform() on line 173.
The lines 174-181 should be moved above line 172. The test setup should follow this order:
- Configure all mock dependencies and initial state
- Perform the action being tested
- Assert the results
This same issue appears in VaultAutofillListProcessorTests.swift:test_perform_profileSwitcher_accountPressed_iOS26() at line 201.
| } | ||
|
|
||
| subject.state.profileSwitcherState.isVisible = true | ||
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Test setup ordering issue: Same problem as in VaultItemSelectionProcessorTests.swift - mock data (lines 202-209) is configured AFTER the action is performed (line 201).
Move lines 202-209 above line 200 to ensure proper test setup order.
| } | ||
|
|
||
| subject.state.profileSwitcherState.isVisible = true | ||
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Critical: Test setup ordering violation
Mock data is configured AFTER the action is performed. Lines 173-180 must be moved above line 172.
Current (incorrect) order:
- Set state (line 171) ✅
- Perform action (line 172) ❌ Too early!
- Configure mocks (lines 173-180) ❌ Too late!
Correct order should be:
- Set state
- Configure all mocks (activeAccount, altAccounts, vaultTimeout)
- Then perform action
- Assert results
This follows the standard Arrange-Act-Assert pattern (Testing.md:1033-1037). Currently the test runs with uninitialized mock values, producing incorrect results.
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) | |
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) | |
| authRepository.activeAccount = .fixture(profile: .fixture(userId: "42")) | |
| authRepository.altAccounts = [ | |
| .fixture(), | |
| ] | |
| authRepository.vaultTimeout = [ | |
| "1": .fiveMinutes, | |
| "42": .immediately, | |
| ] | |
| } | ||
|
|
||
| subject.state.profileSwitcherState.isVisible = true | ||
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Critical: Test setup ordering violation
Same issue as VaultItemSelectionProcessorTests - mock data (lines 201-208) is configured AFTER the action is performed (line 200).
Move lines 201-208 above line 200 to ensure mocks are configured before the test executes.
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) | |
| await subject.perform(.profileSwitcher(.accountPressed(ProfileSwitcherItem.fixture(userId: "1")))) | |
| authRepository.activeAccount = .fixture(profile: .fixture(userId: "42")) | |
| authRepository.altAccounts = [ | |
| .fixture(), | |
| ] | |
| authRepository.vaultTimeout = [ | |
| "1": .fiveMinutes, | |
| "42": .immediately, | |
| ] | |
KatherineInCode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the whole, it looks good, aside from a small thing I mentioned. And the comments Claude generated noting the mock/test order...while it appears to be that way in the original test, I do find it curious. It might be worth adding a comment to them to explain why the order is the way it is?
| throw XCTSkip("This test requires iOS 26 or later") | ||
| } | ||
|
|
||
| subject.state.profileSwitcherState.isVisible = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Doesn't the new profile switcher obviate the need for tracking the state visibility this way?
…l-account-switcher-visibility-tests
|
Overall Assessment: APPROVE Reviewed comprehensive iOS 26 test coverage additions for account switcher visibility across 5 processor test files. Summary: Test Coverage Added:
Positive Observations:
Minor Observation (Not Blocking):
Example from VaultItemSelectionProcessorTests.swift:167-193: // Current order works but could be clearer:
subject.state.profileSwitcherState.isVisible = true // State set first
authRepository.activeAccount = ... // Then mocks
authRepository.altAccounts = ...
authRepository.vaultTimeout = ...
await subject.perform(...) // Then action
// Suggested (more explicit Arrange-Act-Assert):
authRepository.activeAccount = ... // Mocks first
authRepository.altAccounts = ...
authRepository.vaultTimeout = ...
subject.state.profileSwitcherState.isVisible = true // State second
await subject.perform(...) // Action lastThis is a stylistic preference for improved readability and doesn't affect test correctness. Recommendation: APPROVE - This PR meets quality standards and adds valuable test coverage for iOS 26 compatibility. |

🎟️ Tracking
PM-25906
📔 Objective
Add tests for account switcher for iOS 26.
📸 Screenshots
⏰ 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