Repository navigation
[No QA] Remove unsafe assertions from DeployChecklistUtilsTest.ts - #96782
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
|
| mockFetchAllPullRequests.mockImplementation(async (pullRequestNumbers, repo = CONST.APP_REPO) => { | ||
| const pullRequests = mockPullRequestsByRepo[repo] ?? []; | ||
| return pullRequests.filter(({number}) => pullRequestNumbers.includes(number)); | ||
| }); | ||
| mockGetPullRequestMergerLogin.mockImplementation(async (pullRequestNumber) => { | ||
| const pullRequest = mockPRs.find(({number, labels}) => number === pullRequestNumber && labels.some(({name}) => name === CONST.LABELS.INTERNAL_QA)); | ||
| return pullRequest ? 'octocat' : undefined; | ||
| }); |
There was a problem hiding this comment.
🟠 tests/unit/DeployChecklistUtilsTest.ts: mocking fetchAllPullRequests and getPullRequestMergerLogin removes their only coverage
This block changed from mocking the Octokit HTTP layer (pulls.list, pulls.get, paginate) to spying on GithubUtils.fetchAllPullRequests and GithubUtils.getPullRequestMergerLogin directly. That means the real implementations of those two methods no longer run in this test.
I grepped tests/ and these two methods are referenced only in this file, so after this change they are not exercised by any test at all. For example mockGetPullRequestMergerLogin now hard-codes 'octocat', where before the real method extracted merged_by.login from the pulls.get response, so if that extraction logic broke, no test would catch it now. These run in GitHub Actions during deploy, so I would not want to lose that silently.
Can you confirm this boundary change is intentional, and either keep exercising them through the Octokit mock here, or add focused coverage for fetchAllPullRequests and getPullRequestMergerLogin so we do not ship them untested ?
This is beyond a type-assertion cleanup, so worth calling out explicitly.
There was a problem hiding this comment.
Fixed in 40d9d3d.
I removed the direct mocks of fetchAllPullRequests and getPullRequestMergerLogin, so their production implementations now run in this suite.
The tests mock only the lower-level Octokit paginate and pulls.get dependencies, restoring coverage for PR filtering, repository selection, and merger-login extraction.
| const noBodyIssue = {...baseIssue, body: ''}; | ||
|
|
||
| mockListIssues.mockResolvedValue(createListForRepoResponse([noBodyIssue])); | ||
| return getDeployChecklist().then((data) => | ||
| expect(data).toMatchObject({ | ||
| PRList: [], | ||
| PRListMobileExpensify: [], | ||
| deployBlockers: [], | ||
| internalQAPRList: [], | ||
| isSentryChecked: false, | ||
| isGHStatusChecked: false, | ||
| version: '', | ||
| tag: '-staging', | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🟡 tests/unit/DeployChecklistUtilsTest.ts: the "no body" test now asserts the opposite behavior, please confirm intended
Good catch that the old version was broken: const noBodyIssue = baseIssue; noBodyIssue.body = ''; mutated the shared baseIssue fixture, and the .catch((e) => expect(...)) never ran because getDeployChecklist() actually resolves for an empty body, so the assertion was vacuous.
The new {...baseIssue, body: ''} plus .then(...toMatchObject) fixes both, and it matches production: getDeployChecklistData (.github/libs/DeployChecklistUtils.ts:214-229) returns version: '' and tag: '-staging' for an empty body, it only throws "Unable to find ... correct data" if the try block itself throws (for example a malformed url in getIssueOrPullRequestNumberFromURL).
Two things: first, this flips the assertion from "empty body rejects" to "empty body resolves", which is a behavior change beyond type-safety, so flag it for @blimpich.
Second, the real "correct data" throw path is now uncovered, the old test pretended to cover it.
Consider adding a case that actually triggers the catch (an issue with a malformed url) so that error branch stays tested.
There was a problem hiding this comment.
Fixed in 40d9d3d.
I kept the corrected empty-body success behavior and added a separate malformed-URL test that genuinely exercises the getDeployChecklistData catch branch and asserts the expected “Unable to find…” error.
| type Octokit = typeof GithubUtils.octokit; | ||
| type OctokitListForRepo = Octokit['issues']['listForRepo']; | ||
| type ListForRepoResponse = Awaited<ReturnType<OctokitListForRepo>>; | ||
| type OctokitIssue = ListForRepoResponse['data'][number]; | ||
| type PullRequest = Exclude<Awaited<ReturnType<typeof GithubUtils.fetchAllPullRequests>>, void>[number]; |
There was a problem hiding this comment.
🟢 tests/unit/DeployChecklistUtilsTest.ts:20-24 and :30-40: deriving from production types and the Octokit spy setup are strong
This is the right direction. Deriving OctokitIssue, ListForRepoResponse, and PullRequest from GithubUtils.octokit and GithubUtils.fetchAllPullRequests instead of the hand-rolled Label / Issue types removes the as unknown as InternalOctokit and asMutable / Writable casts, and reuses the library types like we discussed on earlier PRs.
Initializing a real Octokit with initOctokitWithToken and spying on listForRepo, rather than assigning a fake object, is cleaner and drops the cast. Wrapping the fake timers in try/finally so useRealTimers() always runs is a real isolation fix too.
Nice work on this part, no changes needed here ✅
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
- The 🟠 comment is the real concern. It swaps out
fetchAllPullRequestsandgetPullRequestMergerLoginfor stubs, and those methods have no other test, so this quietly drops deploy-tooling coverage. That is exactly the kind of "made the types easier by testing less" move worth pushing back on. - The 🟡 comment is a correct fix but it silently flips a test from expecting a rejection to expecting a resolve, and leaves the actual error path uncovered.
Both are defensible, but they are scope creep on a "remove type assertions" PR, so I would ask the author to confirm intent and restore the lost coverage before approving.
|
@ikevin127 Friendly reminder: the requested changes 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 DeployChecklistUtilsTest.ts (🟠 + 🟡) → both addressed ✅
| type Octokit = typeof GithubUtils.octokit; | ||
| type OctokitListForRepo = Octokit['issues']['listForRepo']; | ||
| type ListForRepoResponse = Awaited<ReturnType<OctokitListForRepo>>; | ||
| type OctokitIssue = ListForRepoResponse['data'][number]; | ||
| type PullRequest = Exclude<Awaited<ReturnType<typeof GithubUtils.fetchAllPullRequests>>, void>[number]; |
There was a problem hiding this comment.
You can import types from the github module. Example:
export type {OctokitIssueItem, ListForRepoMethod, InternalOctokit, CreateCommentResponse, ListCommentsResponse, CommitType};
We shouldn't be recreating these if we can just import them.
There was a problem hiding this comment.
Thanks! I removed the recreated test aliases and now import the existing InternalOctokit, ListForRepoMethod, and OctokitIssueItem types from GithubUtils.
@blimpich could you please re-review when you have a chance?
|
🚀 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/DeployChecklistUtilsTest.tsas part of the cleanup for Expensify/App issue #94739.What changed:
GithubUtils.octokitto avoid introducing a direct unlisted package dependency, and added a scoped spelling exception for the GitHub API fieldrebaseable.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/DeployChecklistUtilsTest.ts: 24 violationsAfter:
tests/unit/DeployChecklistUtilsTest.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 25 scenarios and 46 assertions, with focused lint, Knip delta, spellcheck, formatting, TypeScript diagnostics, randomized Jest.
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.