[jsweep] Clean update_pull_request_branches.cjs#36057
Conversation
- Remove redundant continue after pushing to mergeable list - Fix else-block indentation for clarity - Simplify loop to sleep before each iteration (except first) instead of after - Add 8 new tests covering: early return on empty PRs, fatal vs non-fatal error counting, isNonFatalUpdateBranchError directly, non-integer PR number filtering Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36057 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (default_business_additions=0, well under the 100 threshold). The changes consist of modifications to skill query scripts and an MCP server file, none of which require an Architecture Decision Record. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Cleans up the update_pull_request_branches.cjs GitHub Script used in actions/setup to update mergeable PR branches, and expands its Vitest coverage for early-exit and error-classification behavior.
Changes:
- Refactors
filterMergeablePullRequeststo remove a redundantcontinueand tighten skip-reason logging structure. - Moves the per-PR delay to the start of each loop iteration (except the first) for clearer control flow.
- Adds additional unit tests covering early returns, fatal vs non-fatal error accounting, and PR-number filtering.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_pull_request_branches.cjs | Small control-flow cleanups in mergeable filtering and update-loop sleep placement. |
| actions/setup/js/update_pull_request_branches.test.cjs | Adds coverage for early exits, error classification/accounting, and input filtering. |
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: 2
| it("counts fatal errors separately from non-fatal errors", async () => { | ||
| mockGithub.paginate.mockResolvedValue([{ number: 10 }, { number: 11 }]); | ||
| mockGithub.rest.pulls.get.mockResolvedValue({ | ||
| data: { state: "open", mergeable: true, draft: false, head: { repo: { full_name: "owner/repo" } } }, | ||
| }); | ||
| const fatalErr = new Error("Something unexpected"); | ||
| const nonFatalErr = Object.assign(new Error("update branch failed"), { status: 422 }); | ||
| mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(fatalErr).mockRejectedValueOnce(nonFatalErr); | ||
|
|
||
| await expect(moduleUnderTest.main()).resolves.not.toThrow(); | ||
| expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Failed to update branch for PR #10")); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Skipping PR #11")); | ||
| expect(mockCore.notice).toHaveBeenCalledWith(expect.stringContaining("updated=0, skipped=1, failed=1")); | ||
| }); |
| it("filters out non-integer pull request numbers", async () => { | ||
| mockGithub.paginate.mockResolvedValue([{ number: 1 }, { number: "bad" }, { number: null }, { number: 2 }]); | ||
| mockGithub.rest.pulls.get.mockResolvedValue({ | ||
| data: { state: "open", mergeable: true, draft: false, head: { repo: { full_name: "owner/repo" } } }, | ||
| }); | ||
| mockGithub.rest.pulls.updateBranch.mockResolvedValue({ data: {} }); | ||
|
|
||
| await moduleUnderTest.main(); | ||
|
|
||
| expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledTimes(2); | ||
| expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledWith(expect.objectContaining({ pull_number: 1 })); | ||
| expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledWith(expect.objectContaining({ pull_number: 2 })); | ||
| }); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are correct and behavior-equivalent. No blocking issues found.
Review notes
- Sleep refactor (
i > 0guard before vsi < length - 1guard after): semantically identical for all normal and exceptional loop exits — no behavior change. continue→if/else: purely structural; logic is identical.- New tests: the 8 added tests cover previously untested paths. The "counts fatal vs non-fatal errors" test uses substring matching on the notice output, but the adjacent
mockCore.error/mockCore.warningassertions already verify the correct error-type-to-log-level mapping, making the counter assertion sufficient.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.1M
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (13 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — approving with one minor observation on test timer hygiene.
📋 Key Themes & Highlights
Key Themes
- Sleep refactor is correct: The
i < length-1after-sleep toi > 0before-sleep inversion is semantically equivalent in all cases including a single-element list. ✅ continue→if/elseis clean: The restructuring improves readability without changing behaviour. ✅- Test expansion is thorough: +8 tests covering early exits, fatal vs non-fatal error discrimination,
isNonFatalUpdateBranchErrorunit tests, and non-integer PR filtering are all well-scoped.
Minor Observation
- Tests that exercise 2 PRs (
counts fatal errors separately,filters out non-integer pull request numbers) will trigger one realsleep(UPDATE_DELAY_MS)call. If the delay is non-trivial, consider Jest fake timers to keep the suite fast. (Non-blocking.)
Positive Highlights
- ✅ Semantic equivalence of the sleep refactor is easy to verify
- ✅
isNonFatalUpdateBranchErrorunit tests are clearly named and cover all branches - ✅ Formatting/lint/typecheck validation confirmed in PR description
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
|
|
||
| await expect(moduleUnderTest.main()).resolves.not.toThrow(); | ||
| expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Failed to update branch for PR #10")); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Skipping PR #11")); |
There was a problem hiding this comment.
[/tdd] The counts fatal errors separately and filters out non-integer pull request numbers tests each iterate over 2 PRs, triggering one real sleep(UPDATE_DELAY_MS) call between iterations. If UPDATE_DELAY_MS is non-trivial, these tests will be slow.
💡 Suggestion: mock the sleep delay
Consider using Jest fake timers to keep tests fast and deterministic:
beforeEach(() => { jest.useFakeTimers(); });
afterEach(() => { jest.useRealTimers(); });This ensures that if UPDATE_DELAY_MS is ever tuned upward, the test suite does not silently slow down.
|
@copilot review all comments an reviews from copilot and GitHub-actions actor |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I addressed the actionable review feedback about test delays by switching the affected |
Summary
Refactors
update_pull_request_branches.cjsfor correctness and readability, and significantly expands its test coverage with fake timers and new edge-case scenarios. Also removes a minor outdated recommendation from the agentic-workflows agent guide.Changes by file
actions/setup/js/update_pull_request_branches.cjs(modified — medium impact)continue-based control flow toif/elsebranches, improving readability and making early-exit paths explicit.actions/setup/js/update_pull_request_branches.test.cjs(modified — medium impact)isNonFatalUpdateBranchErrorclassification — unit tests for the error-classification helper directly..github/agents/agentic-workflows.md(modified — low impact)emoji:frontmatter value when creating workflows. No functional change.Motivation
The sleep-at-end pattern meant the final loop iteration always incurred a pointless delay, and the
continue-based skip logic obscured which conditions caused early exits. The refactor aligns the code with its intended semantics. The expanded test suite locks in this behaviour and adds coverage for previously untested classification and filtering logic.Risk
Low. The JS refactor is a logic-preserving restructure with no interface changes. New tests are additive. The agent-guide edit is documentation-only.