[jsweep] Clean check_skip_if_match.cjs#22596
Conversation
- Destructure process.env at the top (matching check_skip_if_no_match.cjs style) - Destructure API response directly in the await expression - Reformat minified test file into readable idiomatic style - Add missing edge case test: negative GH_AW_SKIP_MAX_MATCHES (-1) - 15 → 16 test cases total Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Cleans up check_skip_if_match.cjs and its tests to match the structure/style used by the sibling “no match” script, and expands test coverage for max-match parsing edge cases.
Changes:
- Refactors
check_skip_if_match.cjsto destructureprocess.envup-front and destructure the GitHub API response inline. - Reformats
check_skip_if_match.test.cjsinto readable, idiomatic test code. - Adds a new test case validating negative
GH_AW_SKIP_MAX_MATCHESis rejected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| actions/setup/js/check_skip_if_match.cjs | Refactors env/API response reads to match patterns used in sibling implementation. |
| actions/setup/js/check_skip_if_match.test.cjs | Rewrites tests for readability and adds coverage for negative max-matches parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process.env.GH_AW_WORKFLOW_NAME = "test-workflow"; | ||
|
|
||
| await eval(`(async () => { ${checkSkipIfMatchScript}; await main(); })()`); |
There was a problem hiding this comment.
These tests execute the script via fs.readFileSync + eval, which is brittle and inconsistent with most sibling action tests that require("./<script>.cjs") and call main() directly (e.g. actions/setup/js/check_skip_if_no_match.test.cjs:3). Since check_skip_if_match.cjs exports main, consider switching to require/import + calling main() instead of eval to reduce side effects and simplify the test setup.
| originalEnv = { | ||
| GH_AW_SKIP_QUERY: process.env.GH_AW_SKIP_QUERY, | ||
| GH_AW_WORKFLOW_NAME: process.env.GH_AW_WORKFLOW_NAME, | ||
| GH_AW_SKIP_MAX_MATCHES: process.env.GH_AW_SKIP_MAX_MATCHES, | ||
| }; | ||
| const scriptPath = path.join(process.cwd(), "check_skip_if_match.cjs"); | ||
| checkSkipIfMatchScript = fs.readFileSync(scriptPath, "utf8"); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (originalEnv.GH_AW_SKIP_QUERY !== undefined) { | ||
| process.env.GH_AW_SKIP_QUERY = originalEnv.GH_AW_SKIP_QUERY; | ||
| } else { | ||
| delete process.env.GH_AW_SKIP_QUERY; | ||
| } | ||
| if (originalEnv.GH_AW_WORKFLOW_NAME !== undefined) { | ||
| process.env.GH_AW_WORKFLOW_NAME = originalEnv.GH_AW_WORKFLOW_NAME; | ||
| } else { | ||
| delete process.env.GH_AW_WORKFLOW_NAME; | ||
| } | ||
| if (originalEnv.GH_AW_SKIP_MAX_MATCHES !== undefined) { | ||
| process.env.GH_AW_SKIP_MAX_MATCHES = originalEnv.GH_AW_SKIP_MAX_MATCHES; | ||
| } else { | ||
| delete process.env.GH_AW_SKIP_MAX_MATCHES; | ||
| } |
There was a problem hiding this comment.
The test suite saves/restores GH_AW_SKIP_QUERY/NAME/MAX, but not GH_AW_SKIP_SCOPE. If a developer (or CI) runs tests with GH_AW_SKIP_SCOPE already set (e.g. "none"), several expectations (repo scoping) will fail. Include GH_AW_SKIP_SCOPE in originalEnv and restore/delete it in afterEach (or explicitly delete it in beforeEach) to make the tests hermetic.
Summary
Cleaned
actions/setup/js/check_skip_if_match.cjsto align with the patterns used in its sibling filecheck_skip_if_no_match.cjs.Context type: github-script
Changes to
check_skip_if_match.cjsprocess.envat the top — replaces four individualprocess.env.Xreads with a single destructuring assignment (same style ascheck_skip_if_no_match.cjs)response.data.total_count→ direct{ data: { total_count: totalCount } }in theawaitexpressionChanges to
check_skip_if_match.test.cjsGH_AW_SKIP_MAX_MATCHES("-1") — validates the boundary check rejects negative integers (not just zero/NaN)Validation ✅
npm run format:cjs)npm run lint:cjs)npm run typecheck)npm run test:js --no-file-parallelism)check_skip_if_match.test.cjs— 16/16 tests passed