Repository navigation
Fix store screen tests for gender-filtered catalog rows - #46
Conversation
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
left a comment
There was a problem hiding this comment.
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:
- Root cause is real, with no hidden screen bug. I reproduced
main@5eeb347clocally. Both failures areFound 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_bunsrule in the same commit, without touchingstore_screen_test.dart. The other 16 tests pass onmain. On this head, all 18 pass. - UI behavior: none changed. The
store_screen.dartdiff is a rename_storeRows→storeDisplayRowsplus a doc comment, and the call site is updated. The body is unchanged. Thermion,packages/**andpubspecare untouched. The 3 files arestore_screen.dart,test/pump_helpers.dartandtest/store/store_screen_test.dart. - CI:
flutterandrustare both success on run 41390089507, whoseheadShais exactlyee4d8996. The PR is non-draft. - API placement: acceptable.
storeDisplayRowsstays inapp/lib/store/store_screen.dartas a test seam. It can't move topackages/store, because the filter depends onmaleFittedHairAssetIds/femaleFittedHairAssetIdsinapp/lib/avatar/avatar_defaults.dart. DESIGN.md says nothing about code layout. Optional nit: a smallstore_display_rows.dartwould keep a screen file from being the public API. The existing doc comment "Unseriesed items first…" sits abovecosmeticListingVisibleForGenderinstead of the rows function. That predates this PR, but it's a good moment to fix. - No conflict with amiro#45. #45 (head
e0bf9237) touches onlypackages/store/lib/src/collection_highlights.dartandcollection_progress.dartand their tests. It removescompletedSeries,isSetCompleteandcollectedCosmeticIds.storeDisplayRowsuses onlycollectionProgress, which #45 keeps, and nothing inapp/uses the removed APIs. - 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.mdoroverview.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
left a comment
There was a problem hiding this comment.
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 BunsisfindsNothing. The 10 literal names are eachfindsOneWidget.Series #1 · Foundersis present. The list'schildCountis2 * 11 - 1. - Female:
Clean Part,Buzz CutandFull BeardarefindsNothing. The 8 literal names are eachfindsOneWidget.childCountis2 * 9 - 1. - Both use the real
StoreScreenwith anIdentity(avatarGender: 'male'|'female'). The row counts come from the builtListViewdelegate, not fromstoreDisplayRows. _storeTestGender()is nowresolveAvatarGender(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.dartandTODOS.md. app/lib/store/store_screen.dartis identical to the version I reviewed atee4d8996, so no UI change. Thermion andpackages/**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>
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
cosmeticCatalogwhileStoreScreenfilters listings by avatar gender (e.g. Twin Buns is female-fitted and hidden for male).Changes
storeDisplayRowsand derive most test expectations from it;useStoreListViewportsizes the test viewport from row count.storeDisplayRows, so mutants that drop or invert the gender gate fail._storeTestGender()usesresolveAvatarGender(null).Verification
melos run analyze/melos run teste512988: block cleared; M1/M5/M6/M7 mutants killed by literal tests