Run unit tests in CI and characterize the git auth controller - #75
Merged
Conversation
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
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.
Why
CI has never run the unit tests.
ci_verify_build.ymlruns detekt and:app:assembleDebugonly, so the 689 existing tests could go red ondevelopwithout anyone noticing. This is the safety net for the feature-modularization work that follows, where every later step relies on the suite being trustworthy.Two smaller findings came out of the same audit:
GitAuthController— the git-auth state machine shared between:feature:gitand:feature:settings— has no direct test coverage at all. It sits exactly on a boundary that a later change reshapes.:feature:settingsdeclaresimplementation(projects.feature.projects)but uses nothing from it.What changed
.github/workflows/ci_verify_build.yml— newunit-testsjob running./gradlew test, alongside the existing detekt and assemble jobs. Uploads the HTML test reports when it fails.GitAuthControllerCharacterizationTest— 18 characterization tests in:feature:gitpinning the current behaviour ofGitAuthController: prompt opening, token submission, the GitHub device flow, cancellation, and dismissal.feature/settings/build.gradle.kts— dropped the unused:feature:projectsdependency.DataStorePreferencesRepositoryTest— fixed a pre-existing race the new CI job immediately exposed. See below.Behavior
No behavior change. Only test and CI additions, plus one dependency removal with no corresponding source usage.
The failure the new job caught on its first run
DataStorePreferencesRepositoryTest > selected variant is remembered per project across a simulated relaunchfailed in CI with:The test's
withRepositoryhelper cancels itsCoroutineScopein afinally, then immediately opens a second DataStore on the same file to simulate a relaunch. Cancellation is not synchronous: DataStore only releases its exclusive connection to the file once the scope's job has actually completed. Locally the teardown wins that race, so the test always passed; on a slower runner it loses.Fixed by joining the job after cancelling. This bug is not introduced here — it has been latent for as long as the test has existed, because the tests have never run in CI. It is included in this PR because a change whose purpose is turning the gate on cannot land with the gate red.
The characterization tests deliberately assert what the code does today rather than what it arguably should do. Two quirks are pinned as-is, with comments:
onStartGitHubSignInkeys off emitted state (isBusy/device/succeeded), not off whether adeviceJobis live. Until the authenticator emits its first step, all three are false, so a second tap starts a second authentication and cancels the first.Both are pinned so a later refactor of this boundary either preserves them or changes them knowingly.
Scope
Left out on purpose:
src/mainusage, but two of them (:feature:projectsto:data:templates,:feature:editorto:data:local) are used from tests and need converting totestImplementationrather than deleting. That is its own change.Routes.ktbuilds routes throughandroid.net.Uri.encode, which returns stubbed defaults undertestOptions.unitTests.isReturnDefaultValues, and there is no Robolectric here. A JVM test would assert onnulland falsely bless a broken route, so it is not worth adding in this shape.:data:localdetekt baseline. It is the one module without a baseline file, but detekt reports zero findings for it, so the baseline would be empty.Tests
GitAuthControllerCharacterizationTest, covering: prompt open in both sign-in and token modes,hasCredentialsnull-host handling, blank and whitespace token rejection, token trimming and credential save shape, the null-host submission quirk, the unconfigured-GitHub fallback message, device-code surfacing, error mapping, the re-entrancy guard on both sides, mode-switch cancellation, dismissal before and after success, and the 1100 ms deferred auto-close.advanceTimeByruns events in[now, now + n), so the deferred close is asserted at exactly 1100 ms rather than "some time later".MutableSharedFlowand assert that a late emission cannot mutate state. Verified by mutation: deletingdeviceJob?.cancel()fromonAuthModeChangedmakes the test fail.DataStorePreferencesRepositoryTestnow deterministically tears its DataStore down between simulated relaunches.Verification
Verified from a clean worktree of the branch tip, so no uncommitted local work could mask a failure:
./gradlew test detekt— BUILD SUCCESSFUL./gradlew :app:assembleDebug— BUILD SUCCESSFULCI on the branch tip: Unit tests, Detekt and Assemble debug all green. The DataStore race could only be confirmed fixed in CI — it reproduces on runner I/O timings, and five local
--rerun-tasksruns passed both before and after the fix, so local runs prove nothing here../gradlew testwas confirmed to cover every subproject, and to run debug unit tests only — the release-signing guard inapp/build.gradle.ktsreadsgradle.startParameter.taskNames, so atestinvocation cannot trip it and the new job needs no signing configuration.