[No QA] Remove unsafe type assertions from cleanup tests - #97286
[No QA] Remove unsafe type assertions from cleanup tests#97286KJ21-ENG wants to merge 2 commits into
Conversation
KJ21-ENG
changed the title
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
|
| originalTransaction: undefined, | ||
| shouldShowRBR: true, | ||
| }; | ||
| // @ts-expect-error - This scenario deliberately passes a transaction without an amount to exercise the missing-amount branch. |
There was a problem hiding this comment.
🟡 TransactionPreviewUtils.test.ts: @ts-expect-error on the call line is broad
Because functionArgs is an untyped local, the type error only surfaces at the call, so the directive has to sit here. The tradeoff: @ts-expect-error suppresses every type error on that line, not just the intended missing-amount one. If functionArgs later drifts and gains an unrelated mismatch, it'll be silently absorbed and the test could pass for the wrong reason.
Not worth blocking, but if you want it tight, annotate functionArgs with the real arg type so the expected error is scoped to the transaction property.
Same applies belon on the errors: {a: 404, ...} case line, though that one is already narrowed to the single property ✅
| transactionParams: mockTransactionParams as PerDiemExpenseTransactionParams, | ||
| participantParams: mockParticipantParams as RequestMoneyParticipantParams, |
There was a problem hiding this comment.
🟢 PerDiemTest.ts: two blocks still use raw as while siblings were migrated
Lines 603-604 and 692-693 keep:
transactionParams: mockTransactionParams as PerDiemExpenseTransactionParams,
participantParams: mockParticipantParams as RequestMoneyParticipantParams,The mocks these point at (mockTransactionParams at 582/667) are untyped incomplete literals, so satisfies would fail here, which is presumably why they were left.
But createMock<PerDiemExpenseTransactionParams>({...}) accepts the partial and still type-checks it, so it's the consistent replacement and matches what you did in the sibling cases in the same file.
Since these as casts aren't caught by the seatbelt rule they're technically in scope-compliant, but leaving two of the same pattern un-migrated reads as an oversight.
|
|
||
| const opts = jest.mocked(Navigation.revealRouteBeforeDismissingModal).mock.calls.at(0)?.at(1) as {afterTransition: () => void} | undefined; | ||
| opts?.afterTransition(); | ||
| const opts = jest.spyOn(Navigation, 'revealRouteBeforeDismissingModal').mock.calls.at(0)?.[1]; |
There was a problem hiding this comment.
🟢 submitDismissStrategiesTest.ts: mixed .mock.calls access styles
The cast removal here really comes from switching .at(N) (which unions all tuple element types) to precise ?.[N] bracket indexing. But you also flipped the accessor from jest.mocked(...) to jest.spyOn(Navigation, 'revealRouteBeforeDismissingModal').mock.calls... for two cases, while dismissModal at lines 76/214 stays on jest.mocked(...).
jest.mocked(Navigation.revealRouteBeforeDismissingModal).mock.calls.at(0)?.[1] should give the same precise typing without installing a spy, and would keep all the .mock.calls reads in this file on one idiom.
Using spyOn purely as a read-accessor is slightly misleading about intent. Worth a quick check — if jest.mocked types cleanly there, prefer it for consistency.
Minor related note: jest.spyOn(API, 'write') in MergeTransactionTest and the spyOn calls here have no restoreAllMocks/afterEach restore. They call through so behavior is preserved, but the spies persist on the real singletons across the file. beforeEach's jest.clearAllMocks() clears history but doesn't uninstall.
Fine as-is since each spy is read once, just flagging it.
|
@KJ21-ENG (3) comments above to be addressed before approval 🙌 |
2 participants
Explanation of Change
This test-only cleanup removes unsafe type assertions from the ten authorized issue 94739 test files while preserving production-derived types, mock signatures, fixture meaning, and runtime behavior.
Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Tests
git diff --check.This is a test-only type-safety cleanup. Manual app, offline, staging, platform, high-traffic, console, screenshot, and UI checks are N/A because no runtime application behavior changed. No new feature or bug fix was introduced. CSS, assets, markdown, generic-component, Storybook, deeplink, and post-merge scenarios are also N/A.
Count change
Current baseline source:
config/eslint/eslint.seatbelt.tsvat base2c95456c78d65e54730dcde1f758292472ab59fa, rule@typescript-eslint/no-unsafe-type-assertion.Before:
tests/actions/IOU/PerDiemTest.ts: 6tests/actions/IOUTest/PayMoneyRequestTest.ts: 6tests/actions/MergeTransactionTest.ts: 6tests/actions/connections/NetSuite.ts: 6tests/ui/ParentNavigationSubtitleTest.tsx: 6tests/unit/Search/SearchQueryUtilsTest.ts: 7tests/unit/TransactionPreviewUtils.test.ts: 7tests/unit/TryNewDotUtilsTest.ts: 7tests/unit/WorkflowUtilsTest.ts: 7tests/unit/submitDismissStrategiesTest.ts: 7After:
tests/actions/IOU/PerDiemTest.ts: 0tests/actions/IOUTest/PayMoneyRequestTest.ts: 0tests/actions/MergeTransactionTest.ts: 0tests/actions/connections/NetSuite.ts: 0tests/ui/ParentNavigationSubtitleTest.tsx: 0tests/unit/Search/SearchQueryUtilsTest.ts: 0tests/unit/TransactionPreviewUtils.test.ts: 0tests/unit/TryNewDotUtilsTest.ts: 0tests/unit/WorkflowUtilsTest.ts: 0tests/unit/submitDismissStrategiesTest.ts: 0Net reduction:
@typescript-eslint/no-unsafe-type-assertionfindings, from 65 to 0.TSV evidence:
Offline tests
N/A — this is a test-only type-safety cleanup with no offline runtime behavior.
QA Steps
N/A — the title is prefixed
[No QA]because this test-only cleanup has no staging or production behavior to validate.PR Author Checklist
All checklist items were considered. Items unrelated to test-only type-safety cleanup are checked as N/A based on the evidence in the Tests, Offline tests, and QA Steps sections.
### 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
N/A — no runtime or UI behavior changed, so screenshots and videos do not apply.
Changed files (10)
tests/actions/IOU/PerDiemTest.tstests/actions/IOUTest/PayMoneyRequestTest.tstests/actions/MergeTransactionTest.tstests/actions/connections/NetSuite.tstests/ui/ParentNavigationSubtitleTest.tsxtests/unit/Search/SearchQueryUtilsTest.tstests/unit/TransactionPreviewUtils.test.tstests/unit/TryNewDotUtilsTest.tstests/unit/WorkflowUtilsTest.tstests/unit/submitDismissStrategiesTest.ts