[No QA] Reduce unsafe type assertions in TSCompilerUtils tests#95269
Conversation
|
@jayeshmangwani 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] |
2fae008 to
298d03f
Compare
| "../../tests/unit/SubscriptionUtilsTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 2 | ||
| "../../tests/unit/SuggestedFollowupTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 1 | ||
| "../../tests/unit/SuggestionMentionTest.tsx" "@typescript-eslint/no-unsafe-type-assertion" 4 | ||
| "../../tests/unit/TSCompilerUtilsTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 96 |
There was a problem hiding this comment.
It is not necessary to manually tighten the seatbelt. Entries will be removed from this file on merge. I recommend that you avoid doing it manually before merge to avoid merge conflicts.
The only time you should need to touch this file is when you're adding new exceptions (or renaming a file which contained existing exceptions).
298d03f to
914e66f
Compare
|
@blimpich, tagging you here since you're assigned to the issue. I'm not sure whether this PR needs a C+ review, since its No QA and only test update. |
|
@jayeshmangwani Yes, I have tagged @blimpich on the issue thread. There will likely be no need for a C+ review now. As there will be serise of PRs under #94739 and we might not need a C+ review now but may be on later stage PRs where Prod files will be modfied for cleanup. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.29-0 🚀
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.31-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.31-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Reduced
@typescript-eslint/no-unsafe-type-assertionviolations intests/unit/TSCompilerUtilsTest.tsas part of the cleanup for Expensify/Appissue #94739.
What changed:
ts.isExpressionStatement,ts.isPropertyAssignment, andts.isObjectLiteralExpression.main.Why this approach is safe:
Reviewer focus:
tests/unit/TSCompilerUtilsTest.tsand confirm the helper checksare real AST narrowing rather than hidden unsafe casts.
TSCompilerUtilsTest.tstarget-rule row.Safety checks:
SEATBELT_INCREASE.eslint-disablefor@typescript-eslint/no-unsafe-type-assertion.as unknown assubstitutions.Fixed Issues
$ #94739
PROPOSAL:
Count change
Current baseline source:
config/eslint/eslint.seatbelt.tsvBefore:
tests/unit/TSCompilerUtilsTest.ts: 96 violationsAfter:
tests/unit/TSCompilerUtilsTest.ts: 0 violationsNet reduction:
@typescript-eslint/no-unsafe-type-assertionviolations.TSV evidence:
tests/unit/TSCompilerUtilsTest.tsrow fromconfig/eslint/eslint.seatbelt.tsv.CI=true npm run lint -- --no-cache --show-warnings tests/unit/TSCompilerUtilsTest.tsconfig/eslint/eslint.seatbelt.tsvdiff is intentionally notincluded in this PR because Expensify/App post-merge OSBotify automation
tightens the baseline on
main.Tests
Run:
Verify lint passes with no target-rule warnings for the edited file.
Run:
Verify
config/eslint/eslint.seatbelt.tsvlocally contains the decrement describedabove, then restore it before committing:
Run:
npm run test -- tests/unit/TSCompilerUtilsTest.tsVerify the targeted test suite passes.
Run:
Verify TypeScript checks pass.
Offline tests
Not applicable. This is test-only cleanup and does not change network behavior.
QA Steps
No QA required. This PR only changes tests. The ESLint seatbelt baseline is tightened by Expensify/App post-merge OSBotify automation.
PR Author Checklist
For checklist items that are not applicable to this test-only cleanup, checked
means the item was considered and determined not applicable.
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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. This is test-only cleanup.
Android: mWeb Chrome
Not applicable. This is test-only cleanup.
iOS: Native
Not applicable. This is test-only cleanup.
iOS: mWeb Safari
Not applicable. This is test-only cleanup.
MacOS: Chrome / Safari
Not applicable. This is test-only cleanup.
cc @blimpich