[jsweep] Clean update_pull_request_branches.cjs#40246
Conversation
- Replace indexed for-loop with for...of entries() in main() - Add 4 new test cases (17 total, up from 13): - Skips closed pull requests - Skips PRs with mergeable: null (pending computation) - Does not treat non-object errors as non-fatal - Does not treat 404 status errors as non-fatal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40246 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). Neither enforcement condition is met. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR performs a small cleanup pass on update_pull_request_branches.cjs (a github-script entrypoint) and expands the unit test suite to cover additional edge cases around mergeability filtering and non-fatal error classification.
Changes:
- Refactors the update loop in
main()to usefor...of mergeablePullRequests.entries()instead of an indexedforloop. - Adds 4 test cases covering closed PRs,
mergeable: null, non-object errors, and non-422 status errors.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_pull_request_branches.cjs | Refactors the PR update iteration loop for clarity while preserving behavior. |
| actions/setup/js/update_pull_request_branches.test.cjs | Adds new test coverage for mergeability filtering and non-fatal error detection edge cases. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
There was a problem hiding this comment.
No blocking issues found. ✅
The for...of entries() refactor is semantically identical to the indexed loop — correct async sequencing, correct sleep guard, correct types. The four new tests each cover a real edge case previously untested (closed state, mergeable: null, non-object throws, non-422 status codes); all assertions are well-targeted.
Review notes
- Loop refactor:
entries()produces[index, value]tuples;istill gates the sleep — behavior unchanged. isNonFatalUpdateBranchError: thetypeof === 'object' && error !== nullguard correctly prevents'status' in errorfrom throwing on primitives before falling through to the message check.mergeable=nulltest:String(null) === 'null'matches the log template — assertion is accurate.
🔎 Code quality review by PR Code Quality Reviewer · 62.7 AIC · ⌖ 6.8 AIC · ⊞ 5.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approving with one minor test-style note.
📋 Key Themes & Highlights
Key Themes
- Multi-assertion test:
isNonFatalUpdateBranchErrorprimitive-input test bundles 3 input types in oneit(). Considerit.eachfor better failure isolation (non-blocking).
Positive Highlights
- ✅
for...of entries()idiom swap is behaviorally equivalent and cleaner — the indexistill correctly controls the sleep delay - ✅ New tests fill genuine untested paths: closed-PR filtering,
mergeable: nullpending state, primitive/null inputs toisNonFatalUpdateBranchError, and non-whitelisted 404 status codes - ✅ Arrange/Act/Assert structure is consistent across all four new tests
- ✅ All 17 tests pass with format, lint, and typecheck gates green before submission
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 48.1 AIC · ⌖ 7.11 AIC · ⊞ 6.9K
| expect(moduleUnderTest.isNonFatalUpdateBranchError("some string error")).toBe(false); | ||
| expect(moduleUnderTest.isNonFatalUpdateBranchError(null)).toBe(false); | ||
| expect(moduleUnderTest.isNonFatalUpdateBranchError(42)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Three distinct primitive inputs (string, null, number) are asserted in a single it() block — if the first assertion fails, the other two inputs go untested in that run.
💡 Consider it.each for better failure isolation
it.each([
["some string error"],
[null],
[42],
])("does not treat %s as a non-fatal error", (input) => {
expect(moduleUnderTest.isNonFatalUpdateBranchError(input)).toBe(false);
});This is a minor style note — the current approach still validates the behaviour correctly, and all cases fail fast on the same function.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Go: 0 ( Inflation note: The production change is a minor loop refactor (+1 line), while 4 new edge-case tests add 34 lines — intentional test-gap filling rather than speculative inflation. Mocking: Verdict
References:
|
Summary
Clean-up pass on the
update_pull_request_branchesaction script: modernises the PR-iteration loop and expands test coverage for previously untested edge cases.Changes
actions/setup/js/update_pull_request_branches.cjs— refactor (low impact, non-breaking)forloop with a destructuredfor...ofusing.entries().pullNumbervariable; the PR number is now bound directly in the loop header.actions/setup/js/update_pull_request_branches.test.cjs— tests added (low impact, non-breaking)Four new test cases added:
mergeable: null(GitHub "pending" state)Risk Assessment
Commit Log
f3475869eb216daa22