Conversation
- Use destructuring with defaults for allowed/blocked config arrays - Collapse single-line if-body logging statements for readability - Remove redundant comments that restate the code - Add 3 new edge case tests (total: 29 → 32 tests): - default max behavior when not provided - whitespace-only labels sanitized gracefully - initialization logging without allowed/blocked config Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot merge main |
There was a problem hiding this comment.
Pull request overview
Modernizes and “cleans” the add_labels safe-output handler and expands its test coverage with additional edge cases.
Changes:
- Refactors
add_labels.cjsconfig extraction (destructuring defaults) and condenses some logging conditionals. - Adds 3 new edge-case tests to
add_labels.test.cjsaround max defaults, whitespace labels, and initialization logging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| actions/setup/js/add_labels.cjs | Refactors config parsing/logging in the add-labels handler factory. |
| actions/setup/js/add_labels.test.cjs | Adds new edge-case tests for default behavior and logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const result = await handler({ item_number: 1, labels: ["bug"] }, {}); | ||
| expect(result.success).toBe(true); |
There was a problem hiding this comment.
This test name/doc comment says it verifies the default max=10, but it only asserts a single call succeeds. That would still pass for many incorrect defaults (e.g., 1, 100, or even undefined). To actually cover the || 10 behavior, assert that the handler allows 10 processed items and rejects the 11th (or otherwise observe maxCount being used).
| const result = await handler({ item_number: 1, labels: ["bug"] }, {}); | |
| expect(result.success).toBe(true); | |
| // First 10 items should succeed | |
| for (let i = 1; i <= 10; i++) { | |
| const result = await handler({ item_number: i, labels: ["bug"] }, {}); | |
| expect(result.success).toBe(true); | |
| } | |
| // 11th item should be rejected due to default max=10 | |
| const eleventhResult = await handler( | |
| { item_number: 11, labels: ["bug"] }, | |
| {} | |
| ); | |
| expect(eleventhResult.success).toBe(false); |
| const { allowed: allowedLabels = [], blocked: blockedPatterns = [] } = config; | ||
| const maxCount = config.max || 10; | ||
| const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); | ||
| const githubClient = await createAuthenticatedGitHubClient(config); | ||
|
|
||
| // Check if we're in staged mode | ||
| const isStaged = isStagedMode(config); | ||
|
|
||
| core.info(`Add labels configuration: max=${maxCount}`); | ||
| if (allowedLabels.length > 0) { | ||
| core.info(`Allowed labels: ${allowedLabels.join(", ")}`); | ||
| } | ||
| if (blockedPatterns.length > 0) { | ||
| core.info(`Blocked patterns: ${blockedPatterns.join(", ")}`); | ||
| } | ||
| if (allowedLabels.length > 0) core.info(`Allowed labels: ${allowedLabels.join(", ")}`); | ||
| if (blockedPatterns.length > 0) core.info(`Blocked patterns: ${blockedPatterns.join(", ")}`); |
There was a problem hiding this comment.
The destructuring default only applies when config.allowed/config.blocked are undefined, not when they are explicitly set to null. With the current change, a config like { allowed: null } will set allowedLabels to null and then allowedLabels.length will throw. The previous config.allowed || [] behavior treated null as “not configured”. Consider normalizing with const allowedLabels = config.allowed ?? [] and const blockedPatterns = config.blocked ?? [] (or equivalent) so null continues to fall back to [].
|
@copilot review comments |
…ls-d4dcb1eebb56c736 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
Cleaned
add_labels.cjs— the oldest uncleaned file — with targeted syntax modernizations and expanded test coverage.Context
Execution context:
github-script— usescore,github,contextglobals.Changes to
add_labels.cjsallowed/blockedconfig arrays, replacing individualconfig.x || []assignments:ifbodies — logging statements likeif (cond) { core.info(...) }→if (cond) core.info(...)for conciseness// Check if we're in staged mode) and// Extract configurationheader comment — the code speaks for itselfmaxCountkeeps|| 10(not changed to??) to preserve existing behavior wheremax: 0falls back to the default 10Changes to
add_labels.test.cjsAdded 3 new edge case tests (29 → 32 tests total):
should use default max=10 when config.max is not providedshould handle labels array containing only whitespace strings gracefullyshould log initialization info without allowed/blocked when not configuredValidation ✅
npm run format:cjs✓npm run lint:cjs✓ — All matched files use Prettier code style!npm run typecheck✓ — No errorsnpm run test:js -- --no-file-parallelism add_labels✓ — 32 passedThe 14 failures in the full suite (
assign_issue.test.cjs,git_helpers.test.cjs,safe_outputs_tools_loader.test.cjs) are pre-existing and unrelated to this change.