Repository navigation
refactor(auth): drop the 14 unreachable commenter-auth catch wrappers - #449
Open
mrbobbytables wants to merge 1 commit into
Open
mrbobbytables wants to merge 1 commit into
mrbobbytables wants to merge 1 commit into
Conversation
checkOrgMember, checkCollaborator and checkIssueComments each wrap their
whole body in try/catch and resolve false on any failure, so they never
reject. The fourteen `try { … } catch (e) { throw new Error('could not
check … auth: ' + e) }` wrappers around them — in close, reopen, lock,
milestone, retitle, unassign, assign, cc, uncc, labels/remove and in
auth.ts (getOrgCollabCommentUsers, checkCommenterAuth) — were dead code
kept green only by unit tests that mocked the callee to reject, pinning
error messages the action can never emit (including the misspelt
"couldn ot check commentor Auth").
Call the probes directly, have getOrgCollabCommentUsers reuse
checkCommenterAuth (same three probes in the same order, so identical
API traffic), and delete the fifteen mock-only unit cases. Repack dist.
Fixes cncf#386
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Killen <bkillen@linuxfoundation.org>
Contributor
|
Please add a kind label with |
Contributor
9e11e3a
|
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
checkOrgMember,checkCollaboratorandcheckIssueComments(src/utils/auth.ts) each wrap their whole body intry { … } catch { core.warning(…); return false }, so they never reject. The fourteentry { … } catch (e) { throw new Error('could not check … auth: ' + e) }wrappers around them were therefore unreachable from the shipped bundle and were kept green only by unit tests thatvi.mocked the callee to reject (or madecore.warningthrow) — pinning error messages the action can never emit, including the misspeltcouldn ot check commentor Auth.This PR removes them, as recommended in #386:
close.ts,reopen.ts,lock.ts,milestone.ts,retitle.ts,unassign.ts,labels/remove.ts: call the probe directly (close/reopenbecome a singleisAuthor || collaborator || closePolicyAllowsshort-circuit chain, same evaluation order as before)assign.ts,cc.ts,uncc.ts: drop thecould not get authorized userswrappersauth.ts: drop the wrapper ingetOrgCollabCommentUsersand the three incheckCommenterAuth;getOrgCollabCommentUsersnow reusescheckCommenterAuthper user (same three probes in the same order, so identical API traffic). Also fixes the@param args→userjsdoc mismatch oncheckCommenterAuththatnpm run lintwarned about.__tests__/issueCommentTest/errorPaths.test.ts×8,approveMilestoneErrorPaths.test.ts×1,__tests__/label/errorPaths.test.ts×1,__tests__/utils/auth.test.ts×4logger down) and reword thevi.mocklead-in comments that described the removed branches. The mocks themselves stay — the remaining cases still usemockResolvedValueOnce.dist/index.jsrepacked.No behaviour change: every removed
catchwas dead, and every probe call sequence is unchanged.Fixes #386
Testing
npm run all— build, lint:fix, lint, pack, test: 151 files / 2075 tests pass; lint has 0 errors (1 pre-existing jsdoc warning insrc/issueComment/cc.ts:81, not touched here)npm run test:coverage— 100 % lines / 99.7 % branches / 100 % functions / 100 % statements, so every line in the touched files is still reached by the unit suite without the deleted mock-rejection casesnpm run test:coverage:e2e— 72 files / 452 tests pass;src/e2e lines 98.73 % → 99.28 %; the only remaining 0-hit line in the touched files is the unrelatedauth.ts:24Checklist
npm run allto lint and build my codenpm run packand committeddist/