Repository navigation
Conversation
…k test The enrollment-cancelled-while-saving test threw CancellationException from the fake store while the coroutine was still active, so nothing was actually cancelled. Removing the withContext(NonCancellable) guard around the rollback in saveLocalCredential still passed, even though under real cancellation the suspending revoke and the storage-dispatcher record delete would abort immediately and leave the server credential and local record behind. The test now launches enroll() in a job that the fake store cancels during save(), runs storage on a separate test dispatcher so the withContext hop observes the cancellation, and makes the mocked revoke suspend. It asserts the job was cancelled, revoke ran to completion, and both the record and key were deleted. A second case pins that local record and key cleanup still happen when revoke fails. Affects source/api tests only.
Audit of source/api tests under sdk/, session/, signout/, restorecredentials/, billing/, user/ and organizations/ for assertions that would survive a broken implementation. - ClerkTest: "environment not initialized" cases now null out Clerk.environment instead of installing a mock that returns ""/false, so the real null-fallback branches are exercised. The session/user/isSignedIn "returns null/false" cases first seed a signed-in session so they prove state is cleared rather than asserting the default. - SessionTokenFetcherTest: the skipCache test previously stubbed an empty cache, so it passed even if skipCache was ignored; it now provides a valid cached token and asserts the network token is returned. Clerk.environment and Clerk.clientFlow are stubbed in setup to avoid leaking global state. - SignOutServiceTest: the refresh-after-cleanup test now checks that device token and session are already cleared when the refresh runs and that the refreshed client is applied. - RestoreCredentialsTest: signed-out create asserts the specific error. - OrganizationMembershipTest: each can* helper is checked against only its own system permission; granting all permissions at once hid miswiring. - ClerkOfflineCacheTest: replace a real 50ms sleep with a wait on the retry job scheduled after the failure handler runs. - PublishableKeyHelperTest: rename a test that claimed ClerkClientError.
Tighten unit tests in source/api that passed without proving the behavior they are named for. - ClerkApiResultConverterFactoryTest: capture the delegate type requested from Retrofit so the wrap / no-wrap decisions for plain types, Environment, and List<Session> are asserted, and check the converter unwraps ClientPiggybackedResponse into Success. Drop the vacuous "shouldWrapInClientPiggybackedResponse returns false for List of Session" test, which only inspected its own fixture type. - DeviceAttestationHelperTest: verify performAssertion forwards the package name and token, pin getHashedClientId to known SHA-256 vectors, assert the hash cache actually stores entries, make clearCache start from a prepared provider and cached hash, rename the misleading attestDevice "throws" test, and remove the assertNotNull-only scope test. - NetworkConnectivityMonitorTest: assert stop unregisters the registered callback, drops the restore listener, and allows re-registration; assert repeated configure swaps the restore callback. Remove the default-state test that only read back what resetForTesting sets (covered by the resetForTesting test). - RequestLoggingMiddlewareTest: assert the ordinary request body is logged and the sensitive value never is. - ClientSyncingMiddlewareTest: assert the held sign-in completion is the SignInCompleted event for the response.
Audit of the sign-in, sign-up, passkey, biometric credential, shared session and telemetry unit tests for tests that would survive a revert of the behavior they are named for. source/api: - Re-enable PasskeyCreationServiceTest (the whole class was @ignore'd): stub Clerk.session and a named UserApi instead of a relaxed Json mock, use a nonce Credential Manager accepts, assert the returned passkey, the submitted credential JSON, and that verification is skipped on failures. The missing-bundle-JSON test expected a thrown exception that production never propagates; it now asserts the Failure. - Re-enable and fix the ignored passkey happy path in PasskeyAuthenticationServiceTest (it mocked an extension function); it now checks the request challenge/rpId and the attempt params. Assert the unknown-credential message and the converted sign-up id. - Replace vacuous PasskeyCredentialManagerTest smoke tests (try/catch swallowing everything) with delegation tests against a mocked CredentialManager. - PasskeyHelperTest: the "serializes" test never serialized; it now checks the encoded WebAuthn JSON. Drop a data-holder test that only read back constructor args and a duplicate malformed-URL test. - PasskeyServiceTest: drop a redundant 100-id delegation test; assert the passed-through throwable. - SignIn prepared-redirect failure asserts the message and that SSO is never started (it previously passed via an unrelated Clerk-not-initialized failure). - Pin the biometric sign-in locale to "fr-CA" instead of echoing LocaleProvider. - Make the lock-wait contract test wait on the writer's thread state with a long store timeout instead of a 300 ms sleep, and surface writer failures. - Restore Clerk.environment after SignInExtensionsTest and SignUpEmailVerificationStrategyTest, stop shared-session sync after SharedSessionSyncPublicApiTest, and reset the passkey credential managers after the passkey service tests. source/telemetry: - Cover ClerkTelemetryEnvironment behavior (provider delegation, empty publishable key treated as missing), not only constructor shapes.
Tighten ViewModel tests under source/ui userprofile and organizationprofile
that passed without checking the behavior they are named for:
- Verify request payloads, not just the final state: profile update
params, profile image file, password params (incl. sign-out-others),
primary email id, email/TOTP verification codes, TOTP backup codes.
- Verify follow-up side effects: user refresh after profile/image
updates, device list refresh after sign-out, client refresh after
org delete (and none on failure), no biometric cleanup when account
deletion fails, no API call when email is immutable.
- Replace the vacuous change-password resetState test (Idle -> Idle)
with one that resets from an Error state.
- Passkey: assert initial Idle emissions and that the cancelled
createPasskey flow actually ran before expectNoEvents().
- Domains: cover the domains-disabled branch the test name claims.
- Custom row gating: assert exact rendered rows (all{} passed on empty).
- UserProfileAccountViewModelTest: use MainDispatcherRule so
Dispatchers.Main is reset instead of leaking across test classes.
- Drop a redundant CustomRouteNavKey round-trip test.
Replace vacuous or ignored tests in the api module's sso, auth, hostedauth and magiclink test packages with assertions that fail when the covered behavior breaks. Each changed test was checked by mutating production code. - SSOServiceTest: drop the @ignore'd suite and replace it with Robolectric tests for null strategy, create/prepare failures, nonce sign-in completion, the sign-up redirect flow, sign-up to sign-in transfer, and callbacks that arrive with no pending flow - ExternalAccountServiceTest: replace no-op "initially false" and cancel tests with tests that start a real pending connection; cover verified, missing and unverified accounts, the cancellation rethrow, and a stale completion that must not clear a newer connection - SSOServiceCancellationTest: add the case where a stale completion succeeds and must not resolve or clear a newer flow - SSOManagerActivityTest: the "throws" test asserted the Activity's default result code while swallowing uncaught exceptions; it now fails if the exception escapes. Verify that the callback URI is forwarded - GoogleSignInServiceTest: cover transferable=false and assert the unclassified credential exception is preserved - AuthHandleTest: cover hosted auth taking precedence over the SSO fallback for clerk-scheme callbacks (the old test used a non-clerk scheme) - PKCE tests: assert that the challenge is derived from the verifier, and that magic link completion sends the verifier that matches the challenge used at prepare - AuthTest: remove an email OTP test duplicated by AuthOtpTest
…othing Audit of source/ui auth/, signin/, signup/, core/, input/ unit tests and the com.clerk.snapshot Paparazzi suite for tests that would survive a revert of the behaviour they are named for. Logic tests (source/ui): - AuthViewModelTest: replace tests that re-implemented ViewModel logic inline (OAuth result mapping, email regex, identifier resolution, enterprise SSO detection) with tests that drive AuthStartViewModel and assert its state and SignIn/SignUp params; drop enum-name/OAuthProvider tautologies; SignInOrUp now asserts non-fallback errors surface without SignUp.create. - AuthStateEffectsTest: run under the compose test rule instead of an unrecorded Paparazzi snapshot; assert the snackbar message. - AuthStartViewHelperTest: assert the combined email+username autofill hints. - AuthStateForceMfaTest: fix misleading resolveCorrespondingSession name and cover the matching-session path. - SignInFactorOneViewTest: cover the substitution branch of resolveFirstFactor (every existing case passed with an identity implementation). - SignInAttemptHandlerTest: error tests assert the propagated message instead of a value the test set itself. - SignInFactorCodeViewModelTest / SignInPrepareHandlerTest / LastUsedAuthTest: remove tests fully subsumed by stronger siblings; assert state for second factor verification reuse; rename misleading "Throw" tests. - PhoneInputUtilsTest / CountryInfoTest: test regionToFlagEmoji directly (old test only passed because the phone code was 0), make the real-context test assert the detected country, drop data-class equality/toString tests. Snapshots: - Record missing goldens for AuthStartViewSnapshotTest and organizationSwitcherCustomTrigger (verify compared against a blank image). - authTopBar_preservesWideLogoAspectRatio now serves a real 5:1 bitmap through Coil's preview handler; the SVG data URI failed to decode and it rendered the placeholder icon. - passwordSection_Light now renders a user with a password (it duplicated the add-password state). - Remove organizationVerifiedDomainsErrorState / organizationActionDeleteError: byte-identical to AddDomain / DeleteReady because the error is only shown by the scaffold snackbar. - Re-record stale OrganizationProfile goldens whose production design changed (members tabs, domains list, enrollment mode). - Delete orphan goldens with no backing test (old com.clerk.ui.core.*, signin, BackupCodeView, MfaAddTotp, password-section variants).
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: clerk/clerk-android/.coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (50)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. 9 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughThis pull request updates tests across the API, UI, and telemetry modules. The tests add assertions for API results, state transitions, callback handling, cancellation, request contents, and fixture behavior. No production behavior changes are included in the supplied summaries. ChangesSDK and UI test coverage
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The production change does not alter behavior, and the added autofill test checks the expected combined hints. No actionable regression is established; normal CI checks should pass before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
# Conflicts: # source/api/src/test/java/com/clerk/api/passkeys/PasskeyCredentialManagerTest.kt # source/ui/src/test/java/com/clerk/ui/auth/AuthViewModelTest.kt # source/ui/src/test/java/com/clerk/ui/input/PhoneInputUtilsTest.kt # source/ui/src/test/java/com/clerk/ui/userprofile/security/passkey/UserProfilePasskeyViewModelTest.kt # source/ui/src/test/java/com/clerk/ui/userprofile/security/password/UserProfileChangePasswordViewModelTest.kt
# Conflicts: # source/api/src/test/java/com/clerk/api/sso/ExternalAccountServiceTest.kt # source/ui/src/test/java/com/clerk/ui/auth/AuthViewModelTest.kt
# Conflicts: # source/api/src/test/java/com/clerk/api/sso/ExternalAccountServiceTest.kt # source/api/src/test/java/com/clerk/api/sso/SSOManagerActivityTest.kt # source/ui/src/main/java/com/clerk/ui/auth/AuthStartView.kt
This was referenced Oct 1, 2026
# Conflicts: # source/ui/src/test/java/com/clerk/ui/userprofile/security/device/DeviceViewModelTest.kt
This was referenced Oct 2, 2026
Collaborator
Author
|
Closing in favour of two focused PRs off current About half of this PR has already shipped, and Already on
Re-opened
Dropped
Still open
The 🤖 Generated with Claude Code |
swolfand
added a commit
that referenced
this pull request
Oct 6, 2026
…cover breaks Carries over the source/api and source/telemetry test changes from #985 that still apply to main. Re-enables PasskeyCreationServiceTest and the switched-off PasskeyAuthenticationServiceTest/SSOServiceTest cases, replaces assertions that could not fail (non-null checks, default Activity results, empty caches, all-permissions-at-once) with checks on the actual params, results and errors. Adapts the SSO tests to main's Clerk.auth routing: SSOServiceTest stubs and verifies Auth.emitAuthError, and the stale-completion cancellation test stubs SignUpApi.createSignUp instead of SignUp.create.
swolfand
added a commit
that referenced
this pull request
Oct 6, 2026
Carries over the source/ui test changes from #985 that still apply to main, and ports the auth tests onto the Clerk.auth facade the prebuilt UI now uses. AuthStartViewModel: the sign-in-or-up test now checks that a non-fallback sign-in error is surfaced without starting a sign-up, sign-up chooses email or username params from the identifier format, and OAuth sign-up ends in SignUpSuccess. SignInAttemptHandlerTest checks the error message passed to the callback. Profile and organization view model tests verify the params sent to the API and the refresh after success. Removes tests that couldn't fail: enum name checks, data-class equality/toString, exact duplicates, and Idle->Loading checks already covered by tests that also assert the final state.
swolfand
added a commit
that referenced
this pull request
Oct 6, 2026
…cover breaks (#1049) * test(api): make api and telemetry tests fail when the behaviour they cover breaks Carries over the source/api and source/telemetry test changes from #985 that still apply to main. Re-enables PasskeyCreationServiceTest and the switched-off PasskeyAuthenticationServiceTest/SSOServiceTest cases, replaces assertions that could not fail (non-null checks, default Activity results, empty caches, all-permissions-at-once) with checks on the actual params, results and errors. Adapts the SSO tests to main's Clerk.auth routing: SSOServiceTest stubs and verifies Auth.emitAuthError, and the stale-completion cancellation test stubs SignUpApi.createSignUp instead of SignUp.create. * test(api): verify the sign-up call production makes and replace explanatory comments with names
swolfand
added a commit
that referenced
this pull request
Oct 6, 2026
) * test(ui): make ui tests fail when the behaviour they cover breaks Carries over the source/ui test changes from #985 that still apply to main, and ports the auth tests onto the Clerk.auth facade the prebuilt UI now uses. AuthStartViewModel: the sign-in-or-up test now checks that a non-fallback sign-in error is surfaced without starting a sign-up, sign-up chooses email or username params from the identifier format, and OAuth sign-up ends in SignUpSuccess. SignInAttemptHandlerTest checks the error message passed to the callback. Profile and organization view model tests verify the params sent to the API and the refresh after success. Removes tests that couldn't fail: enum name checks, data-class equality/toString, exact duplicates, and Idle->Loading checks already covered by tests that also assert the final state. * test(ui): name what the user profile tests check instead of commenting it
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.
What
An audit of the whole unit test suite (
source/api,source/ui,source/telemetry), about 216 test files, looking for tests that don't really test anything. It fixes tests that:Only test files and snapshot images change. There are no production code changes.
Each strengthened test was mutation-checked: the production line it covers was temporarily broken, the test was confirmed to fail, then the file was restored.
Results
main):source:api:testDebugUnitTest):source:ui:testDebugUnitTest)verifyPaparazziDebug(not run in CI)detektandspotlessCheckpass.Highlights
Switched-off tests brought back
PasskeyCreationServiceTestwas switched off with@Ignore. Turning it back on showed 8 of its 9 tests failing, because they depended on an unmockedClerk.sessionand a faked serializer. It's now rewritten to check the submitted credential JSON, the returned passkey and the exact errors.PasskeyAuthenticationServiceTest's sign-in test was also switched off. It's back, and now checks the sign-in strategy, the challenge and rpId, and the exact attempt parameters.SSOServiceTestwas switched off and its tests exercised nothing. It's replaced with 7 tests covering nonce sign-in, sign-up redirects, the sign-up→sign-in transfer, prepare and create failures, and a callback with no flow in progress.Tests that checked nothing
AuthViewModelTest:"x" == "x", anotherif (true).ClerkTest: the "environment not initialized" tests installed a mock environment, so the real null fallback never ran.ClerkApiResultConverterFactoryTest: only checked results weren't null. It now checks the type is wrapped and unwrapped correctly.DeviceAttestationHelperTest:valwasn't null.ExternalAccountServiceTest: never set up a connection in progress, so the logic under test never ran.PasskeyCredentialManagerTest: a try/catch swallowed every failure.assertNotNull(true), reset tests that went from Idle to Idle, and Turbine tests that read an emission without asserting it.Tests that would survive a revert
SessionTokenFetcherTest: theskipCachetest had an empty cache, so it passed even ifskipCachewas ignored.OrganizationMembershipTest: granted all permissions at once, so a wrongly wiredcan*helper still passed. Now each permission is granted one at a time.SignInAuthenticateWithRedirectTest: failed for an unrelated "Clerk must be initialized" reason.SSOManagerActivityTest: assertedRESULT_CANCELED, which is an Activity's default result.PhoneInputUtilsTest: passed only because a mocked country code was 0.Recently added tests
BiometricCredentialsTest's rollback test for a cancelled enrollment threwCancellationExceptionfrom a fake without cancelling anything, so it still passed withNonCancellableremoved. It now cancels the job for real. There's a companion test where the server revoke fails.Flakiness and isolation
delayorThread.sleepraces are replaced with explicit synchronization or virtual time. Affected:ClerkOfflineCacheTestand the biometric file-lock test.Clerk.environment, shared-session-sync, credential-manager andStorageHelperstate that leaked across test classes.Snapshots
8f20caa6), the wide-logo top bar (it had rendered the placeholder) andpasswordSection_Light(identical to dark).The remaining 12
verifyPaparazziDebugfailures were already failing onmain:paparazzi.snapshotas a harness with no golden:ClerkThemeSnapshotTest,UserProfileExternalAccountSectionTest, and 2 inUserProfileEmailRowTest. They should move to the compose test rule, asAuthStateEffectsTestdoes here.Production bugs found (not fixed here)
Verification.Statushas two values swapped:FAILEDis@SerialName("expired")andEXPIREDis@SerialName("failed")(Verification.kt:35-36). Nothing inside the SDK branches on them, but it's a public enum.connectExternalAccountcan hang:SSOManagerActivity.authorizationFailed()cancels hosted auth and SSO but not a pending external-account connection. The nextconnectExternalAccountalso overwrites the pending deferred without completing it.getDomain()removeswww.anywhere in the host, not only at the start:foowww.example.com→fooexample.com.raw_id(snake_case viaClerkApi.json) where WebAuthn specifiesrawId. This only matters if the server doesn't acceptraw_id.Clerk.reset()doesn't stop shared-session sync. This may be intentional.🤖 Generated with Claude Code
Summary by CodeRabbit