Repository navigation
[No QA] Remove unsafe type assertions from SidebarUtilsTest.ts tests - #96674
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
|
| // discriminated union narrowing issues with originalMessage fields. | ||
| const lastAction = { | ||
| ...createRandomReportAction(2), | ||
| const lastAction: ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.ADD_APPROVER_RULE> = { |
There was a problem hiding this comment.
🟡 tests/unit/SidebarUtilsTest.ts:4837: good fix, but confirm the fixture is still complete after dropping the random factory
This rewrite is the right idea: typing the fixture as ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.ADD_APPROVER_RULE> narrows the discriminated union so originalMessage is correctly typed, which is exactly why the old code needed the as ReportAction cast and the explanatory comment. It also surfaced a real fixture bug:
approverAccountIDwas the number12345, but the type isstring(src/types/onyx/OriginalMessage.ts:683), so'12345'is the correct value now
Two things to confirm: first, you dropped ...createRandomReportAction(2) and replaced it with explicit fields, so the fixture now only carries what is listed. The type annotation guarantees the required fields, but any optional field the factory used to set (and that the LHN last-action assertion might read) is now absent, please confirm the assertion does not depend on one of those.
Second, confirm the approverAccountID number to string change does not alter what the rendered last-action message asserts (it should not, since the type always expected a string, but worth a glance).
There was a problem hiding this comment.
Confirmed.
The removed factory fields (message, avatar, automatic, shouldShow, lastModified, and others) are not read by this test or the ADD_APPROVER_RULE handler. The explicit fixture contains every field used to select and render the handler.
approverAccountID is only checked for truthiness and is not included in the rendered message, so correcting it from 12345 to '12345' does not alter the asserted output.
| // @ts-expect-error Intentionally exercise the runtime guard with invalid input. | ||
| report: undefined, |
There was a problem hiding this comment.
🟢 tests/unit/SidebarUtilsTest.ts:1040-1041: @ts-expect-error is the right tool here, flagging for @blimpich
Swapping report: undefined as unknown as Report for a // @ts-expect-error with a description is the correct way to feed a deliberately invalid input into a runtime-guard test: it documents intent and will start failing if the signature ever changes to accept undefined. One note for @blimpich: this technically trades one suppression for another (@ts-expect-error rather than a cast), so if the team wants zero suppressions in these files we should discuss, but for "prove the falsy-report guard returns false" I think it is appropriate.
Nice contrast with getReportsToDisplayInLHN at line 4396, where reports: undefined needed no suppression at all because that param is nullable ✅
| }, | ||
| ], | ||
| }; | ||
| } satisfies TransactionViolationsCollectionDataSet; |
There was a problem hiding this comment.
🟢 tests/unit/SidebarUtilsTest.ts:110, :113, :743, :3308: the type-safe patterns here are exactly right
Calling these out because they are the best examples in the series so far. } satisfies TransactionViolationsCollectionDataSet and } satisfies ReportAction validate the object shape without widening or an unsafe assertion, which is the ideal replacement for as. Typed annotations like const MOCK_TRANSACTIONS: OnyxCollection<Transaction> = {...} instead of {...} as OnyxCollection<Transaction> are the same idea.
And replacing getOriginalMessage<...>(mockIOUAction) as OriginalMessageIOU (plus its eslint-disable) with an if (!originalMessage) throw narrowing removes both the cast and the suppression.
No changes needed, just good to see ✅
| [`${ONYXKEYS.COLLECTION.REPORT}${MOCK_REPORT.reportID}` as const]: MOCK_REPORT, | ||
| }; | ||
| } satisfies ReportCollectionDataSet; | ||
| const onyxReports: ReportCollectionDataSet = MOCK_REPORTS; |
There was a problem hiding this comment.
🟢 tests/unit/SidebarUtilsTest.ts:860 and :950: minor, the onyxReports alias looks redundant
Small one: since MOCK_REPORTS already uses satisfies ReportCollectionDataSet, the extra const onyxReports: ReportCollectionDataSet = MOCK_REPORTS then ...onyxReports in multiSet seems unnecessary, ...MOCK_REPORTS should spread fine given the satisfies guarantees assignability.
Not blocking, just a small simplification if it also works for you locally ✅
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM
This is a strong PR, the cleanest type-work in the batch (real satisfies, discriminated-union generic, runtime narrowing, and it surfaced a genuine number-versus-string fixture bug). Nothing here blocks.
The one thing I would actually want an answer on is the 🟡 comment: dropping ...createRandomReportAction(2) for a hand-built fixture can quietly drop fields the assertion relied on, so a quick confirm from the author that the explicit fixture is complete is worth getting before we merge.
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.46-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
Removed
@typescript-eslint/no-unsafe-type-assertionviolations fromtests/unit/SidebarUtilsTest.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/SidebarUtilsTest.ts: 35 violationsAfter:
tests/unit/SidebarUtilsTest.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 Jest passed all 112 SidebarUtils tests. Target lint, writable TSV verification, formatting, React Compiler applicability, and diff hygiene passed on the exact signed revision. Trusted Fork CI also passed.
Offline tests
Not applicable because this is a unit-test-only type-safety cleanup with no production network or offline behavior change.
QA Steps
Not applicable for manual staging or production QA because production and UI code are unchanged; the affected behavior is covered by the focused unit suite and trusted CI.
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 on Android Native, Android mWeb Chrome, iOS Native, iOS mWeb Safari, and MacOS Chrome/Safari because only unit-test implementation changed and no platform runtime code is modified.
Not applicable because the change has no user-interface, visual, asset, or styling impact.
Android: mWeb Chrome
Not applicable on Android Native, Android mWeb Chrome, iOS Native, iOS mWeb Safari, and MacOS Chrome/Safari because only unit-test implementation changed and no platform runtime code is modified.
Not applicable because the change has no user-interface, visual, asset, or styling impact.
iOS: Native
Not applicable on Android Native, Android mWeb Chrome, iOS Native, iOS mWeb Safari, and MacOS Chrome/Safari because only unit-test implementation changed and no platform runtime code is modified.
Not applicable because the change has no user-interface, visual, asset, or styling impact.
iOS: mWeb Safari
Not applicable on Android Native, Android mWeb Chrome, iOS Native, iOS mWeb Safari, and MacOS Chrome/Safari because only unit-test implementation changed and no platform runtime code is modified.
Not applicable because the change has no user-interface, visual, asset, or styling impact.
MacOS: Chrome / Safari
Not applicable on Android Native, Android mWeb Chrome, iOS Native, iOS mWeb Safari, and MacOS Chrome/Safari because only unit-test implementation changed and no platform runtime code is modified.
Not applicable because the change has no user-interface, visual, asset, or styling impact.