[No QA] Remove unsafe type assertions from ReportTest.ts tests#96520
Conversation
| }; | ||
|
|
||
| const getMockFetch = (fetch: typeof global.fetch) => fetch as MockFetch; | ||
| const getTransactionProperty = (value: unknown, property: 'pendingAction' | 'convertedAmount'): unknown => { |
There was a problem hiding this comment.
This helper returns unknown, so every expect(getTransactionProperty(...)).toBe(...) below now compares unknown — the assertions lost the type safety the old as OnyxTypes.Transaction gave them. And this value never crosses a serialization boundary: it comes straight from buildOptimisticChangePolicyData and is genuinely a Transaction at runtime, just widened by the OnyxUpdate container. Suggest one type guard narrowing to the existing prod type, then access fields directly:
const isTransactionValue = (value: unknown): value is OnyxTypes.Transaction =>
typeof value === 'object' && value !== null && 'transactionID' in value;
const txn = isTransactionValue(transactionOptimisticData?.value) ? transactionOptimisticData.value : undefined;
expect(txn?.pendingAction).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE);Typed assertions restored, no string-keyed accessor.
– written by Claude on Ben's behalf
There was a problem hiding this comment.
Thanks! Fixed in 4f60886.
I replaced getTransactionProperty(), which returned unknown, with a type guard that narrows the complete update to the production OnyxUpdate MERGE variant.
The assertions now access value.pendingAction and value.convertedAmount directly with production typing.
I used the Onyx transaction key instead of requiring transactionID inside value, because these MERGE payloads are partial transaction updates and do not necessarily include transactionID.
| return undefined; | ||
| }; | ||
|
|
||
| type GuidedSetupItem = { |
There was a problem hiding this comment.
GuidedSetupData and TaskForParameters are already defined and exported from src/libs/actions/Report/index.ts. This local GuidedSetupItem is a lossy subset (type: string vs the discriminated type: 'task' | 'message') and will silently drift from production if a task field changes. Import the real type and narrow to it instead of hand-rolling one in the test.
– written by Claude on Ben's behalf
There was a problem hiding this comment.
Sidenote: the fact that you created a new type in order to make this test pass the typecheck was itself a code smell. We shouldn't be creating new types in a test file in order to pass type checks, that is a sign that something is wrong.
There was a problem hiding this comment.
Thanks! Fixed in 4f60886.
I removed the local GuidedSetupItem type and now import and use the exported production GuidedSetupData type from the Report action.
There was a problem hiding this comment.
Follow-up in 504cf9e0134: CI exposed that Array.find() did not preserve narrowing through the nested production union. I imported the existing exported TaskForParameters type and added an explicit Extract<TaskForParameters, {type: 'task'}> predicate at the three affected lookups. No test-local domain type was introduced.
| completedTaskReportActionID?: string; | ||
| }; | ||
|
|
||
| const parseGuidedSetupData = (value: string): GuidedSetupItem[] => { |
There was a problem hiding this comment.
The runtime narrow here is unavoidable — this value crosses JSON.stringify/JSON.parse (guidedSetupData: string param, serialized at Report/index.ts:1482), so types are erased at the parse point regardless of test vs prod. No objection to a guard. Two asks: (1) narrow to the imported GuidedSetupData, not the local type; (2) make it shallow — the per-field validation and throw are dead code in a test that controls its own input:
const isGuidedSetupData = (v: unknown): v is GuidedSetupData =>
Array.isArray(v) && v.every((i) => typeof i === 'object' && i !== null && 'type' in i);– written by Claude on Ben's behalf
There was a problem hiding this comment.
Agreed—resolved in 4f60886.
The test-only guided-setup type has been removed, so the parser and assertions now stay aligned with the production discriminated union.
There was a problem hiding this comment.
Additional detail: the runtime guard now performs only the requested shallow validation—an array whose entries are non-null objects containing a type property. The per-field validation and custom error were removed. In 504cf9e0134, I also added an explicit predicate using the exported production TaskForParameters type so TypeScript correctly narrows the VIEW_TOUR task without introducing another local type.
| return undefined; | ||
| }; | ||
|
|
||
| type GuidedSetupItem = { |
There was a problem hiding this comment.
Sidenote: the fact that you created a new type in order to make this test pass the typecheck was itself a code smell. We shouldn't be creating new types in a test file in order to pass type checks, that is a sign that something is wrong.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.44-0 🚀
|
3 participants
Explanation of Change
Removed
@typescript-eslint/no-unsafe-type-assertionviolations fromtests/actions/ReportTest.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/actions/ReportTest.ts: 50 violationsAfter:
tests/actions/ReportTest.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: Canonical verification passed focused lint, CI-mode generation, all 259 focused Jest tests, Oxfmt, React Compiler checks, and diff hygiene; trusted Fork CI passed.
Offline tests
Not applicable: this automated-test-only cleanup has no offline runtime behavior.
QA Steps
No manual QA required: only existing automated tests changed, with no user-facing behavior change.
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: the change has no platform-specific runtime behavior.
Not applicable: there are no visual or user-facing changes.
Android: mWeb Chrome
Not applicable: the change has no platform-specific runtime behavior.
Not applicable: there are no visual or user-facing changes.
iOS: Native
Not applicable: the change has no platform-specific runtime behavior.
Not applicable: there are no visual or user-facing changes.
iOS: mWeb Safari
Not applicable: the change has no platform-specific runtime behavior.
Not applicable: there are no visual or user-facing changes.
MacOS: Chrome / Safari
Not applicable: the change has no platform-specific runtime behavior.
Not applicable: there are no visual or user-facing changes.