Skip to content

[jsweep] Clean remove_trigger_label.cjs - #29251

Merged
pelikhan merged 2 commits into
mainfrom
jsweep/remove-trigger-label-68596c098464d9d3
Apr 30, 2026
Merged

[jsweep] Clean remove_trigger_label.cjs#29251
pelikhan merged 2 commits into
mainfrom
jsweep/remove-trigger-label-68596c098464d9d3

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Summary

Cleans remove_trigger_label.cjs and adds a comprehensive test suite.

Context

Execution context: github-script (uses core, github, context globals)

Changes

Code cleanup:

  • Moved require('./error_helpers.cjs') from inside main() to top-level imports — consistent with all other files in the codebase

Tests added (14 tests):
New file remove_trigger_label.test.cjs covers:

  • Missing / invalid GH_AW_LABEL_NAMES env var (3 cases)
  • workflow_dispatch event → skip with empty output
  • No label in payload → skip
  • Label not in configured list → skip, preserve label output
  • issues event: happy path + missing issue number
  • pull_request event: happy path + missing PR number
  • discussion event: happy path via GraphQL + missing node IDs
  • 404 error is non-fatal (label already removed by another run)
  • Non-404 API errors produce a warning (not a failure)

Validation ✅

  • Formatting: npm run format:cjs
  • Linting: npm run lint:cjs
  • Type checking: npm run typecheck
  • Tests: 14/14 passed ✓

Generated by jsweep - JavaScript Unbloater · ● 1.7M ·

  • expires on May 2, 2026, 4:55 AM UTC

- Move require('./error_helpers.cjs') from inside main() to top-level imports
- Add comprehensive test suite (14 tests) covering all code paths:
- Missing/invalid GH_AW_LABEL_NAMES config
- workflow_dispatch skip
- No label in payload
- Label not in configured list
- issues, pull_request, discussion label removal
- 404 non-fatal error handling
- Non-404 API error warnings
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@pelikhan
pelikhan marked this pull request as ready for review April 30, 2026 05:02
CopilotAI review requested due to automatic review settings April 30, 2026 05:02
@github-actionsgithub-actionsBot mentioned this pull request Apr 30, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Cleans up the remove_trigger_label.cjs GitHub Action script and adds a comprehensive Vitest suite to validate behavior across supported events and error conditions.

Changes:

  • Moved getErrorMessage import to top-level in remove_trigger_label.cjs for consistency.
  • Added remove_trigger_label.test.cjs with coverage for config parsing, event-specific behavior, skip paths, and API error handling.
Show a summary per file
FileDescription
actions/setup/js/remove_trigger_label.cjsHoists error_helpers.cjs import to module scope to align with codebase patterns.
actions/setup/js/remove_trigger_label.test.cjsAdds a full test suite exercising label removal behavior across events and error 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: 1

@@ -0,0 +1,220 @@
// @ts-check
import { describe, it, expect, beforeEach, vi } from "vitest";

CopilotAIApr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afterEach is used later in this test file but isn’t imported from vitest. The rest of the test suite consistently imports the hooks it uses (e.g. actions/setup/js/action_conclusion_otlp.test.cjs:2). Please add afterEach to the Vitest import list for consistency and to avoid relying on global hook injection.

Suggested change
import{describe,it,expect,beforeEach,vi}from"vitest";
import{describe,it,expect,beforeEach,afterEach,vi}from"vitest";

Copilot uses AI. Check for mistakes.
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot review all comments

@github-actions

Copy link
Copy Markdown
ContributorAuthor

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

Excellent test quality

MetricValue
New/modified tests analyzed14
✅ Design tests (behavioral contracts)14 (100%)
⚠️ Implementation tests (low value)0 (0%)
Tests with error/edge cases14 (100%)
Duplicate test clusters0
Test inflation detected⚠️ Yes (220 test lines vs 1 production line — new test for existing code)
🚨 Coding-guideline violationsNone

Test Classification Details

View all 14 test classifications
TestFileClassificationNotes
should fail when GH_AW_LABEL_NAMES is not setremove_trigger_label.test.cjs:68✅ DesignError path — config validation
should fail when GH_AW_LABEL_NAMES is invalid JSONremove_trigger_label.test.cjs:74✅ DesignError path — malformed config
should fail when GH_AW_LABEL_NAMES is not an arrayremove_trigger_label.test.cjs:80✅ DesignError path — wrong config type
should skip label removal for workflow_dispatchremove_trigger_label.test.cjs:88✅ DesignBehavioral contract — event gating
should skip removal when payload has no labelremove_trigger_label.test.cjs:97✅ DesignEdge case — missing payload
should skip removal when label is not configuredremove_trigger_label.test.cjs:106✅ DesignBehavioral contract — allow-list check
should remove label from issueremove_trigger_label.test.cjs:118✅ DesignHappy path with parameter assertions
should skip when issue number is missingremove_trigger_label.test.cjs:129✅ DesignEdge case — defensive guard
should remove label from pull requestremove_trigger_label.test.cjs:141✅ DesignBehavioral contract — PR event path
should skip when PR number is missingremove_trigger_label.test.cjs:160✅ DesignEdge case — defensive guard
should remove label from discussion via graphqlremove_trigger_label.test.cjs:172✅ DesignBehavioral contract — GraphQL mutation
should skip when discussion node_id is missingremove_trigger_label.test.cjs:192✅ DesignEdge case — defensive guard
should treat 404 error as already-removed (non-fatal)remove_trigger_label.test.cjs:204✅ DesignError path — idempotency invariant
should warn on non-404 API errorsremove_trigger_label.test.cjs:212✅ DesignError path — non-fatal API failure

Mocking Strategy Assessment

All mocked targets are GitHub Actions runtime globals (global.core, global.github, global.context). These are legitimate external I/O mocks — no internal business-logic functions are mocked. The approach is consistent with the pattern used by other test files in actions/setup/js/.


Test Inflation Note

The raw inflation ratio (220 test lines : 1 production line added) exceeds the 2:1 threshold, triggering the standard 10-point penalty. However, this is context-appropriate: the test file is newly added to cover an already-existing production file (remove_trigger_label.cjs, 138 lines). The single production-file change was a cleanup (-2 lines, +1 line). The test coverage itself is proportional to the production code being tested, not inflated.


Language Support

Tests analyzed:

  • 🟨 JavaScript (*.test.cjs): 14 tests (vitest)

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). All 14 tests verify observable behavioral contracts. The test suite covers all three event types (issues, pull_request, discussion), all configuration error cases, all defensive guards, and the critical 404-idempotency invariant.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References:

🧪 Test quality analysis by Test Quality Sentinel · ● 450.9K ·

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 14 tests verify observable behavioral contracts across all event types and error paths.

CopilotAI commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@copilot review all comments

Added afterEach to the Vitest import list in remove_trigger_label.test.cjs (11cff5b). All 14 tests still pass.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pelikhan