Skip to content

Deduplicate skip-query gate logic for setup action guards #43480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 5, 2026
Merged

Deduplicate skip-query gate logic for setup action guards #43480

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d70242
Initial plan
Copilot Jul 5, 2026
aa78625
refactor: deduplicate skip query gate logic
Copilot Jul 5, 2026
08ad2b9
refactor: address review feedback on runSkipQueryGate helper
Copilot Jul 5, 2026
21dac8f
fix: resolve TS2345 type error and address remaining review threads
Copilot Jul 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: address review feedback on runSkipQueryGate helper 
- Break single-line destructure to multi-line (Prettier at printWidth:240 normalizes it; semantically correct)
- Use workflowName in log line — eliminates dead validation
- Remove thresholdStr default from helper (callers own their defaults)
- Move runSkipQueryGate describe to top-level sibling of buildSearchQuery
- Remove duplicate loadModule inside runSkipQueryGate describe
- Add missing test cases: skipQuery/workflowName early-exit guards,
  success path (skip condition not met), API error path
- Assert writeDenialSummary side effects in skip-path test

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
  • Loading branch information
@pelikhan
Copilot and pelikhan authored Jul 5, 2026
commit 08ad2b900d827fab84b4a6191f506a565fc031ca
4 changes: 3 additions & 1 deletion actions/setup/js/check_skip_if_helpers.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ function buildSearchQuery(skipQuery, skipScope) {
* }} options
*/
async function runSkipQueryGate(options) {
const { skipQuery, workflowName, thresholdStr = "1", thresholdEnvVar, thresholdLabel, checkLabel, outputName, skipScope, shouldSkip, warningMessage, successMessage, denialSummaryMessage, denialSummaryNextStep } = options;
const { skipQuery, workflowName, thresholdStr, thresholdEnvVar, thresholdLabel, checkLabel, outputName, skipScope, shouldSkip, warningMessage, successMessage, denialSummaryMessage, denialSummaryNextStep } = options;

if (!skipQuery) {
core.setFailed(`${ERR_CONFIG}: Configuration error: GH_AW_SKIP_QUERY not specified.`);

@github-actions github-actions Bot Jul 5, 2026

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.

The runSkipQueryGate function uses core, github, and context as globals — consistent with how the callers work under @actions/github-script. However, those globals are not declared or imported in this helper module, so static analysis (and IDE tooling) cannot verify them. Consider adding a brief comment referencing the expected ambient globals, or — if the module is ever tested in isolation without a global-script harness — accept them as implicit. The test file's beforeEach does set them up, so tests are fine; this is a documentation/discoverability concern only.

@copilot please address this.

@github-actions github-actions Bot Jul 5, 2026

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.

The destructuring line is very long (170+ chars) — the entire options bag unpacked on one line. Breaking it into a multi-line destructure would aid readability and make diffs easier to review if individual parameters change later.

const {
  skipQuery, workflowName, thresholdStr = "1",
  thresholdEnvVar, thresholdLabel, checkLabel, outputName, skipScope,
  shouldSkip, warningMessage, successMessage,
  denialSummaryMessage, denialSummaryNextStep,
} = options;

@copilot please address this.

Expand All @@ -53,6 +53,8 @@ async function runSkipQueryGate(options) {
return;
}

core.info(`Running ${checkLabel} gate for workflow: ${workflowName}`);

const threshold = parseInt(thresholdStr, 10);
if (isNaN(threshold) || threshold < 1) {
core.setFailed(`${ERR_CONFIG}: Configuration error: ${thresholdEnvVar} must be a positive integer, got "${thresholdStr}".`);
Expand Down
149 changes: 87 additions & 62 deletions actions/setup/js/check_skip_if_helpers.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,68 +69,6 @@ describe("check_skip_if_helpers.cjs - buildSearchQuery", () => {
});
});

describe("check_skip_if_helpers.cjs - runSkipQueryGate", () => {
/** @returns {Promise<{runSkipQueryGate: (options: any) => Promise<void>}>} */
const loadModule = () => import("./check_skip_if_helpers.cjs");

beforeEach(() => {
global.github = {
rest: {
search: {
issuesAndPullRequests: vi.fn(),
},
},
};
});

it("uses thresholdEnvVar in validation errors", async () => {
const { runSkipQueryGate } = await loadModule();

await runSkipQueryGate({
skipQuery: "is:issue is:open",
workflowName: "test-workflow",
thresholdStr: "invalid",
thresholdEnvVar: "GH_AW_SKIP_MAX_MATCHES",
thresholdLabel: "Maximum matches threshold",
checkLabel: "skip-if-match",
outputName: "skip_check_ok",
skipScope: undefined,
shouldSkip: () => false,
warningMessage: () => "",
successMessage: () => "",
denialSummaryMessage: () => "",
denialSummaryNextStep: "",
});

expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("GH_AW_SKIP_MAX_MATCHES must be a positive integer"));
expect(global.github.rest.search.issuesAndPullRequests).not.toHaveBeenCalled();
});

it("applies policy decision and output name when skip condition matches", async () => {
global.github.rest.search.issuesAndPullRequests.mockResolvedValue({ data: { total_count: 2 } });
const { runSkipQueryGate } = await loadModule();

await runSkipQueryGate({
skipQuery: "is:issue is:open",
workflowName: "test-workflow",
thresholdStr: "1",
thresholdEnvVar: "GH_AW_SKIP_MAX_MATCHES",
thresholdLabel: "Maximum matches threshold",
checkLabel: "skip-if-match",
outputName: "skip_check_ok",
skipScope: undefined,
shouldSkip: (totalCount, threshold) => totalCount >= threshold,
warningMessage: () => "skip warning",
successMessage: () => "success message",
denialSummaryMessage: () => "summary message",
denialSummaryNextStep: "summary next step",
});

expect(mockCore.warning).toHaveBeenCalledWith("skip warning");
expect(mockCore.setOutput).toHaveBeenCalledWith("skip_check_ok", "false");
});
});

describe("scope: default (repo-scoped)", () => {
it("appends repo:owner/repo when skipScope is undefined", async () => {
const { buildSearchQuery } = await loadModule();
Expand Down Expand Up @@ -171,3 +109,90 @@ describe("check_skip_if_helpers.cjs - buildSearchQuery", () => {
});
});
});

describe("check_skip_if_helpers.cjs - runSkipQueryGate", () => {
/** @returns {Promise<{runSkipQueryGate: (options: any) => Promise<void>}>} */
const loadModule = () => import("./check_skip_if_helpers.cjs");

/** @type {Record<string, any>} */
const baseOptions = {
skipQuery: "is:issue is:open",
workflowName: "test-workflow",
thresholdStr: "1",
thresholdEnvVar: "GH_AW_SKIP_MAX_MATCHES",
thresholdLabel: "Maximum matches threshold",
checkLabel: "skip-if-match",
outputName: "skip_check_ok",
skipScope: undefined,
shouldSkip: (/** @type {number} */ totalCount, /** @type {number} */ threshold) => totalCount >= threshold,
warningMessage: () => "skip warning",
successMessage: () => "success message",
denialSummaryMessage: () => "summary message",
denialSummaryNextStep: "summary next step",
};

beforeEach(() => {
global.github = {
rest: {
search: {
issuesAndPullRequests: vi.fn(),
},
},
};
});

it("fails with config error when skipQuery is undefined", async () => {
const { runSkipQueryGate } = await loadModule();
await runSkipQueryGate({ ...baseOptions, skipQuery: undefined });
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("GH_AW_SKIP_QUERY not specified"));
expect(global.github.rest.search.issuesAndPullRequests).not.toHaveBeenCalled();
});

it("fails with config error when workflowName is undefined", async () => {
const { runSkipQueryGate } = await loadModule();
await runSkipQueryGate({ ...baseOptions, workflowName: undefined });
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("GH_AW_WORKFLOW_NAME not specified"));
expect(global.github.rest.search.issuesAndPullRequests).not.toHaveBeenCalled();
});

it("uses thresholdEnvVar in validation errors", async () => {
const { runSkipQueryGate } = await loadModule();
await runSkipQueryGate({ ...baseOptions, thresholdStr: "invalid" });
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("GH_AW_SKIP_MAX_MATCHES must be a positive integer"));
expect(global.github.rest.search.issuesAndPullRequests).not.toHaveBeenCalled();
});

it("applies policy decision and output name when skip condition matches", async () => {
global.github.rest.search.issuesAndPullRequests.mockResolvedValue({ data: { total_count: 2 } });
const { runSkipQueryGate } = await loadModule();

await runSkipQueryGate({ ...baseOptions });

expect(mockCore.warning).toHaveBeenCalledWith("skip warning");
expect(mockCore.setOutput).toHaveBeenCalledWith("skip_check_ok", "false");
expect(mockCore.summary.addRaw).toHaveBeenCalled();
expect(mockCore.summary.write).toHaveBeenCalled();
});

it("sets output to true and logs success when skip condition is not met", async () => {
global.github.rest.search.issuesAndPullRequests.mockResolvedValue({ data: { total_count: 0 } });
const { runSkipQueryGate } = await loadModule();
const successMessage = vi.fn(() => "all clear");

await runSkipQueryGate({ ...baseOptions, successMessage });

expect(mockCore.setOutput).toHaveBeenCalledWith("skip_check_ok", "true");
expect(successMessage).toHaveBeenCalledWith(0, 1);
expect(mockCore.warning).not.toHaveBeenCalled();
});

it("calls setFailed with ERR_API prefix when search throws", async () => {
global.github.rest.search.issuesAndPullRequests.mockRejectedValue(new Error("rate limited"));
const { runSkipQueryGate } = await loadModule();

await runSkipQueryGate({ ...baseOptions });

expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("ERR_API"));
expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("rate limited"));
});
});
Loading