Skip to content

Test persisted Test Centre answers and activation paths - #2770

Merged
ryanbr merged 1 commit into
ryanbr:mainfrom
kavemang:agent/testcentre-production-tests-2708
Oct 15, 2026
Merged

ryanbr merged 1 commit into
ryanbr:mainfrom
kavemang:agent/testcentre-production-tests-2708

Conversation

@kavemang

@kavemang kavemang commented Oct 11, 2026 •

Copy link
Copy Markdown

What this PR does

Remove the Test Centre answer writer and migration stamp on both platforms after verifying that only tests call them. Retarget the tests onto the persisted questionnaire reader and shipped activation behavior, with coverage for empty/malformed data, domain separation and unrelated-key preservation.

The Apple test fixture now snapshots and restores the defaults it touches; its unused isolated-suite helper did not actually isolate any existing test. Existing persisted answer formats, readers, activation gates and drained-row tally behavior remain unchanged. Legacy keys are neither migrated nor deleted.

Type of change

  • Refactor / cleanup

How it was tested

  • Focused Linux Swift compile of the actual TestCentre/TestDomain and all seven committed activation tests passed, retaining actor annotations and async XCTest fixtures. Only the unrelated PuffinExperiment key is supplied as its existing constant; this does not compile the whole Apple app.
  • Actual production Kotlin TestCentre/TestDomain with all seven committed tests passed using JUnit, the Android SDK SharedPreferences interface, real org.json, and the committed in-memory preference fixture. This is targeted JVM evidence, not full app compilation.
  • Private wrong-domain reader mutations fail the persisted-questionnaire test on both platforms; no mutation is committed.
  • Guarded parity refresh against origin/main passed its identity ratchet (290 existing findings). Derived snapshots remain byte-identical to the base, so no metadata changes are needed.
  • Core Tools: all 153 tests pass. Source hygiene, whitespace and diff-scoped i18n pass. Actual head e960c4a6bf1883907319e4b32430d3a6334c34b2: all six expected CI checks passed, confirmed twice with a stable complete roster.
  • Android full Kotlin/KSP compilation and APK build passed (4m56s); full testFullDebugUnitTest passed (2m5s).
  • Universal macOS Strand build and StrandTests passed: 2,280 tests, two skipped, zero failures. All seven TestCentreActivationTests passed, including the persisted-reader tests and async fixture lifecycle.
  • iOS NOOPiOS build passed. Its test step is skipped by workflow design: compile-only evidence. Both Apple jobs skipped merging current main; validation is at the actual PR head, not a speculative later merge.
  • Tools CI passed 234 capture, 50 parity repository-acceptance, two RR preservation, and 153 core tests. Hygiene and i18n passed. No warnings name the changed production/test files in native logs; unchanged TestCentreView carries existing deprecation warnings. No BLE sequencing/commands or hardware behavior changes.

Checklist

  • Native Android unit tests and compilation passed
  • Native macOS StrandTests and both Apple builds passed
  • No UI style changes or hardcoded protocol frames
  • No generated build output or secrets committed
  • Neutral issue reference; issue closure left to maintainers

Related issues

Refs #2708

@kavemang
kavemang marked this pull request as ready for review October 11, 2026 02:35
@ryanbr

ryanbr commented Oct 15, 2026

Copy link
Copy Markdown
Owner

Reviewed and verified locally. Green at e960c4a, 6/6.

setAnswers and migrate were both test-only on both platforms, so the removals are safe and the build agrees.

The Swift test rework improves on what was there. The old setUp cleared live testcentre.* keys out of UserDefaults.standard and never put them back, so running the suite wiped a developer's real Test Centre state; the save and restore pair now leaves it as it found it. freshDefaults() was dead code, building a suite nothing used. testAnswersRejectsMalformedPersistedQuestionnaire is new ground, since nothing previously asked what a corrupt blob does to the reader.

Two things worth raising, neither of them blocking.

The first is the one I would act on. TestCentre.answers(_:) is not an orphan after this PR, it is a reader with no writer. TestBundleAssembler.swift:320 puts it into every exported bundle as meta.json's questionnaire field, and the comment at :304 describes the contents as "whatever the tester saved for this profile". Nothing in the app ever saved it: setAnswers was the only writer on either platform, and its only callers were these two tests. So that block has been empty in every bundle anyone has ever exported, and once this merges there is no writer left in the tree at all. The removal is still correct, and the dead end is much older than this PR, but it deserves its own issue before the last trace that the field was meant to be writable disappears. Either the Test Centre grows the questionnaire step the assembler expects, or the field comes out of the bundle schema.

The second is smaller. activationPreservesUnrelatedKeys on the Kotlin side asserts that writing one key in a SharedPreferences file leaves another key in the same file alone, which is close to tautological. In fairness the test it replaces was worse: it built a legacy store, passed a different store to TestCentre, called migrate(), then asserted legacy was untouched, which no implementation could have failed. The Swift twin has teeth because PuffinExperiment.deepDataKey is a genuinely separate namespace. Pointing the Kotlin one at a real legacy prefs file would restore the parity the file header claims.

Also worth a note: the Swift malformed loop carries three inputs and the Kotlin one carries two, with no counterpart for {"answer":null}. Probably deliberate given how org.json handles a JSON null differently from JSONDecoder, but since the header promises a value-for-value mirror it is worth either adding the case or saying why it is absent.

@ryanbr
ryanbr merged commit f495123 into ryanbr:main Oct 15, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants