Refactor UI tests to use app extension helpers and improve test stability#2
Conversation
…d optimize Home performance - Create UITestHelpers.swift with shared SeedContent, XCUIApplication navigation extensions, and screenshot convenience — removes ~470 lines of duplication across 4 test files - Replace all 25+ sleep() calls in E2E tests with proper waitForExistence() to make tests faster and less flaky - Fix HomeScreen onAppear re-shuffling quick cards on every tab switch by tracking initial load state and only refreshing mutable sections (checklists, inventory, notes) on tab re-entry - Extract ConnectivityNoticePresenter (@observable) from duplicated observe/handle/present/auto-dismiss pattern shared by HomeScreen and SettingsScreen (~60 lines removed per screen) - Optimize evaluateSupplyReadiness with Dictionary-based category lookup (O(1) vs O(n) per template item), pre-computed expiry threshold, and pre-lowercased keywords https://claude.ai/code/session_01XrVM9UFJFH5rcwmcW4tcyx
Codacy's Analysis Summary0 new issue (≤ 0 issue)
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR introduces significant production refactoring alongside its stated goal of UI test stabilization, which increases its risk profile. While the test helpers and UI stability improvements are valuable, the PR is currently not up to standards due to increased complexity in screen connectivity logic and a lack of unit tests for new performance-critical code in HomeSupport and ConnectivityNoticePresenter. High-risk files like HomeScreen.swift and SettingsScreen.swift have been flagged for high cyclomatic complexity and lack corresponding test coverage. Before merging, the medium-severity issues regarding navigation helper assertions and the lack of verification for inventory readiness logic must be addressed.
About this PR
- The inventory lookup optimization in HomeSupport (O(1) dictionary grouping) is particularly sensitive and lacks unit tests to verify edge cases like empty categories or keyword mismatches. New production logic should be backed by unit tests, not just UI tests.
- This PR has mixed scope: it contains sensitive production logic refactoring (HomeSupport performance, ConnectivityNoticePresenter) alongside UI test helpers. Ensure this is intentional as it increases the risk beyond just 'test stability'.
Test suggestions
- UI navigation using new extensions (tapTab, navigateToMoreItem, openLibraryChapter)
- Auto-dismiss logic of ConnectivityNoticePresenter after the 4-second timeout
- Inventory readiness evaluation correctness post-optimization (HomeSupport)
- HomeScreen reload logic correctly skips randomized sections on subsequent appearances
- Quick card finding helpers using consolidated SeedContent list
- Unit tests for O(1) inventory lookup optimization in HomeSupport
- Unit tests for ConnectivityNoticePresenter state transitions
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Auto-dismiss logic of ConnectivityNoticePresenter after the 4-second timeout
2. Inventory readiness evaluation correctness post-optimization (HomeSupport)
3. Unit tests for O(1) inventory lookup optimization in HomeSupport
4. Unit tests for ConnectivityNoticePresenter state transitions
🗒️ Improve review quality by adding custom instructions
| } | ||
|
|
||
| private func settingsConnectivityNotice( | ||
| private static func settingsConnectivityNotice( |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Similar to the home screen, the settingsConnectivityNotice method is overly complex at a CCN of 9. Extract the logic for determining the ConnectivityStatusNotice into a dedicated mapper or use a switch statement on the state transitions to improve maintainability.
| } | ||
|
|
||
| private func homeConnectivityNotice( | ||
| private static func homeConnectivityNotice( |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The method homeConnectivityNotice has a cyclomatic complexity of 9 (threshold 8). Given HomeScreen.swift is flagged as an uncovered complex file, this logic should be simplified to reduce maintenance risk. Refactor to use a switch statement or a declarative mapping for connectivity state transitions.
| func tapTab(_ name: String) { | ||
| let button = tabBars.firstMatch.buttons[name] | ||
| if button.waitForExistence(timeout: 3) { | ||
| button.tap() | ||
| } | ||
| } | ||
|
|
||
| /// Navigates into a More tab list item by label. | ||
| @MainActor | ||
| func navigateToMoreItem(_ label: String) { | ||
| tapTab("More") | ||
|
|
||
| let item = staticTexts[label] | ||
| if item.waitForExistence(timeout: 3) { | ||
| item.tap() | ||
| return | ||
| } | ||
|
|
||
| let button = buttons[label] | ||
| if button.waitForExistence(timeout: 2) { | ||
| button.tap() | ||
| return | ||
| } | ||
|
|
||
| let cell = cells.matching( | ||
| NSPredicate(format: "label CONTAINS[c] %@", label) | ||
| ).firstMatch | ||
| if cell.waitForExistence(timeout: 2) { | ||
| cell.tap() | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
These navigation helpers should assert that the target element exists before attempting to tap it. This provides clearer failure messages and avoids wasting time on subsequent steps if navigation failed.
| func firstQuickCardButton() -> XCUIElement? { | ||
| for label in SeedContent.quickCardLabels { | ||
| let button = buttons.matching( | ||
| NSPredicate(format: "label CONTAINS[c] %@", label) | ||
| ).firstMatch | ||
| if button.waitForExistence(timeout: 1) { | ||
| return button | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| /// Returns the label of the first visible quick card, or `nil`. | ||
| @MainActor | ||
| func firstVisibleQuickCardLabel() -> String? { | ||
| for label in SeedContent.quickCardLabels { | ||
| let match = buttons.matching( | ||
| NSPredicate(format: "label CONTAINS[c] %@", label) | ||
| ).firstMatch | ||
| if match.waitForExistence(timeout: 1) { | ||
| return label | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Iterating waitForExistence calls in a loop is slow. Optimize firstQuickCardButton and firstVisibleQuickCardLabel to fetch all buttons once and filter them locally by label instead of calling the waiter for each label index.
| .navigationBarTitleDisplayMode(.inline) | ||
| .onAppear(perform: loadDashboard) | ||
| .task { await observeConnectivity() } | ||
| .task { |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Use the id parameter on the .task modifier to keep the presenter's motion settings in sync with system changes (e.g., .task(id: accessibilityReduceMotion)).
There was a problem hiding this comment.
Pull request overview
This PR refactors OSA’s UI test suite to centralize common navigation/interaction helpers on XCUIApplication, reduces test flakiness by replacing hardcoded sleeps with explicit waits, and extracts shared connectivity-notice presentation logic into a reusable presenter used by Home and Settings.
Changes:
- Added
UITestHelpers.swiftwith shared seeded quick card labels,XCUIApplicationhelper extensions, and a screenshot helper forXCTestCase. - Updated multiple UI test suites to use the new helpers and replace
sleep()withwaitForExistence(...)-style waits. - Refactored Home/Settings connectivity notice handling to use
ConnectivityNoticePresenter, and optimized supply readiness evaluation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| OSAUITests/UITestHelpers.swift | Adds shared UI test helpers, seeded quick card labels, and screenshot convenience. |
| OSAUITests/OSARotationUITests.swift | Switches to app. helpers and removes duplicated helpers/sleeps in rotation flows. |
| OSAUITests/OSAFullE2EVisualTests.swift | Replaces sleeps with waits, uses shared seed labels, and updates screenshot calls. |
| OSAUITests/OSAContentAndInputTests.swift | Migrates content/input smoke tests to shared XCUIApplication helpers. |
| OSAUITests/OSAAccessibilitySmokeTests.swift | Migrates accessibility smoke tests to shared XCUIApplication helpers. |
| OSA/Shared/Components/ConnectivityBadge.swift | Introduces ConnectivityNoticePresenter to consolidate notice presentation behavior. |
| OSA/Features/Settings/SettingsScreen.swift | Uses ConnectivityNoticePresenter for connectivity state + notice display and auto-dismiss. |
| OSA/Features/Home/HomeSupport.swift | Optimizes evaluateSupplyReadiness() with grouped inventory lookup + precomputed thresholds. |
| OSA/Features/Home/HomeScreen.swift | Uses ConnectivityNoticePresenter and reduces Home reload churn on tab re-entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func tapTab(_ name: String) { | ||
| let button = tabBars.firstMatch.buttons[name] | ||
| if button.waitForExistence(timeout: 3) { | ||
| button.tap() |
There was a problem hiding this comment.
tapTab(_:) silently does nothing if the tab button isn't found. In screenshot-style tests this can lead to false positives by continuing on the wrong screen. Consider making this fail the test when the tab doesn't exist (e.g., XCTFail/XCTAssertTrue) or return Bool and require callers to assert success.
| button.tap() | |
| button.tap() | |
| } else { | |
| XCTFail("Expected tab bar button named '\(name)' to exist, but it was not found.") |
| /// Manages connectivity notice presentation with auto-dismiss. | ||
| /// | ||
| /// Extracts the observe → handle change → present notice → auto-dismiss pattern | ||
| /// shared by HomeScreen and SettingsScreen. | ||
| @MainActor @Observable | ||
| final class ConnectivityNoticePresenter { | ||
| private(set) var notice: ConnectivityStatusNotice? |
There was a problem hiding this comment.
ConnectivityNoticePresenter uses the @Observable macro, but this file only imports SwiftUI. Elsewhere in the codebase, @Observable types explicitly import Observation; without it this is likely to fail to compile. Add import Observation at the top of this file (or wherever the module requires it).
| /// Taps a tab bar button by name and waits for the UI to settle. | ||
| @MainActor | ||
| func tapTab(_ name: String) { | ||
| let button = tabBars.firstMatch.buttons[name] | ||
| if button.waitForExistence(timeout: 3) { | ||
| button.tap() | ||
| } | ||
| } |
There was a problem hiding this comment.
tapTab(_:) is documented as "waits for the UI to settle", but the implementation only taps (no post-tap wait). This mismatch can make UI tests flaky again and makes the doc comment misleading. Either add an explicit settle/wait after tapping (e.g., wait for a stable element) or update the doc comment to match the behavior.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Anthony Johnson II (AJ-EthereaLogic-ai)
merged commit 8ccbbad
into
main
Anthony Johnson II (AJ-EthereaLogic-ai)
deleted the
claude/e2e-testing-optimization-s9rpQ
branch
Summary
This PR consolidates repeated UI test helper logic into reusable
XCUIApplicationextensions and improves test stability by replacing hardcoded sleeps with proper wait conditions. A newUITestHelpers.swiftfile provides a single source of truth for test utilities and seeded content.Key Changes
Test Infrastructure
New
UITestHelpers.swift: Centralizes all UI test helper functions asXCUIApplicationextensions:tapTab(),navigateToMoreItem(),openLibraryChapter(),navigateBack()scrollToElement(),scrollToTop()dismissModal(),openNewNoteComposer()firstQuickCardButton(),firstVisibleQuickCardLabel()findButton(),firstHittableElement()openMapScreen(),submitAskQuestion(),handleLocationPermissionIfNeeded()screenshot(_:app:)extension onXCTestCaseShared seed content:
SeedContent.quickCardLabelsenum provides single source of truth for quick card titles used across all test suitesTest Stability Improvements
sleep()calls with proper wait conditions usingwaitForExistence(timeout:)screenshot(_:app:)helper with explicit app parameterTest Updates
app.prefixed helpersProduct Code Changes
ConnectivityNoticePresenterclass; improved dashboard reload logic to avoid visual churn on tab re-entryConnectivityNoticePresenterfor consistencyConnectivityNoticePresenterclass to extract shared observe → handle → present → dismiss patternevaluateSupplyReadiness()with pre-computed expiry threshold and category-grouped inventory lookupBenefits
https://claude.ai/code/session_01XrVM9UFJFH5rcwmcW4tcyx