Conversation
…in flow The legacy sign-in recovery tests set legacySignInRecovery and isPresented in the same runloop turn via a test-only preview flag, so the recovery sheet was requested while the auth picker sheet it is nested inside was still animating in. UIKit drops a presentation issued while another is in progress, which made testLegacyRecoveryEmailLinkOptionNavigatesWithPrefilledEmail fail intermittently on CI. Production never produces that state, since legacySignInRecovery is only ever set from tryPresentLegacySignInRecovery after a failed sign-in, long after the picker has settled. Both tests now drive that real path instead, seeding an account that carries both password and emailLink sign-in methods so buildLegacySignInRecovery returns a context rather than nil.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the legacy sign-in recovery flow in the Firebase SwiftUI example app and its associated E2E tests. Key changes include moving to synchronous sign-out in the test view to prevent race conditions, updating the test suite to use a more robust recovery sheet presentation, and improving the emulator OOB code fetching logic. Review feedback highlighted several areas where force-unwrapping URLs could lead to runtime crashes, suggesting the use of safer guard-let patterns, and recommended using waitForExistence for UI elements to improve test stability in CI environments.
Wait for the password field rather than asserting bare existence, and guard the URLs built in fetchOobCode and createLegacyRecoveryUser instead of force-unwrapping them, matching the idiom createTestUser already uses.
The email/password case asserted that the email field still held text the test itself had typed into it, which passes whether or not recovery does anything, because EmailAuthView sits at the picker root and keeps its own state across the sheet. It now asserts the sheet is actually dismissed, and is named for what it verifies. Prefill remains covered by the email link case, where EmailLinkView is built fresh by the navigation destination. The email link assertion now polls for the value rather than reading it once, since the field becomes queryable before onAppear applies the prefill. createLegacyRecoveryUser also asserts the seeded account really reports both sign-in methods, so a change in emulator behaviour fails immediately instead of surfacing later as an unexplained timeout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two legacy sign-in recovery tests drove the sheet through a test-only preview flag that set
legacySignInRecoveryandisPresentedin the same runloop turn, so the recovery sheet asked to present while the auth picker sheet it is nested inside was still animating in. UIKit drops a presentation issued while another is already in progress, which madetestLegacyRecoveryEmailLinkOptionNavigatesWithPrefilledEmailfail intermittently on CI, most often as the first test after a test runner relaunch. The.xcresultfrom a failing run shows the picker fully presented, the app unauthenticated, and no recovery view anywhere in the accessibility hierarchy.Production never reaches that state, because
legacySignInRecoveryis only ever set fromtryPresentLegacySignInRecoveryafter a failed sign-in, long after the picker has settled. Rather than paper over the timing, both tests now drive that real path: they seed an account carrying bothpasswordandemailLinksign-in methods, which is what makesbuildLegacySignInRecoveryreturn a context instead of nil, then fail a password sign-in so the sheet appears the way it does for a real user. The preview flag and its init-time assignment are gone, and the deferredTask { try signOut() }inTestViewis now a synchronous call so it can no longer mutate observable auth state mid-presentation.Verified locally against the auth emulator with five iterations of each test, all green.
Maintainer note: Fixes internal CPRN-452
One coverage note worth flagging: the email/password case can no longer verify the prefill itself.
EmailAuthViewsits at the picker root rather than being pushed, so it keeps its own@Stateacross the sheet, and the test has necessarily typed the email into that field already in order to trigger the failed sign-in. Asserting the field afterwards therefore passes whether or not recovery does anything. That case now asserts the sheet is dismissed and is named accordingly, and the prefill behaviour is covered by the email link case, whereEmailLinkViewis built fresh by the navigation destination and its field genuinely starts empty.