[jsweep] Clean update_handler_factory.cjs#39019
Conversation
- Split long destructuring onto multiple lines for readability - Replace forEach configParts building with spread + filter + map - Break long withRetry call and ternary metadata captures across multiple lines - Add 5 new tests: staged mode, buildUpdateData skipped, deferred resolution, itemFilter skip, itemFilter proceed (34 tests total, up from 29) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39019 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). Neither enforcement condition is met. |
There was a problem hiding this comment.
Pull request overview
This PR performs a small readability refactor in the shared JavaScript update handler factory and extends its Vitest suite with additional coverage for previously untested control-flow branches.
Changes:
- Refactors
configPartsconstruction increateUpdateHandlerFactoryfrom an imperativeforEach + pushto a declarativefilter + mapspread. - Adds new tests for staged mode,
buildUpdateData(...).skipped, deferred number resolution, anditemFilterearly-return behavior.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_handler_factory.cjs | Refactors configuration logging assembly (configParts) for readability. |
| actions/setup/js/update_handler_factory.test.cjs | Adds tests covering staged/skipped/deferred/itemFilter branches. |
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("should return staged result when config.staged is true", async () => { | ||
| const mockExecuteUpdate = vi.fn(); | ||
|
|
||
| const handlerFactory = factoryModule.createUpdateHandlerFactory({ | ||
| itemType: "update_test", |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 84/100 — Excellent
📊 Metrics & Test Classification (5 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
i️ Test Inflation Context (not flagged as a concern)The metric shows a ~18:1 ratio (127 test lines added vs. 7 production lines added), which mechanically triggers the inflation penalty (−10 pts). However, the production change is a small The 10-point deduction reflects the mechanical metric, not an actual quality concern. 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.
|
|
@copilot run pr-finisher skill |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Non-blocking observations on the 5 new tests and 1 source change. No correctness bugs found — the forEach → filter+map+spread refactor is functionally equivalent. Four test-quality gaps noted inline.
### Summary of findings
Source change (update_handler_factory.cjs):
The compact functional rewrite is clean and correct. One observation: the refactor makes it less obvious that additionalConfig values are ignored (only its keys are whitelisted against config). See inline comment.
New tests (update_handler_factory.test.cjs):
All five new tests cover real production paths that lacked coverage (staged, deferred, skipped, filter-skip, filter-proceed). The structural coverage is good; the assertion depth has gaps:
- Staged test:
previewInfo.updateFieldsandhasRawBodynot asserted - Filter short-circuit:
buildUpdateDatanot verified as skipped - Both filter tests:
itemFiltercall arguments not verified — a wrong-repo or wrong-number bug would be invisible
🔎 Code quality review by PR Code Quality Reviewer · 555.7 AIC · ⌖ 13.2 AIC · ⊞ 16.7K
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.staged).toBe(true); | ||
| expect(result.previewInfo).toMatchObject({ number: 42 }); |
There was a problem hiding this comment.
Staged-mode preview assertions are incomplete — future regressions in what the staged payload captures will not be detected.
💡 Suggested additions
The staged path returns previewInfo: { number, updateFields, hasRawBody }. This test only validates number. A refactor that accidentally emptied updateFields or miscaptured hasRawBody would silently pass.
Consider adding:
expect(result.previewInfo.updateFields).toEqual(["title"]);
expect(result.previewInfo.hasRawBody).toBe(false);
// optionally: formatSuccessResult not called in staged mode
expect(handlerConfig.formatSuccessResult).not.toHaveBeenCalled();The updateFields list is the core of what staged mode is communicating — it deserves explicit coverage.
| const handler = await handlerFactory({}); | ||
| const result = await handler({ title: "Test" }); | ||
|
|
||
| expect(mockItemFilter).toHaveBeenCalled(); |
There was a problem hiding this comment.
itemFilter short-circuit does not verify buildUpdateData is skipped — a regression leaving buildUpdateData called after filter rejection would be invisible.
When itemFilter returns a truthy result the handler returns immediately (before buildUpdateData is reached). The test should assert expect(mockBuildUpdateData).not.toHaveBeenCalled() to lock that control-flow order down. Without it, a future accidental reorder won't fail the test.
| const handler = await handlerFactory({}); | ||
| const result = await handler({ title: "Test" }); | ||
|
|
||
| expect(mockItemFilter).toHaveBeenCalled(); |
There was a problem hiding this comment.
Both itemFilter tests verify the filter was called but not what it received — a bug passing the wrong repo, item number, or config to the filter would still pass these tests.
Production code calls itemFilter(githubClient, repoResult.repoParts, itemNumber, config). Consider asserting the arguments explicitly:
expect(mockItemFilter).toHaveBeenCalledWith(
expect.anything(), // githubClient
{ owner: 'testowner', repo: 'testrepo' },
42, // itemNumber
expect.objectContaining({}) // config
);Without this, a cross-repo routing bug that passes the wrong repoParts or a wrong itemNumber to the filter would be completely invisible here.
| const configParts = [ | ||
| `max=${maxCount}`, | ||
| `target=${updateTarget}`, | ||
| ...Object.entries(additionalConfig) |
There was a problem hiding this comment.
additionalConfig values are silently ignored — only its keys are used as a whitelist — the refactored form makes this harder to notice.
Both old and new code read config[key] (runtime value) for the log entry, not additionalConfig[key] (schema value). The old code made this slightly visible via the unused value destructure variable; the new compact form hides it entirely.
If a future contributor adds a value to additionalConfig expecting it to appear in the log (e.g., as a default or label), it will be silently ignored. Consider a short comment to document intent:
// additionalConfig keys act as a whitelist; values are logged from the runtime config
...Object.entries(additionalConfig)
.filter(([key]) => config[key] !== undefined)
.map(([key]) => `${key}=${config[key]}`),
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Summary
Cleans up
update_handler_factory.cjsand expands its test coverage with a dedicated control-flow describe block.Changes
actions/setup/js/update_handler_factory.cjsforEachloop with a declarative array spread usingfilter/mapoverObject.entries(additionalConfig).actions/setup/js/update_handler_factory.test.cjshandler control flowdescribe block containing five test cases:resolveItemNumberitemFilterblocking executionitemFilterpassing through toexecuteUpdateImpact Assessment
update_handler_factory.cjsupdate_handler_factory.test.cjsNotes
forEach→spread refactor is purely stylistic/declarative.