Skip to content

Fix store screen tests for gender-filtered catalog rows - #46

Merged
ucguy4u merged 3 commits into
mainfrom
cursor/fix-store-screen-tests-cf67
Nov 2, 2026
Merged

ucguy4u merged 3 commits into
mainfrom
cursor/fix-store-screen-tests-cf67

Conversation

@ucguy4u

@ucguy4u ucguy4u commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes main CI store widget tests after the catalog grew to 11 items and gender-aware row filtering landed in 5eeb347.

Root cause: Tests iterated the full cosmeticCatalog while StoreScreen filters listings by avatar gender (e.g. Twin Buns is female-fitted and hidden for male).

Changes

  • Export storeDisplayRows and derive most test expectations from it; useStoreListViewport sizes the test viewport from row count.
  • Literal gender tests (per architect review): hard-coded visible/hidden item names and list lengths for male and female identities — independent of storeDisplayRows, so mutants that drop or invert the gender gate fail.
  • _storeTestGender() uses resolveAvatarGender(null).
  • TODOS.md: one line documenting cosmetic gender gating.

Verification

  • melos run analyze / melos run test
  • Architect re-review on e512988: block cleared; M1/M5/M6/M7 mutants killed by literal tests
Open in Web Open in Cursor 

Store widget tests iterated the full cosmeticCatalog, but the screen
filters listings by avatar gender (e.g. Twin Buns is female-fitted).
Derive expectations from storeDisplayRows and size the viewport from
row count so lazy ListView builds every visible row.

Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>

@ucguy4u ucguy4u left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested and blocking

Reviewed head ee4d8996093700e96af3f87dc86ce9e1d2c642d3 (base main @ 5eeb347c). There is no earlier architect review on this PR, so nothing is superseded.

Blocking: the updated tests no longer pin the gender gate, so a broken gate would pass. The screen and the tests now both call storeDisplayRows, and nothing asserts a literal list or count. I ran the PR's app/test/store/store_screen_test.dart against mutants of storeDisplayRows / cosmeticListingVisibleForGender (full results below):

mutant result
M1: drop the gender filter, visible = catalog.toList() tests still pass (18/18). Twin Buns would show for male.
M5: invert the male rule (hide hair_simple_parted / hair_buzzed for male instead of hair_buns) tests still pass (18/18)
M2: series block first, then unseriesed items fails (2 tests)
M3: drop series grouping, no headers and no series items fails (7 tests)
M3b: flat list, no headers fails (5 tests)
M4: series header after its items fails (1 test)

So ordering and grouping are guarded, but only by the on-screen position and header assertions that already existed. Gender filtering has no guard. Nothing, before or after this PR, asserts that male hides Twin Buns, that female hides the beard and the male-fitted hair, or what the row counts are. Before this PR the tests looped over the raw catalog, so the gate was unguarded too. This PR is what makes a new gate test necessary, because the tests no longer touch the raw catalog.

What to add (small). One literal test per gender that builds the real StoreScreen with Identity(avatarGender: 'male' | 'female') in InMemoryIdentityRepository, using the literal names:

  • Male: 10 items shown: Classic Frame, Signature Tee, Clean Part, Buzz Cut, Village Tunic, Village Trousers, Leather Boots, Riviera Optics, Long Flow, Full Beard. Twin Buns is findsNothing. storeDisplayRows(...).length == 11 (10 items + 1 series header).
  • Female: 8 items shown: Classic Frame, Signature Tee, Twin Buns, Village Tunic, Village Trousers, Leather Boots, Riviera Optics, Long Flow. Clean Part, Buzz Cut and Full Beard are findsNothing. Row count is 9.

I wrote this as a scratch test. It passes on this head and fails under M1 and M5. Keep the existing derived assertions too. Also make _storeTestGender() come from resolveAvatarGender(null) rather than a literal AvatarGender.male, so the "default identity → male" assumption is tested rather than restated.

Verified, no concerns:

  1. Root cause is real, with no hidden screen bug. I reproduced main @ 5eeb347c locally. Both failures are Found 0 widgets with text "Twin Buns", in "lists every catalog item…" and "series items render after unseriesed items…". Commit 5eeb347 added the Twin Buns listing and the male-hides-hair_buns rule in the same commit, without touching store_screen_test.dart. The other 16 tests pass on main. On this head, all 18 pass.
  2. UI behavior: none changed. The store_screen.dart diff is a rename _storeRows → storeDisplayRows plus a doc comment, and the call site is updated. The body is unchanged. Thermion, packages/** and pubspec are untouched. The 3 files are store_screen.dart, test/pump_helpers.dart and test/store/store_screen_test.dart.
  3. CI: flutter and rust are both success on run 41390089507, whose headSha is exactly ee4d8996. The PR is non-draft.
  4. API placement: acceptable. storeDisplayRows stays in app/lib/store/store_screen.dart as a test seam. It can't move to packages/store, because the filter depends on maleFittedHairAssetIds / femaleFittedHairAssetIds in app/lib/avatar/avatar_defaults.dart. DESIGN.md says nothing about code layout. Optional nit: a small store_display_rows.dart would keep a screen file from being the public API. The existing doc comment "Unseriesed items first…" sits above cosmeticListingVisibleForGender instead of the rows function. That predates this PR, but it's a good moment to fix.
  5. No conflict with amiro#45. #45 (head e0bf9237) touches only packages/store/lib/src/collection_highlights.dart and collection_progress.dart and their tests. It removes completedSeries, isSetComplete and collectedCosmeticIds. storeDisplayRows uses only collectionProgress, which #45 keeps, and nothing in app/ uses the removed APIs.
  6. Gender gating is undocumented. It's product behavior (female hides the beard and male-fitted hair, male hides Twin Buns). It's described only in code comments (cosmeticListingVisibleForGender, avatar_defaults.dart). It isn't mentioned in DESIGN.md, TODOS.md, docs/product/requirements.md or overview.md. Worth one line in TODOS (store section) when the literal test is added. Non-blocking.

Method note: I ran these in a scratch clone with the native thermion build hooks stubbed out, because the box has no clang. The store tests use FakeAvatarRenderer, so nothing depends on the native library. Baseline on this head was 18/18 pass, and main reproduced the 2 failures.

COMMENT only. I have not merged, marked ready or pushed anything. Once the literal gender assertions exist, I'll re-review that head.

Two widget tests assert hard-coded visible/hidden item names and list
row counts for male and female identities, independent of
storeDisplayRows. _storeTestGender() now uses resolveAvatarGender(null).

Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>

@ucguy4u ucguy4u left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architect comments clear on this head — not an approval

Re-reviewed head e5129888a410cb9477c24ebff3b786ab2778cd1f. This supersedes my block, review 5732221370 at ee4d8996. No blocking comments remain.

The block is resolved. Mutant results. I ran the head's app/test/store/store_screen_test.dart: baseline 20/20 pass. Then I ran each mutant of storeDisplayRows / cosmeticListingVisibleForGender in the screen:

mutant before (ee4d899) now (e512988)
M1: drop gender filter survived killed (male and female literal tests fail)
M5: invert male hair rule (hide male-fitted hair for male) survived killed (male literal test fails)
M2: series block before unseriesed items killed killed
M3: drop series grouping and headers killed killed
M3b: flat list, no headers killed killed (the 2 literal tests also fail)
M4: series header after its items killed killed
M6a: female stops hiding the beard (facialHair) not run killed (female test fails)
M6b: female stops hiding male-fitted hair (Clean Part and Buzz Cut) not run killed
M6c: female stops hiding only Buzz Cut not run killed
M7: female also hides Twin Buns (the female-fitted hair) not run killed

The tests assert hard-coded names and counts, not storeDisplayRows.

  • Male: Twin Buns is findsNothing. The 10 literal names are each findsOneWidget. Series #1 · Founders is present. The list's childCount is 2 * 11 - 1.
  • Female: Clean Part, Buzz Cut and Full Beard are findsNothing. The 8 literal names are each findsOneWidget. childCount is 2 * 9 - 1.
  • Both use the real StoreScreen with an Identity(avatarGender: 'male'|'female'). The row counts come from the built ListView delegate, not from storeDisplayRows.
  • _storeTestGender() is now resolveAvatarGender(null). The derived-expectation tests remain in the file, which is fine now that the literal ones exist.

Delta ee4d8996..e5129888: tests and docs only.

  • 2 files: app/test/store/store_screen_test.dart and TODOS.md.
  • app/lib/store/store_screen.dart is identical to the version I reviewed at ee4d8996, so no UI change. Thermion and packages/** are untouched.

TODOS.md line is accurate. It says store rows hide hair/beard items that don't fit the chosen avatar gender, and points at cosmeticListingVisibleForGender in app/lib/store/store_screen.dart and the literal widget tests. That matches the code (female hides the beard and male-fitted hair, male hides Twin Buns). It's placed in the #6 Store section. Nit, not blocking: the surrounding #6 text on this base still describes the old "2 free + 1 paid" catalog and a stubbed entitlement. That's pre-existing and is fixed by amiro#44, not by this PR.

CI is green on this exact SHA. flutter and rust both succeeded on run 41392191842, whose headSha is e5129888. The PR is non-draft.

Still true from my earlier review, with no action needed here: storeDisplayRows being public in the app layer is acceptable, and there's no conflict with amiro#45, which touches only packages/store collection files.

Method: I ran these in a scratch clone with the native thermion hooks stubbed out. The store tests use FakeAvatarRenderer, so nothing depends on the native library.

COMMENT only. I have not merged, marked ready or pushed anything.

Move the unseriesed-then-series doc off cosmeticListingVisibleForGender
and document gender visibility on the filter itself.

Co-authored-by: Uday Chauhan <chauhan.s.uday26@gmail.com>
@ucguy4u
ucguy4u merged commit 6019a61 into main Nov 2, 2026
2 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