[No QA] Reduce unsafe type assertions in ReportPreviewActionUtils tests - #96778
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
|
| reportID: `${REPORT_ID}`, | ||
| status: CONST.TRANSACTION.STATUS.PENDING, | ||
| amount: 10, | ||
| merchant: 'Merchant', | ||
| date: '2025-01-01', | ||
| } as unknown as Transaction; | ||
| created: '2025-01-01', |
There was a problem hiding this comment.
🟢 tests/actions/ReportPreviewActionUtilsTest.ts:349 and :786: good catch on date to created
Nice, this is a real fixture bug the old cast was hiding. date is not a top-level Transaction field (the only date in the type is on ReservationTimeDetails, the trip reservation object), so {date: '2025-01-01'} as unknown as Transaction was setting a property that production code never reads.
createMock<Transaction> correctly rejected it, and created is the right field (src/types/onyx/Transaction.ts:524). I checked that ReportPreviewActionUtils does not read created, so both pending tests still exercise the same path, they just have a valid date now instead of a dead one.
No change needed, just confirming it is correct ✅
| const report = { | ||
| reportID: REPORT_ID, | ||
| const report = createMock<Report>({ | ||
| reportID: `${REPORT_ID}`, |
There was a problem hiding this comment.
🟢 tests/actions/ReportPreviewActionUtilsTest.ts:75: reportID string fix is correct
REPORT_ID is 1 (a number, line 40) but Report.reportID is a string, so the old reportID: REPORT_ID was a number-vs-string mismatch that as unknown as Report hid. Wrapping it as `${REPORT_ID}` is the right fix.
The report is merged under the same key and the assertions check the returned action, not reportID, so no behavior change - correct ✅
| @@ -71,14 +71,14 @@ describe('getReportPreviewAction', () => { | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
🟢 general: createMock<T> usage is consistent and safe
Same sanctioned createMock<T> pattern as the other PRs, including createMock<NetSuiteConnection>({}) for the connection fixture. The partials are type-checked against the real models, which is exactly what surfaced the two bugs above.
This is the direction we wanted for the series, nothing to change ✅
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - This one is a clean approve, no blockers, no nits worth holding on.
@blimpich Worth noting: this cleanup surfaced two genuine (if minor) fixture bugs, a numeric reportID and a non-existent date field, which is good evidence that removing these casts is finding real problems, not just shuffling types.
All the fixtures still pass and exercise the same behavior ✅
blimpich
left a comment
blimpich
left a comment
There was a problem hiding this comment.
Nice and simple! Looks great 👍
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.45-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.45-14 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
Reduced
@typescript-eslint/no-unsafe-type-assertionviolations intests/actions/ReportPreviewActionUtilsTest.tsas part of the cleanup forExpensify/App issue #94739.
What changed:
createMock<T>helper.createdpropertyinstead of the unsupported
dateproperty.Why this approach is safe:
createMock<T>validates fixture fields against the real TypeScript model.behavior.
Fixed Issues
$ #94739
PROPOSAL: N/A — test-only cleanup.
Count change
Current baseline source:
config/eslint/eslint.seatbelt.tsvBefore:
tests/actions/ReportPreviewActionUtilsTest.ts: 28 violationsAfter:
tests/actions/ReportPreviewActionUtilsTest.ts: 0 violationsNet reduction:
@typescript-eslint/no-unsafe-type-assertionviolations.The generated TSV change is intentionally not included because Expensify/App
post-merge automation updates the baseline on
main.Offline tests
Not applicable. This is a test-only cleanup with no network or product behavior
changes.
QA Steps
No QA required. This PR only changes test fixtures and type-safe test setup.
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.All platform, staging, production, offline, high-traffic, UI, asset, deeplink,
markdown, generic-component, and Storybook items were considered and are not
applicable because this is a test-only cleanup.
Screenshots/Videos
Not applicable. This is test-only cleanup; no UI or visual behavior changed.
Android: Native
Not applicable.
Android: mWeb Chrome
Not applicable.
iOS: Native
Not applicable.
iOS: mWeb Safari
Not applicable.
MacOS: Chrome / Safari
Not applicable.
cc @blimpich