-
Notifications
You must be signed in to change notification settings - Fork 0
Add iOS Release-config UI test gate to TestFlight workflow (#253) #261
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
Changes from all commits
aac4586
1c7fd57
266394b
281d75a
991a920
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,9 @@ public actor APIClient { | |
| defaults.removeObject(forKey: uiTestStoreDefaultsKey) | ||
| AudioEngine.resetPersistedPrefs() | ||
| Self.clearPersistedSessionArtifacts(session: session) | ||
| // Flush the in-memory cache to cfprefsd so the immediately | ||
| // following read sees the cleared state. Issue #266 follow-up. | ||
| defaults.synchronize() | ||
| } | ||
|
|
||
| if let persistedData = defaults.data(forKey: uiTestStoreDefaultsKey), | ||
|
|
@@ -48,6 +51,15 @@ public actor APIClient { | |
| self.uiTestStore = UITestStore.makeDefault(seedAuthenticated: parsedUITestConfig.seedAuthenticated) | ||
| persistUITestStore() | ||
| } | ||
|
|
||
| // Diagnostic for issue #266 / PR #261 — log what the app actually | ||
| // observed at init so we can see in xcresult logs whether the env | ||
| // vars + reset-store contract are doing what we expect on macos-26 | ||
| // CI runners. Cheap, noise-only in test builds since UITestConfig | ||
| // is nil in production. | ||
| print("[E2E-DIAG] APIClient.init uiTestMode=YES seedAuth=\(parsedUITestConfig.seedAuthenticated) resetStore=\(parsedUITestConfig.resetStore) forceOffline=\(parsedUITestConfig.forceLaunchOffline) forceTokenExpired=\(parsedUITestConfig.forceTokenExpired) finalIsAuthenticated=\(uiTestStore?.isAuthenticated ?? false)") | ||
| } else { | ||
| print("[E2E-DIAG] APIClient.init uiTestMode=NO") | ||
| } | ||
|
Comment on lines
+61
to
63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This diagnostic print runs whenever UI-test mode is not enabled, which includes normal production app launches. That causes unconditional runtime logging in non-test builds and contradicts the stated intent that diagnostics are test-only. Gate this branch behind a debug/test condition (or remove it) so production builds do not emit E2E diagnostic logs. [logic error] Severity Level: Minor 🧹- ⚠️ Production launches emit internal "[E2E-DIAG]" diagnostic log lines.
- ⚠️ Test-only diagnostics pollute normal device and crash logs.
- ⚠️ Minor startup overhead from unconditional console printing.Steps of Reproduction ✅1. Launch the iOS app built from this PR in a normal (non-UI-test) configuration so that
the main entry point `StillPointApp` runs (ios/StillPointApp/StillPointApp.swift:5-13),
which creates a `WindowGroup` showing `RootView()`.
2. Observe that `RootView` (ios/StillPointApp/Views/RootView.swift:4-7) holds `@State
private var appVM = AppViewModel()` and attaches a `.task { await appVM.checkAuth() }`
modifier (RootView.swift:12-13), so `AppViewModel.checkAuth()` is invoked on initial view
appearance on every cold start.
3. In `checkAuth()` (ios/StillPointApp/ViewModels/AppViewModel.swift:70-87), the method
calls `APIClient.shared.me()` (AppViewModel.swift:85), which lazily initializes the
singleton `APIClient.shared` defined in `APIClient`
(ios/StillPointShared/Sources/StillPointShared/APIClient.swift:5-8, 16-23).
4. During `APIClient.init` (APIClient.swift:16-63), `UITestConfig.fromProcessInfo()` is
evaluated (line 29) and typically returns `nil` for normal production/dev launches as
described by the comment at lines 55-59 ("UITestConfig is nil in production"). That causes
the `else` branch at lines 61-63 to execute, which runs `print("[E2E-DIAG] APIClient.init
uiTestMode=NO")` unconditionally for every non-UI-test startup, emitting E2E diagnostic
logging in non-test builds.Fix in Cursor | Fix in VSCode Claude (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** ios/StillPointShared/Sources/StillPointShared/APIClient.swift
**Line:** 61:63
**Comment:**
*Logic Error: This diagnostic print runs whenever UI-test mode is not enabled, which includes normal production app launches. That causes unconditional runtime logging in non-test builds and contradicts the stated intent that diagnostics are test-only. Gate this branch behind a debug/test condition (or remove it) so production builds do not emit E2E diagnostic logs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ set -euo pipefail | |
| LANE="${1:-smoke}" | ||
| MAX_RETRIES="${2:-1}" | ||
| ATTEMPT=1 | ||
| UI_TESTS_DIR="${IOS_UI_TESTS_DIR:-ios/StillPointUITests}" | ||
| UI_TESTS_DIR="${IOS_UI_TESTS_DIR:-ios/StillPointAppUITests}" | ||
| SECRETS_REQUIRED="${E2E_SECRETS_REQUIRED:-true}" | ||
| STATUS_FILE="artifacts/e2e/ios/${LANE}.status" | ||
| FINAL_STATUS="failed" | ||
|
|
@@ -28,9 +28,13 @@ if [[ "${E2E_ENV:-}" == "prod" || "${E2E_BASE_URL:-}" =~ still-point\.me ]]; the | |
| fi | ||
|
|
||
| if [[ ! -d "${UI_TESTS_DIR}" ]]; then | ||
| echo "iOS E2E suite not present (${UI_TESTS_DIR}); skipping ${LANE} lane." | ||
| FINAL_STATUS="skipped" | ||
| exit 0 | ||
| # Hard-fail: a missing test directory was previously silently treated as "skipped" | ||
| # which let broken-on-Release builds (e.g. build 8 / issue #250) ship with green | ||
| # CI. The contract is that the suite must be present and runnable. | ||
| echo "::error::iOS E2E suite not present at expected path '${UI_TESTS_DIR}'." | ||
| echo "::error::Set IOS_UI_TESTS_DIR to override, or restore the test bundle." | ||
| FINAL_STATUS="failed" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ "${SECRETS_REQUIRED}" == "true" ]] && [[ -z "${E2E_TEST_USER_EMAIL:-}" || -z "${E2E_TEST_USER_PASSWORD:-}" ]]; then | ||
|
|
@@ -43,21 +47,65 @@ if [[ "${E2E_TEST_USER_EMAIL:-}" =~ @still-point\.me$ ]]; then | |
| exit 1 | ||
| fi | ||
|
|
||
| TEST_PLAN="${IOS_TEST_PLAN:-StillPointE2E}" | ||
| TEST_DESTINATION="${IOS_TEST_DESTINATION:-platform=iOS Simulator,name=iPhone 16,OS=latest}" | ||
| resolve_test_destination() { | ||
| # Honor explicit caller override first. | ||
| if [[ -n "${IOS_TEST_DESTINATION:-}" ]]; then | ||
| echo "${IOS_TEST_DESTINATION}" | ||
| return | ||
| fi | ||
|
|
||
| # Pick the first iPhone simulator that's actually installed on this host. | ||
| # Prevents the "iPhone 16 doesn't exist on macos-26 runners" / "iPhone 17 will | ||
| # eventually disappear too" infra-failure mode CodeAnt flagged. We grep simctl | ||
| # output with a trailing " (" so "iPhone 17" doesn't accidentally match | ||
| # "iPhone 17 Pro". | ||
| local simctl_output | ||
| simctl_output="$(xcrun simctl list devices available 2>/dev/null || true)" | ||
| local preferred=( | ||
| "iPhone 17 Pro" | ||
| "iPhone 17" | ||
| "iPhone 17 Pro Max" | ||
| "iPhone 17e" | ||
| "iPhone 16e" | ||
| "iPhone 16 Pro" | ||
| "iPhone 16" | ||
| "iPhone 15 Pro" | ||
| "iPhone 15" | ||
| "iPhone Air" | ||
| ) | ||
| local name | ||
| for name in "${preferred[@]}"; do | ||
| if grep -F " ${name} (" <<<"${simctl_output}" >/dev/null 2>&1; then | ||
| echo "platform=iOS Simulator,name=${name},OS=latest" | ||
| return | ||
| fi | ||
| done | ||
|
|
||
| # Last-resort fallback: keep the prior hardcoded default so we fail with a | ||
| # clear xcodebuild error instead of an empty destination. | ||
| echo "platform=iOS Simulator,name=iPhone 17,OS=latest" | ||
|
Comment on lines
+64
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Simulator auto-selection only checks a fixed shortlist and then falls back to a hardcoded Severity Level: Major
|
||
| } | ||
|
|
||
| TEST_DESTINATION="$(resolve_test_destination)" | ||
| echo "Using iOS test destination: ${TEST_DESTINATION}" | ||
| TEST_SCHEME="${IOS_TEST_SCHEME:-StillPoint}" | ||
| PROJECT_PATH="${IOS_TEST_PROJECT:-ios/StillPoint.xcodeproj}" | ||
| TEST_CONFIGURATION="${IOS_TEST_CONFIGURATION:-}" | ||
|
|
||
| resolve_test_target() { | ||
| # The test target and class are both named StillPointAppUITests. | ||
| # Smoke runs the full Begin -> Session -> Complete -> History golden path | ||
| # (the test that would have caught issue #250 if the runner had actually | ||
| # been executing tests). Critical runs the full UI test class. | ||
| case "$1" in | ||
| smoke) | ||
| echo "StillPointUITests/SmokeTests" | ||
| echo "StillPointAppUITests/StillPointAppUITests/testLaunchLoginCompleteSessionAndHistoryPersistence" | ||
| ;; | ||
| critical) | ||
| echo "StillPointUITests/CriticalPathTests" | ||
| echo "StillPointAppUITests/StillPointAppUITests" | ||
| ;; | ||
| *) | ||
| echo "StillPointUITests/${1}" | ||
| echo "StillPointAppUITests/${1}" | ||
| ;; | ||
| esac | ||
| } | ||
|
|
@@ -69,14 +117,20 @@ run_lane() { | |
| test_target="$(resolve_test_target "${lane_tag}")" | ||
| result_bundle="artifacts/e2e/ios/${lane_tag}-attempt-${ATTEMPT}.xcresult" | ||
|
|
||
| local -a xcodebuild_args=( | ||
| test | ||
| -project "${PROJECT_PATH}" | ||
| -scheme "${TEST_SCHEME}" | ||
| -destination "${TEST_DESTINATION}" | ||
| -resultBundlePath "${result_bundle}" | ||
| -only-testing:"${test_target}" | ||
| ) | ||
| if [[ -n "${TEST_CONFIGURATION}" ]]; then | ||
| xcodebuild_args+=(-configuration "${TEST_CONFIGURATION}") | ||
| fi | ||
|
|
||
| set +e | ||
| xcodebuild test \ | ||
| -project "${PROJECT_PATH}" \ | ||
| -scheme "${TEST_SCHEME}" \ | ||
| -testPlan "${TEST_PLAN}" \ | ||
| -destination "${TEST_DESTINATION}" \ | ||
| -resultBundlePath "${result_bundle}" \ | ||
| -only-testing:"${test_target}" \ | ||
| xcodebuild "${xcodebuild_args[@]}" \ | ||
| | tee "artifacts/e2e/ios/${lane_tag}-attempt-${ATTEMPT}.log" | ||
| local status=$? | ||
| set -e | ||
|
|
||
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.
Remove PII from auth diagnostic logging and gate it to UI-test mode.
Line 78 logs
currentUseremail directly. That’s user identifier leakage in runtime logs outside CI test contexts.Proposed fix
func checkAuth() async { let startedAt = Date() isLoading = true defer { isLoading = false - // Diagnostic for issue `#266` / PR `#261` — log the post-checkAuth view - // state so we can correlate with APIClient.init's [E2E-DIAG] line - // when iOS UI tests fail on the auth-screen waitForExistence. - print("[E2E-DIAG] checkAuth.done currentView=\(viewSlug(currentView)) currentUser=\(currentUser?.email ?? "nil") elapsedMs=\(Int(Date().timeIntervalSince(startedAt) * 1_000))") + if shouldEmitE2EDiagnostics { + print("[E2E-DIAG] checkAuth.done currentView=\(viewSlug(currentView)) currentUserPresent=\(currentUser != nil) elapsedMs=\(Int(Date().timeIntervalSince(startedAt) * 1_000))") + } } @@ } + + private var shouldEmitE2EDiagnostics: Bool { + let raw = ProcessInfo.processInfo.environment["SP_UI_TEST_MODE"]?.lowercased() + return raw == "1" || raw == "true" || raw == "yes" || raw == "on" + }📝 Committable suggestion
🤖 Prompt for AI Agents