[No QA] Remove unsafe type assertions from ReportSecondaryActionUtilsTest.ts - #96777
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
|
| _submitterEmail: Parameters<typeof ReportUtils.isWorkspaceEligibleForReportChange>[0], | ||
| policy: Parameters<typeof ReportUtils.isWorkspaceEligibleForReportChange>[1], |
There was a problem hiding this comment.
🟢 tests/unit/ReportSecondaryActionUtilsTest.ts:5388-5390: the Parameters<...> annotations here are redundant
Since MockConfig already types isWorkspaceEligibleForReportChange as boolean | IsWorkspaceEligibleForReportChangeMock, TypeScript contextually types the callback params when you pass it to setupMocks, so you do not need to annotate each param. This can just be:
setupMocks({
isWorkspaceEligibleForReportChange: (_submitterEmail, policy) => policy?.id === POLICY_ID,
});_submitterEmail and policy are still fully typed from the real isWorkspaceEligibleForReportChange signature through IsWorkspaceEligibleForReportChangeMock.
Same type safety, less noise. Minor, not blocking.
|
|
||
| it('does not include PRINT option when the report is in OPEN state', () => { | ||
| const report = { | ||
| const report = createMock<Report>({ |
There was a problem hiding this comment.
🟢 tests/unit/ReportSecondaryActionUtilsTest.ts:213: createMock<T> is the right call
Good use of createMock<Report>({...}) instead of {...} as unknown as Report. This actually type-checks the partial against Report (so a typo or wrong value type is caught at the call site) and keeps the single widening assertion isolated in the shared helper.
This is the reuse-a-shared-util direction we discussed on the earlier PRs in this series, so it is nice to see it applied here. Nothing to change ✅
| isSettled: boolean; | ||
| }>; | ||
|
|
||
| const setupMocks = (mocks: MockConfig = {}) => { |
There was a problem hiding this comment.
🟢 tests/unit/ReportSecondaryActionUtilsTest.ts:5320-5338: setupMocks unroll looks correct
Unrolling the dynamic jest.spyOn(ReportUtils, method as keyof typeof ReportUtils) loop into explicit per-method spies is the right way to drop the as any and the eslint-disable, since the dynamic-key version could not be typed.
I checked the defaults and they all match the old ones (canEditReportPolicy defaults true, the rest false), and ?? correctly keeps an explicit false from a caller.
No behavior change. Nothing to change here ✅
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - The only actionable item is the 🟢 simplification at line 5388 (drop the redundant per-param Parameters<...> annotations). All 165 tests pass and no cast was hidden behind a helper.
This one is a clean approve once the author trims those annotations, or even as-is if we consider that too minor to hold on.
|
🚀 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
Removed
@typescript-eslint/no-unsafe-type-assertionviolations fromtests/unit/ReportSecondaryActionUtilsTest.tsas part of the cleanup for Expensify/App issue #94739.What changed:
Report,Policy, andTransactionassertions withcreateMock<T>.anycasts with typed mock definitions.jest.mockedfor theisPolicyAdminmocks.Why this approach is safe:
Reviewer focus:
Fixed Issues
$ #94739
PROPOSAL: #94739 (comment)
Count change
Current baseline source:
config/eslint/eslint.seatbelt.tsvBefore:
tests/unit/ReportSecondaryActionUtilsTest.ts: 31 violationsAfter:
tests/unit/ReportSecondaryActionUtilsTest.ts: 0 violationsNet reduction:
@typescript-eslint/no-unsafe-type-assertionviolations.TSV evidence:
Tests
npm run lint -- --show-warnings tests/unit/ReportSecondaryActionUtilsTest.ts
CI=true npm run lint -- --no-cache --show-warnings tests/unit/ReportSecondaryActionUtilsTest.ts
npm run test -- tests/unit/ReportSecondaryActionUtilsTest.ts
The focused Jest suite passed all 165 tests. Fork CI also passed typecheck, lint, formatting, React Compiler compliance, and focused Jest against current
main.Offline tests
Not applicable: this test-only cleanup adds no network behavior.
QA Steps
Not applicable: production behavior and UI are unchanged.
PR Author Checklist
Test-only scope note: all platform, staging, offline, UI, asset, deeplink, shared-component, and high-traffic items below were reviewed and are marked complete as not applicable to this unit-test-only change.
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionDesignlabel and/or tagged@Expensify/designmainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test stepsScreenshots/Videos
Not applicable: only a unit-test source file changed; there is no UI to capture.