[No QA] Remove unsafe type assertions from DomainSelectorsTest.ts tests#96518
Conversation
| shared: {}, | ||
| }; | ||
|
|
||
| type DomainBoundarySecurityGroup = { |
There was a problem hiding this comment.
These two test-local types shadow production and don't earn their place.
DomainBoundarySecurityGroup is a hand-loosened copy of DomainSecurityGroup (src/types/onyx/Domain.ts:104, whose shared is Record<string, 'read' | null>). DomainBoundaryValue is a ≈unknown union. Both only type boundaryEntries, and every value there flows through Reflect.set(fixture, key, value) — value: any, so the static type is erased at the write and never asserted against. Net effect ≈ Record<string, unknown>, plus a drift liability: change DomainSecurityGroup and this copy silently rots (the same flaw flagged on GuidedSetupItem in #96520).
Suggest: type boundaryEntries: Record<string, unknown> and delete both types. Where a named loose shape genuinely helps, use Partial<DomainSecurityGroup> — production itself already does this at Domain.ts:131.
– written by Claude on Ben's behalf
There was a problem hiding this comment.
Ben here, claude commented but just adding my actual voice here. I think that creating a new type here is a code smell.
There was a problem hiding this comment.
Fixed in 71fa790.
I removed DomainBoundarySecurityGroup and DomainBoundaryValue, and changed boundaryEntries to Record<string, unknown>. This keeps malformed boundary inputs explicit at the Reflect.set boundary without maintaining test-local copies of production models.
|
|
||
| type DomainBoundaryValue = BaseVacationDelegate | DomainBoundarySecurityGroup | boolean | number | string | null | undefined; | ||
|
|
||
| type DomainSecurityGroupPendingActionsBoundary = { |
There was a problem hiding this comment.
This type invents a top-level pendingAction field on a security-group pending-action entry — and I traced it end to end: no code path, client or server, ever produces that shape. The production type DomainSecurityGroupPendingActions correctly has no such field, and it should stay that way.
Evidence:
- The App is the only writer of the DOMAIN_PENDING_ACTIONS store. The backend never writes it (
COLLECTION_DOMAIN_PENDING_ACTIONShas zero hits in Web-Expensifylib; on failure it writes onlycreateGroupErrors/deleteGroupErrorsto DOMAIN_ERRORS,Auth.php:8337,8359).pendingActionis a client-side offline marker. - Every App write for a security group uses a named field, never a top-level
pendingAction: update →{[settingsName]: UPDATE}(src/libs/actions/Domain.ts:1932), delete →{deleteGroup: DELETE}(:2027), create →{createGroup: ADD}(:2256).
So this boundary type exists only to construct a shape production correctly rejects. (Likely origin: the member pending-action variant does have a top-level pendingAction via GeneralDomainMemberPendingAction — Domain.ts:1078 — and got copied onto the security-group case.)
The fix is in the test, not the type — see the comment on the pendingAction: DELETE line below.
– written by Claude on Ben's behalf
There was a problem hiding this comment.
Fixed in 71fa790.
I removed DomainSecurityGroupPendingActionsBoundary entirely. The security-group pending-action fixtures now use the production-supported named fields instead of inventing a top-level pendingAction.
| } as unknown as OnyxEntry<DomainPendingActions>; | ||
| const domainPendingActions = createFixture(domainPendingActionsFixture); | ||
| const groupPendingActions: DomainSecurityGroupPendingActionsBoundary = { | ||
| pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, |
There was a problem hiding this comment.
{pendingAction: DELETE} on a security-group entry is a shape that never ships. The real "group is being deleted" shape the App writes is {deleteGroup: DELETE} (src/libs/actions/Domain.ts:2027). Every delete-related case in this block uses the fictional pendingAction form, so the suite never exercises the shape that actually occurs — and isSecurityGroupPendingDeleteSelector only returns true here by accident, because Object.values(group).some(a => a === DELETE) happens to catch any DELETE-valued field.
Replace these inputs with the real fields ({deleteGroup: DELETE}, {createGroup: ADD}, {[settingName]: UPDATE}). They type-check against production with no boundary type at all, which deletes DomainSecurityGroupPendingActionsBoundary and makes the suite finally cover the real data.
This fiction predates the PR (the old code used {pendingAction: DELETE} as unknown as ...), so this is arguably beyond a mechanical assertion-cleanup — fine to fix here or in a fast follow-up, but the boundary type is the smoking gun that surfaced a real coverage gap.
– written by Claude on Ben's behalf
There was a problem hiding this comment.
Fixed in 71fa790.
I replaced the fictional top-level pendingAction inputs with production-conforming shapes: deleteGroup: DELETE, createGroup: ADD, and named setting fields for update/delete actions. The tests now exercise the shapes written by the App while preserving the existing test-case identities.
| shared: {}, | ||
| }; | ||
|
|
||
| type DomainBoundarySecurityGroup = { |
There was a problem hiding this comment.
Ben here, claude commented but just adding my actual voice here. I think that creating a new type here is a code smell.
Reviewer Checklist
|
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
Type-safety audit (the part that matters for this issue)
I checked every way an implementer could make the number drop without actually improving safety. All clear:
| Check | Result |
|---|---|
eslint-disable @typescript-eslint/no-unsafe-type-assertion added anywhere |
None |
New as X / as unknown as X double-casts introduced |
None (only 'domain_defaultSecurityGroupID' as const, a const assertion the rule does not flag) |
| Seatbelt baseline for this file | Confirmed 56 in config/eslint/eslint.seatbelt.tsv:1981, so 56 -> 0 is credible |
| 88 test scenarios preserved | Ran npx jest tests/unit/DomainSelectorsTest.ts -> 88 passed |
Two things worth calling out as actual improvements, not just relocations of unsafety:
- The well-formed entry options in
DomainFixtureOptionsare properly typed:admins?: Array<[string, number]>,securityGroups?: Array<[string, DomainSecurityGroup]>,vacationDelegates?: Record<number, BaseVacationDelegate>,lockedAccounts?: Record<number, boolean>. The old... as unknown as OnyxEntry<Domain>threw away all value checking; the new builder enforces the value types. Net safety went up, not sideways. createFixture(fixture, overrides)usesPartial<T>, so the pending-action / errors overrides are type-checked against the real model now.
The one spot that could have hidden a cheat — the renamed keys — is legitimate. In isSecurityGroupPendingDeleteSelector tests the implementer changed pendingAction: ... to createGroup / deleteGroup / enableStrictPolicyRules. I confirmed against both the selector and the type:
src/selectors/Domain.ts:234checksObject.values(groupPendingActions ?? {}).some((action) => action === DELETE)— it inspects values, not keys, so the key name is irrelevant to the result.deleteGroup,createGroup,name,enableStrictPolicyRulesare all real fields ofDomainSecurityGroupPendingActions(src/types/onyx/DomainPendingActions.ts). The oldpendingActionkey was not a valid field on that type, which is exactly why theas unknown aswas needed. Swapping to real fields is the correct fix, and behavior is preserved because the selector is value-based.
The boundaryEntries: Record<string, unknown> + Reflect.set escape hatch is reserved only for deliberately malformed / foreign-prefix keys (the "ignore keys that don't match the prefix", null/undefined, someOtherKey cases). That is unavoidable — those tests exist to prove the selector ignores garbage — and scoping the untyped injection to just those entries while keeping the base fixture typed is the honest way to do it.
Non-blocking findings
🟠 Seatbelt TSV is intentionally not committed — confirm the count actually locks in. The baseline row still reads 56, and the whole payout is measured by that count. Confirm the repo's post-merge automation actually tightens 56 -> 0 on main (the author's stated assumption). If it does not, the baseline stays loose (allows up to 56 again) and the reduction is not locked in even though the file is clean. This is a process risk, not a code issue, but it is the one item most worth confirming for this payout structure.
🟡 Base-fixture pollution changes what the fixtures represent. createDomainFixture() seeds every non-empty domain with {validated: true, accountID: 1, email: 'test@example.com', domain_defaultSecurityGroupID: ''}. Tests like the admin / member / vacation-delegate ones previously used bare objects with only the prefixed keys under test. It is harmless today because every selector filters by key prefix, so the four base props are always ignored, and all 88 tests pass. But it weakens the 1:1 correspondence between the fixture and the scenario, and it is a latent trap: the moment a selector reads validated or accountID, a test that meant "bare domain" would silently carry values. Preferred fix: have the builder start from {} and add base identity fields only when a test opts in (the selectSecurityGroupForAccount cases that need validated/accountID/email).
🟢 Readability trade-off. The createDomainFixture builder (9 options, Reflect.set / Reflect.deleteProperty loops) is heavier than the inline literals it replaces. For test data, inline literals are often easier to review because you see the exact shape. This is the cost the issue mandates, so acceptable, but the file did get harder to skim.
🟢 createFixture returns T after deleting a required prop. e.g.
const domainMemberSharedNVP = createFixture(cardFeedsFixture);
Reflect.deleteProperty(domainMemberSharedNVP, 'settings');The variable is still statically typed CardFeeds (with settings) while the runtime object has no settings. It works because the selector optional-chains, but the static type now lies. The safety here is runtime-honest, not type-honest. Minor, tolerable in a test.
cc @KJ21-ENG
✅ Net: Clean, honest work that removes 56 assertions the right way. The 🟡 / 🟢 items are polish, and the 🟠 is a process confirmation rather than a code defect, no merge-blockers.
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.44-0 🚀
|
4 participants
Explanation of Change
Removed
@typescript-eslint/no-unsafe-type-assertionviolations fromtests/unit/DomainSelectorsTest.tsas part of the cleanup for Expensify/App issue #94739.What changed:
Why this approach is safe:
Reviewer focus:
Safety checks:
Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Count change
Current baseline source:
config/eslint/eslint.seatbelt.tsvBefore:
tests/unit/DomainSelectorsTest.ts: 56 violationsAfter:
tests/unit/DomainSelectorsTest.ts: 0 violationsNet reduction:
@typescript-eslint/no-unsafe-type-assertionviolations.TSV evidence:
main.Tests
Run:
Verify focused lint passes with no target-rule warnings for the edited file.
Run:
Verify the generated seatbelt row reflects 0 violations, then restore
config/eslint/eslint.seatbelt.tsvbefore committing.Run:
Verify the focused test suite passes.
Verification result: Focused lint, CI lint and TSV generation, all 88 DomainSelectorsTest cases, formatting, React Compiler, diff hygiene, and trusted Fork CI passed.
Offline tests
Not applicable: this test-only fixture refactor does not change offline application behavior.
QA Steps
Run the focused lint and DomainSelectorsTest commands above. No staging or production QA is required because application runtime behavior is unchanged.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Not applicable: this test-only change does not modify runtime behavior on any platform.
Screenshots and videos are not applicable because there are no user-visible changes.
Android: mWeb Chrome
Not applicable: this test-only change does not modify runtime behavior on any platform.
Screenshots and videos are not applicable because there are no user-visible changes.
iOS: Native
Not applicable: this test-only change does not modify runtime behavior on any platform.
Screenshots and videos are not applicable because there are no user-visible changes.
iOS: mWeb Safari
Not applicable: this test-only change does not modify runtime behavior on any platform.
Screenshots and videos are not applicable because there are no user-visible changes.
MacOS: Chrome / Safari
Not applicable: this test-only change does not modify runtime behavior on any platform.
Screenshots and videos are not applicable because there are no user-visible changes.