Repository navigation
[No QA] Remove unsafe type assertions from BulkEditTest.ts tests - #96671
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
|
| }; | ||
| } | ||
|
|
||
| function getBulkEditUpdates(writeSpy: APIWriteSpy, callIndex = 0): Record<PropertyKey, unknown> | undefined { |
There was a problem hiding this comment.
🟡 tests/actions/IOUTest/BulkEditTest.ts:53-61: getBulkEditUpdates returns undefined where the old code would throw
I would make this throw instead of returning undefined when the write call or params are missing. The old code did JSON.parse(params.updates) on an asserted value, so if the write never happened the test failed loudly.
Now the tests use updates?.taxAmount and expect(...).toBeUndefined(), so if getBulkEditUpdates returns undefined the assertion passes even though nothing was written. Today it is saved by the preceding expect(updates?.category).toBe('Food') (and similar) in each of these tests, so it is not broken, but it is fragile: the first test that only checks toBeUndefined will pass vacuously.
Can we return the narrowed object or throw with a clear message when the call or params are absent, same idea as getRequiredWriteCall in the ReportWorkflowTest.ts PR ? That would keep the fail-fast behavior we had before.
| function isOnyxData(value: unknown): value is OnyxData<OnyxKey> { | ||
| return typeof value === 'object' && value !== null && 'optimisticData' in value; | ||
| type BulkEditWriteParams = {updates: string}; | ||
| type OptimisticDataEntry = {key: string; onyxMethod: string; value?: unknown}; |
There was a problem hiding this comment.
🟡 tests/actions/IOUTest/BulkEditTest.ts:28: OptimisticDataEntry duplicates the production OnyxUpdate type
OptimisticDataEntry = {key: string; onyxMethod: string; value?: unknown} is a looser copy of OnyxUpdate which is already exported from react-native-onyx. This is the same point @blimpich raised on the other PRs in this series: we should not mint generic Onyx types inside a specific test file.
Can we type the entry from OnyxUpdate (or a Pick of it) and keep the runtime guard, instead of a new local shape ? The JSON.parse boundary still needs a guard at runtime, that part is fine.
| return isObject(value) && Array.isArray(value.optimisticData); | ||
| } | ||
|
|
||
| function getBulkEditWriteData(writeSpy: APIWriteSpy, callIndex = 0): BulkEditWriteData { |
There was a problem hiding this comment.
🟡 tests/actions/IOUTest/BulkEditTest.ts:38-51: the write-spy narrowing helpers should be shared, not per-file
getBulkEditWriteData, getOptimisticDataFromWriteSpy and isOptimisticDataEntry do the same job as the helpers added in ReportWorkflowTest.ts (getRequiredWriteCall) and PolicyTest.ts (requireRecordArrayProperty).
Three files in this series now each have their own "narrow API.write optimistic data" helper. Rather than adding a third, can we land one shared helper in tests/utils/typeGuards.ts and reuse it here ? This is worth settling once for the whole #94739 series so we stop duplicating it.
| delete (transaction as Partial<Transaction>).transactionThreadReportID; | ||
| delete transaction.transactionThreadReportID; |
There was a problem hiding this comment.
🟢 tests/actions/IOUTest/BulkEditTest.ts:1631: clean cast removal, confirmed
delete transaction.transactionThreadReportID; without the old as Partial<Transaction> is correct, transactionThreadReportID is optional on Transaction (src/types/onyx/Transaction.ts:592), so delete is valid here without the cast. Nothing to change, just noting I checked it ✅
| const draftKey = `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_BULK_EDIT_TRANSACTION_ID}` as OnyxKey; | ||
| const draftKey: `${typeof ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${string}` = `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_BULK_EDIT_TRANSACTION_ID}`; |
There was a problem hiding this comment.
🟢 tests/actions/IOUTest/BulkEditTest.ts:2284 and the removed eslint-disable blocks — nice
Replacing as OnyxKey with the explicit `${typeof ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${string}` annotation is the right way to do it.
And removing the two /* eslint-disable @typescript-eslint/no-explicit-any ... */ blocks with the as any casts is exactly the point of this issue. This is the strongest part of the PR ✅
There was a problem hiding this comment.
@ikevin127 I don't think this comment adds any value. Can you please keep your inline comments to things that you think should be changed, rather than just noting when something is done correctly? Or explain to me what the value of these comments are?
There was a problem hiding this comment.
Sure, I'll stop adding comments that don't require changes ✅
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
(3) 🟡 medium severity comments to be addressed before moving forward.
|
Resolved all three review findings in 4bf034e, with the Knip follow-up in 01ceba3:
Please re-review when convenient. Merge note: #96671 is independently mergeable and does not need to wait for #96513 or #96519. If either helper PR lands first, I will update this branch from |
|
@ikevin127 Friendly reminder: the requested changes and merge conflict have been addressed, and CI is green. Could you please re-review when you have a chance? Thanks! |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments, I verified BulkEditTest.ts (3 findings, all 🟡) → all addressed ✅
blimpich
left a comment
blimpich
left a comment
There was a problem hiding this comment.
Mostly there, one minor comment
|
|
||
| function isOnyxData(value: unknown): value is OnyxData<OnyxKey> { | ||
| return typeof value === 'object' && value !== null && 'optimisticData' in value; | ||
| type APIWriteSpy = jest.SpiedFunction<typeof API.write>; |
There was a problem hiding this comment.
Generic type should not be in specific file
There was a problem hiding this comment.
Thanks! I removed the local APIWriteSpy alias and inlined jest.SpiedFunction<typeof API.write> in the two helper signatures, avoiding a new shared or production abstraction.
@blimpich could you please re-review when you have a chance?
| const draftKey = `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_BULK_EDIT_TRANSACTION_ID}` as OnyxKey; | ||
| const draftKey: `${typeof ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${string}` = `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_BULK_EDIT_TRANSACTION_ID}`; |
There was a problem hiding this comment.
@ikevin127 I don't think this comment adds any value. Can you please keep your inline comments to things that you think should be changed, rather than just noting when something is done correctly? Or explain to me what the value of these comments are?
|
🚀 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/actions/IOUTest/BulkEditTest.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/actions/IOUTest/BulkEditTest.ts: 25 violationsAfter:
tests/actions/IOUTest/BulkEditTest.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: The focused Jest suite passed all 33 existing scenarios, with focused lint, formatting, TSV restoration, and diff hygiene also passing.
Offline tests
Not applicable: this test-only type-safety cleanup adds no network behavior.
QA Steps
Not applicable: production behavior and UI are unchanged.
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: only a unit-test source file changed.
Not applicable: there are no user-interface changes.
Android: mWeb Chrome
Not applicable: only a unit-test source file changed.
Not applicable: there are no user-interface changes.
iOS: Native
Not applicable: only a unit-test source file changed.
Not applicable: there are no user-interface changes.
iOS: mWeb Safari
Not applicable: only a unit-test source file changed.
Not applicable: there are no user-interface changes.
MacOS: Chrome / Safari
Not applicable: only a unit-test source file changed.
Not applicable: there are no user-interface changes.