Skip to content

test(journal): cover production typed catalog resolution - #2768

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

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

Conversation

@kavemang

@kavemang kavemang commented Oct 11, 2026 •

Copy link
Copy Markdown

What this PR does

Four journal catalog regressions on each platform call unused string-only mergers, so they can stay green when the typed resolver used by the journal UI regresses. Remove both mergeCatalog / mergeJournalCatalog copies and run those assertions through the unchanged production resolvedItems / resolveJournalItems paths and real legacy migration.

Add matching checks for saved canonical/display/type/group/order metadata surviving normalized imported aliases and persistence reload, and for hiding/restoring an imported alias without duplicating its saved item. Inject an optional UserDefaults suite into the Apple catalog store (defaulting to standard) so this test class's catalog mutations use isolated suites with teardown cleanup. Update obsolete helper-name comments in both Insights screens. No scoring, schema, persisted representation or UI styling changes.

Type of change

  • Refactor / cleanup
  • CI / tooling (production regression coverage)

How it was tested

  • Linux Swift harness: 11 committed catalog methods passed against the entire production catalog implementation, using Foundation UserDefaults/JSON persistence. Linux scaffolding supplies ObservableObject/Published and the localized-label constructor; this does not validate Combine publication, Apple app compilation or the unrelated journal-entry/Insights methods in the class.
  • Kotlin harness: 12 committed methods passed against the actual pure production catalog, normalizer and JSON persistence with pinned Kotlin 1.9.24/JDK 17/JUnit 4.13.2 and the real org.json library. This is not a full Android app build.
  • Private mutations discarding saved metadata in Swift and removing hidden filtering in Kotlin were rejected (nine assertions and three test methods respectively).
  • Core Tools: 153 tests passed. Source hygiene, diff-scoped i18n and whitespace pass.
  • Verified unchanged head 83a7e4701ff1454952c29c5d8ce4f44d9530608a: the complete expected six-check roster passed and was confirmed stable twice. Native Android app Kotlin/KSP compilation, APK build and full testFullDebugUnitTest passed; universal macOS Strand and iOS NOOPiOS builds passed.
  • Native macOS StrandTests ran 2,281 tests, with two skipped and zero failures. All 14 JournalLogicTests methods passed explicitly, including both new metadata/reload and hidden-alias cases. iOS evidence is compile-only; its test step is skipped by design. No warning names the changed source/test files in the inspected native logs.
  • Tools Python CI passed capture, parity repository acceptance, legacy R-R preservation and core suites; hygiene and i18n also passed. No package source changed, and app/UI helper removals lie outside scanned parity roots, so no derived parity metadata change is required.
  • CI's merge-current-main steps were skipped; validation covers the actual PR head rather than a hypothetical merge with later main changes.
  • The shipped resolvers and serialization remain unchanged on both platforms; only unused twins were removed. No BLE changes or hardware testing applies.

Checklist

  • Native Android full unit suite passes
  • Both Apple app builds and macOS StrandTests pass
  • No warnings naming changed source/test files in inspected native logs
  • UI uses existing tokens; no styling changes
  • No hardcoded hex frame bytes
  • Both platforms updated
  • Follows docs/CONTRIBUTING.md
  • No generated output or secrets committed

Related issues

Refs #2708

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

ryanbr commented Oct 15, 2026

Copy link
Copy Markdown
Owner

Reviewed and verified locally. Green at 83a7e47, 6/6.

mergeCatalog and mergeJournalCatalog had no production callers on either platform. The only non-test references were three doc comments, and this PR updates all three, so the removal is complete.

Three parts stand out.

The defaults: injection on JournalCatalogStore is the right call and probably the most valuable line in the PR. The previous tests ran JournalCatalogStore() against UserDefaults.standard, so a test run wrote into the real app namespace and whatever a developer had saved locally leaked into the assertions. An isolated suite per test, with a teardown that removes the persistent domain, makes the catalog tests deterministic for the first time.

Adding "Did you take magnesium?\u{00a0}" is real new coverage rather than a reshuffle. The comment above that test has named the non-breaking space as the export's doing since #224, and no test input had ever actually contained one: the old pair only exercised the trailing newline and the doubled interior space. The same string lands on the Kotlin side, so the two stay value-for-value.

testSavedMetadataSurvivesNormalizedImportAndReload is the strongest addition. It pins display name, kind, group and sortIndex across a reload and against an imported alias with different casing and spacing at the same time, which is the combination that would silently fork a renamed item into two rows.

One small thing, take it or leave it. Both rewritten comments drop the "Triage fix (a)/(b)" marker that pointed back at why experimentCandidates deliberately avoids the full resolver. The behavioural reason survives in the new wording, but the pointer does not, and that marker is the only thing in the tree naming that triage. Keeping those four words would preserve the trail.

Good to merge.

@ryanbr
ryanbr merged commit 840adfd 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