[jsweep] Clean add_labels.cjs#35134
Conversation
Add tests for required_labels and required_title_prefix filter features, and add issues.get mock to default setup for these scenarios. - Add mockGithub.rest.issues.get default mock in beforeEach - Add 6 new tests covering required_labels filtering (pass/fail) - Add tests covering required_title_prefix filtering (pass/fail) - Add test for combined required_labels + required_title_prefix - Total: 39 → 45 tests (6 added) 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. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35134 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
This PR improves unit test coverage for the add_labels handler by adding coverage for the required_labels and required_title_prefix filter paths that require github.rest.issues.get.
Changes:
- Added a default
mockGithub.rest.issues.getimplementation in the sharedbeforeEachtest setup. - Added new tests covering required-label and required-title-prefix filtering behavior (skip vs add labels), including a combined filter scenario.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/add_labels.test.cjs | Adds default issues.get mock and new tests for required_labels / required_title_prefix filter behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| it("should skip when item does not have all required_labels", async () => { | ||
| const handler = await main({ max: 10, required_labels: ["needs-triage"] }); | ||
|
|
||
| mockGithub.rest.issues.get = async () => ({ | ||
| data: { title: "Some issue", labels: [{ name: "bug" }] }, |
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (5 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approving with minor suggestions on assertion completeness.
📋 Key Themes & Highlights
Observations
- Payload not asserted: The two "should add labels" tests verify that
addLabelswas called but not what labels were passed. Addingexpect(addLabelsCalls[0].labels).toEqual([...])would make each test a full specification. - Combined-filter happy path missing: The combined
required_labels + required_title_prefixtest only exercises the failure branch; a matching success case would complete the coverage.
Positive Highlights
- ✅ Good breadth: all four skip/pass branches of both filters are individually covered
- ✅
beforeEachcorrectly updated so the defaultgetmock doesn't break existing tests - ✅ Clear, readable test names and well-structured Arrange / Act / Assert flow
- ✅ All 45 tests passing, linting and type-check clean
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M
| addLabelsCalls.push(params); | ||
| return {}; | ||
| }; | ||
|
|
There was a problem hiding this comment.
[/tdd] The assertion only checks that addLabels was called once, not which labels were sent — a bug passing the wrong labels would still pass this test.
💡 Suggested addition
expect(addLabelsCalls[0].labels).toEqual(["enhancement"]);This makes the test a true specification: it verifies both that the action was taken and that the correct payload was used.
| mockGithub.rest.issues.addLabels = async params => { | ||
| addLabelsCalls.push(params); | ||
| return {}; | ||
| }; |
There was a problem hiding this comment.
[/tdd] Same as above — addLabelsCalls.length === 1 confirms invocation but not the payload.
💡 Suggested addition
expect(addLabelsCalls[0].labels).toEqual(["automation"]);| data: { title: "Not ready issue", labels: [{ name: "approved" }] }, | ||
| }); | ||
|
|
||
| const result = await handler({ item_number: 100, labels: ["bug"] }, {}); |
There was a problem hiding this comment.
[/tdd] The combined-filter test only covers the "passes labels, fails prefix" path. Consider adding a case where both conditions are satisfied to confirm the happy path works end-to-end with both filters active.
💡 Sketch
it("should add labels when both required_labels and required_title_prefix match", async () => {
const handler = await main({
max: 10,
required_labels: ["approved"],
required_title_prefix: "[Ready]",
});
const addLabelsCalls = [];
mockGithub.rest.issues.get = async () => ({
data: { title: "[Ready] Deploy v2", labels: [{ name: "approved" }] },
});
mockGithub.rest.issues.addLabels = async params => {
addLabelsCalls.push(params);
return {};
};
const result = await handler({ item_number: 100, labels: ["deploy"] }, {});
expect(result.success).toBe(true);
expect(addLabelsCalls.length).toBe(1);
});
There was a problem hiding this comment.
COMMENT — test-only changes, no production code modified; one non-blocking gap in combined-filter coverage.
### Findings
Non-blocking: The combined required_labels + required_title_prefix test only exercises one failure mode (labels pass, prefix fails). The "both conditions satisfied → labels added" happy path is not covered under the combined config, which means a regression that erroneously skips when both filters are present would not be caught. Suggested fix included in the inline comment.
Already flagged (existing review): PR description claims +6 tests but the diff adds 5.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.5M
| expect(result.success).toBe(false); | ||
| expect(result.skipped).toBe(true); | ||
| expect(result.error).toContain("required prefix"); | ||
| }); |
There was a problem hiding this comment.
Combined-filter test is missing the "both conditions pass" happy path, leaving a gap in AND-semantics coverage.
💡 Details
The only combined-config test covers: required_labels ✅ + required_title_prefix ❌ → skipped.
Two paths are untested under the combined config:
- labels-fail, prefix-pass — a production bug that skips the labels check when a prefix is also configured would go undetected (the standalone
required_labelstest uses a config without a prefix, so it can't catch combined-mode regressions). - both pass — no combined-config test verifies that labels are actually added when both filters are satisfied. A bug that erroneously returns
skipped: truein the combined path would pass all current tests.
Suggested addition:
it("should add labels when both required_labels and required_title_prefix are satisfied", async () => {
const handler = await main({
max: 10,
required_labels: ["approved"],
required_title_prefix: "[Ready]",
});
mockGithub.rest.issues.get = async () => ({
data: { title: "[Ready] Approved issue", labels: [{ name: "approved" }] },
});
const result = await handler({ item_number: 100, labels: ["bug"] }, {});
expect(result.success).toBe(true);
expect(result.skipped).toBeUndefined();
});
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed the thread and pushed a follow-up in |
Summary
Improves test coverage for
actions/setup/js/add_labels.cjsby adding missing test cases identified during review. No production code was changed.What changed
actions/setup/js/add_labels.test.cjs(tests only)required_labelsfiltering behaviourrequired_title_prefixfiltering behaviourrequired_labels+required_title_prefixchecks to cover interaction between the two filtersissues.getstub that returns both title and labels, enabling the new test scenariosWhy
Prior coverage did not exercise the combined filter path — a PR could satisfy one filter but not the other, and that logic was untested. These additions close the gap surfaced in review feedback.
Testing
All changes are in the test file itself. Run the existing JS test suite to verify:
Notes
add_labels.test.cjs)