[No QA] Clean up unsafe type assertions in unit tests - #97360
Conversation
|
@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
|
| if ( | ||
| typeof accountExecutiveDetail !== 'object' || | ||
| accountExecutiveDetail === null || | ||
| !('accountID' in accountExecutiveDetail) || | ||
| typeof accountExecutiveDetail.accountID !== 'number' || | ||
| !('avatar' in accountExecutiveDetail) || | ||
| typeof accountExecutiveDetail.avatar !== 'string' | ||
| ) { | ||
| throw new Error('Expected Account Executive personal details'); | ||
| } |
There was a problem hiding this comment.
🟢 ReportUtilsTest.ts:956: The inline narrowing block is heavier than the assertion needs
This is a lot of hand-rolled in/typeof narrowing inline in the test body to read two fields.
personalDetailsCall[1] is a merge into ONYXKEYS.PERSONAL_DETAILS_LIST, so its value type is known, a small typed helper (e.g. narrow the merge value to PersonalDetailsList and pull Object.values(...).at(0)) would read cleaner and keep this idiom reusable across the other merge-call assertions in this file, rather than open-coding the guard once here.
Not blocking, but if this narrowing shape recurs it belongs in tests/utils, not copy-pasted.
Also minor: after the guard you switched avatar to non-optional access (accountExecutiveDetail.avatar) but left accountExecutiveDetail?.accountID optional two lines down, pick one now that it's narrowed.
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Only 1 non-blocking comment above ☝️
Large but disciplined test-only cleanup (172 no-unsafe-type-assertion removals across ten files). I ran all ten suites against the checked-out head (b93b978a070, the PR's exact head): ReportUtils + TransactionUtils gave 1459/1459, and the other eight are green under UTC.
Three ImportTransactions date tests fail only under my local PDT timezone (created: "2024-01-15" vs "2024-01-14"); they pass under TZ=UTC, the diff never touches date logic, and CI is UTC-green, so that's an environment artifact, not the PR.
Nothing here blocks.
Explanation of Change
This test-only cleanup removes 172
@typescript-eslint/no-unsafe-type-assertionfindings from the ten authorized unit-test files. It replaces unsafe assertions with production-derived types, typed mocks and fixtures, and narrow type-safe handling for intentional malformed runtime fixtures while preserving the existing test scenarios and runtime behavior.Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Count change
Current baseline source:
config/eslint/eslint.seatbelt.tsvat base2c95456c78d65e54730dcde1f758292472ab59farecords the baseline for@typescript-eslint/no-unsafe-type-assertion.Before:
tests/unit/BankAccountUtilsTest.ts: 16 findingstests/unit/ImportTransactions.test.ts: 16 findingstests/unit/ReportUtilsTest.ts: 19 findingstests/unit/TransactionUtilsTest.ts: 20 findingstests/unit/ViolationUtilsTest.ts: 20 findingstests/unit/hooks/useAssignCard.test.ts: 16 findingstests/unit/hooks/useFilterFormValues.test.ts: 15 findingstests/unit/hooks/useSearchBulkActionsDuplicateTest.ts: 19 findingstests/unit/libs/SplitExpenseUtils.test.ts: 15 findingstests/unit/useAccountIndicatorChecksTest.ts: 16 findingsAfter:
tests/unit/BankAccountUtilsTest.ts: 0 findingstests/unit/ImportTransactions.test.ts: 0 findingstests/unit/ReportUtilsTest.ts: 0 findingstests/unit/TransactionUtilsTest.ts: 0 findingstests/unit/ViolationUtilsTest.ts: 0 findingstests/unit/hooks/useAssignCard.test.ts: 0 findingstests/unit/hooks/useFilterFormValues.test.ts: 0 findingstests/unit/hooks/useSearchBulkActionsDuplicateTest.ts: 0 findingstests/unit/libs/SplitExpenseUtils.test.ts: 0 findingstests/unit/useAccountIndicatorChecksTest.ts: 0 findingsNet reduction:
@typescript-eslint/no-unsafe-type-assertionfindings across the ten target files.TSV evidence:
b93b978a070c58b9b87fc82d6628b0210d56e3eapreserves that verified content.Tests
git diff --checkfor all ten targets.30562977469passed typecheck, lint, format, React Compiler, and Jest for exact headb93b978a070c58b9b87fc82d6628b0210d56e3ea.Offline tests
N/A — this is a test-only type-safety cleanup; it changes no application runtime or network behavior, and no manual offline test was performed.
QA Steps
N/A — this is a test-only cleanup and the title starts with
[No QA]; no staging or production manual QA was performed.PR Author Checklist
Checklist note: every item below is checked to record that it was considered. Platform, staging or production, offline, screenshot or video, high-traffic, console, and product-component testing are N/A for this test-only cleanup; no manual platform or product testing is claimed. The automated evidence is listed in
### Tests.### 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 UI changes; no screenshots or videos were produced.
Changed files (10)
tests/unit/BankAccountUtilsTest.tstests/unit/ImportTransactions.test.tstests/unit/ReportUtilsTest.tstests/unit/TransactionUtilsTest.tstests/unit/ViolationUtilsTest.tstests/unit/hooks/useAssignCard.test.tstests/unit/hooks/useFilterFormValues.test.tstests/unit/hooks/useSearchBulkActionsDuplicateTest.tstests/unit/libs/SplitExpenseUtils.test.tstests/unit/useAccountIndicatorChecksTest.ts