From 93958d04c167811b9953bae8fe784a60938c619f Mon Sep 17 00:00:00 2001 From: David Losert Date: Mon, 26 Aug 2024 20:28:32 +0200 Subject: [PATCH] chore: Refactors and unifies the getPullRequestNumber functionality --- src/index.ts | 2 +- src/inputs/getPullChanges.test.ts | 4 +- src/inputs/getPullChanges.ts | 4 +- src/inputs/getPullRequestNumber.test.ts | 117 ++++++++++++++++++++++++ src/inputs/getPullRequestNumber.ts | 83 ++++++++--------- src/inputs/options.ts | 45 +-------- src/report/generateCommitSHAUrl.test.ts | 4 +- src/writeSummaryToPR.test.ts | 4 +- 8 files changed, 165 insertions(+), 98 deletions(-) create mode 100644 src/inputs/getPullRequestNumber.test.ts diff --git a/src/index.ts b/src/index.ts index 7162112..5002b4d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,12 +9,12 @@ import { parseVitestJsonSummary, } from "./inputs/parseJsonReports.js"; import { createOctokit } from "./octokit.js"; +import { generateCommitSHAUrl } from "./report/generateCommitSHAUrl.js"; import { generateFileCoverageHtml } from "./report/generateFileCoverageHtml.js"; import { generateHeadline } from "./report/generateHeadline.js"; import { generateSummaryTableHtml } from "./report/generateSummaryTableHtml.js"; import type { JsonSummary } from "./types/JsonSummary.js"; import { writeSummaryToPR } from "./writeSummaryToPR.js"; -import { generateCommitSHAUrl } from "./report/generateCommitSHAUrl.js"; const run = async () => { const octokit = createOctokit(); diff --git a/src/inputs/getPullChanges.test.ts b/src/inputs/getPullChanges.test.ts index 4cdec0c..bec0baa 100644 --- a/src/inputs/getPullChanges.test.ts +++ b/src/inputs/getPullChanges.test.ts @@ -1,7 +1,7 @@ -import { beforeEach, describe, expect, it, vi, Mock } from "vitest"; +import { Mock, beforeEach, describe, expect, it, vi } from "vitest"; +import type { Octokit } from "../octokit"; import { FileCoverageMode } from "./FileCoverageMode"; import { getPullChanges } from "./getPullChanges"; -import type { Octokit } from "../octokit"; // Avoid logs vi.mock("@actions/core"); diff --git a/src/inputs/getPullChanges.ts b/src/inputs/getPullChanges.ts index 9be524a..8febb38 100644 --- a/src/inputs/getPullChanges.ts +++ b/src/inputs/getPullChanges.ts @@ -1,8 +1,8 @@ import * as core from "@actions/core"; import * as github from "@actions/github"; import { RequestError } from "@octokit/request-error"; -import { FileCoverageMode } from "./FileCoverageMode"; import type { Octokit } from "../octokit"; +import { FileCoverageMode } from "./FileCoverageMode"; interface Params { fileCoverageMode: FileCoverageMode; @@ -13,7 +13,7 @@ interface Params { export async function getPullChanges({ fileCoverageMode, prNumber, - octokit + octokit, }: Params): Promise { // Skip Changes collection if we don't need it if (fileCoverageMode === FileCoverageMode.None) { diff --git a/src/inputs/getPullRequestNumber.test.ts b/src/inputs/getPullRequestNumber.test.ts new file mode 100644 index 0000000..b646de2 --- /dev/null +++ b/src/inputs/getPullRequestNumber.test.ts @@ -0,0 +1,117 @@ +import type * as core from "@actions/core"; +import { + type Mock, + afterEach, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; +import type { Octokit } from "../octokit"; +import { getPullRequestNumber } from "./getPullRequestNumber"; + +type Core = typeof core; + +// Avoid logs +vi.mock("@actions/core", async (importOriginal): Promise => { + const original: Core = await importOriginal(); + return { + ...original, + startGroup: vi.fn(), + info: vi.fn(), + warning: vi.fn(), + debug: vi.fn(), + }; +}); + +const mockContext = vi.hoisted(() => ({ + repo: { + owner: "owner", + repo: "repo", + }, + payload: {}, + eventName: "", +})); +vi.mock("@actions/github", () => ({ + context: mockContext, +})); + +describe("getPullRequestNumber()", () => { + let mockOctokit: Octokit; + beforeEach(() => { + vi.clearAllMocks(); + mockOctokit = { + paginate: { + iterator: vi.fn(), + }, + rest: { + pulls: { + list: vi.fn(), + }, + }, + } as unknown as Octokit; + }); + + afterEach(() => { + mockContext.payload = {}; + mockContext.eventName = ""; + vi.unstubAllEnvs(); + }); + + it("returns the PR number from the input 'pr-number' if valid ", async () => { + vi.stubEnv("INPUT_PR-NUMBER", "123"); + + const result = await getPullRequestNumber(mockOctokit); + expect(result).toBe(123); + }); + + it("returns the PR number from payload.pull_request context if found", async () => { + mockContext.payload = { + pull_request: { + number: 456, + }, + }; + + const result = await getPullRequestNumber(mockOctokit); + expect(result).toBe(456); + }); + + it("returns the PR number from payload.workflow_run context if found", async () => { + mockContext.eventName = "workflow_run"; + mockContext.payload = { + workflow_run: { + pull_requests: [{ number: 789 }], + head_sha: "testsha", + }, + }; + + const result = await getPullRequestNumber(mockOctokit); + expect(result).toBe(789); + }); + + it("calls the API to find PR number by the head_sha of the payload.workflow_run when called from a fork", async () => { + mockContext.eventName = "workflow_run"; + mockContext.payload = { + workflow_run: { + pull_requests: [], + head_sha: "testsha", + }, + }; + + (mockOctokit.paginate.iterator as Mock).mockReturnValue([ + { + data: [{ number: 101, head: { sha: "testsha" } }], + }, + ]); + + const result = await getPullRequestNumber(mockOctokit); + expect(result).toBe(101); + }); + + it("returns undefined if no pr number is found", async () => { + mockContext.payload = {}; + const result = await getPullRequestNumber(mockOctokit); + expect(result).toBeUndefined(); + }); +}); diff --git a/src/inputs/getPullRequestNumber.ts b/src/inputs/getPullRequestNumber.ts index 8a498a4..743f55c 100644 --- a/src/inputs/getPullRequestNumber.ts +++ b/src/inputs/getPullRequestNumber.ts @@ -2,66 +2,54 @@ import * as core from "@actions/core"; import * as github from "@actions/github"; import type { Octokit } from "../octokit"; -async function getPullRequestNumberFromTriggeringWorkflow( +async function getPullRequestNumber( octokit: Octokit, ): Promise { - core.info( - "Trying to get the triggering workflow in order to find the pull-request-number to comment the results on...", - ); + // Get the user-defined pull-request number and perform input validation + const prNumberFromInput = core.getInput("pr-number"); + const processedPrNumber: number | undefined = Number(prNumberFromInput); - if (!github.context.payload.workflow_run) { - core.info( - "The triggering workflow does not have a workflow_run payload. Skipping comment creation.", - ); - return undefined; + // Check if it is a full integer. Check for non-null as qhen the option is not set, the parsed input will be an empty string + // which becomes 0 when parsed to a number. + if (Number.isSafeInteger(processedPrNumber) && processedPrNumber !== 0) { + core.info(`Received pull-request number: ${processedPrNumber}`); + return processedPrNumber; } - const originalWorkflowRunId = github.context.payload.workflow_run.id; - - const { data: originalWorkflowRun } = - await octokit.rest.actions.getWorkflowRun({ - owner: github.context.repo.owner, - repo: github.context.repo.repo, - run_id: originalWorkflowRunId, - }); - - if (originalWorkflowRun.event !== "pull_request") { + if (github.context.payload.pull_request) { core.info( - "The triggering workflow is not a pull-request. Skipping comment creation.", + `Found pull-request number in the action's "payload.pull_request" context: ${github.context.payload.pull_request.number}`, ); - return undefined; + return github.context.payload.pull_request.number; } - // When the actual pull-request is not coming from a fork, the pull_request object is correctly populated and we can shortcut here - if ( - originalWorkflowRun.pull_requests && - originalWorkflowRun.pull_requests.length > 0 - ) { - return originalWorkflowRun.pull_requests[0].number; - } - - // When the actual pull-request is coming from a fork, the pull_request object is not populated (see https://github.com/orgs/community/discussions/25220) - core.info( - `Trying to find the pull-request for the triggering workflow run with id: ${originalWorkflowRunId} (${originalWorkflowRun.url}) with HEAD_SHA ${originalWorkflowRun.head_sha}...`, - ); - - // The way to find the pull-request in this scenario is to query all existing pull_requests on the target repository and find the one with the same HEAD_SHA as the original workflow run - const pullRequest = await findPullRequest( - octokit, - originalWorkflowRun.head_sha, - ); + if (github.context.eventName === "workflow_run") { + // Workflow_runs triggered from non-forked PRs will have the PR number in the payload + if (github.context.payload.workflow_run.pull_requests.length > 0) { + core.debug( + `Found pull-request number in the action's "payload.workflow_run" context: ${github.context.payload.workflow_run.pull_requests[0].number}`, + ); + return github.context.payload.workflow_run.pull_requests[0].number; + } - if (!pullRequest) { - core.info( - "Could not find the pull-request for the triggering workflow run. Skipping comment creation.", + // ... in all other cases, we have to call the API to get a matching PR number + core.debug( + "Trying to find pull-request number in payload.workflow_run context by calling the API", + ); + return await findPullRequestBySHA( + octokit, + github.context.payload.workflow_run.head_sha, ); - return undefined; } - return pullRequest.number; + core.info("No pull-request number found. Comment creation will be skipped!"); + return undefined; } -async function findPullRequest(octokit: Octokit, headSha: string) { +async function findPullRequestBySHA( + octokit: Octokit, + headSha: string, +): Promise { core.startGroup("Querying REST API for pull-requests."); const pullRequestsIterator = octokit.paginate.iterator( octokit.rest.pulls.list, @@ -79,12 +67,13 @@ async function findPullRequest(octokit: Octokit, headSha: string) { `Comparing: ${pullRequest.number} sha: ${pullRequest.head.sha} with expected: ${headSha}.`, ); if (pullRequest.head.sha === headSha) { - return pullRequest; + return pullRequest.number; } } } core.endGroup(); + core.info(`Could not find the pull-request for commit "${headSha}".`); return undefined; } -export { getPullRequestNumberFromTriggeringWorkflow }; +export { getPullRequestNumber }; diff --git a/src/inputs/options.ts b/src/inputs/options.ts index 376f40a..65fc8f9 100644 --- a/src/inputs/options.ts +++ b/src/inputs/options.ts @@ -4,7 +4,7 @@ import * as github from "@actions/github"; import type { Octokit } from "../octokit"; import type { Thresholds } from "../types/Threshold"; import { type FileCoverageMode, getCoverageModeFrom } from "./FileCoverageMode"; -import { getPullRequestNumberFromTriggeringWorkflow } from "./getPullRequestNumber"; +import { getPullRequestNumber } from "./getPullRequestNumber"; import { getViteConfigPath } from "./getViteConfigPath"; import { parseCoverageThresholds } from "./parseCoverageThresholds"; @@ -58,9 +58,9 @@ async function readOptions(octokit: Octokit): Promise { ? await parseCoverageThresholds(viteConfigPath) : {}; - // Get the user-defined pull-request number and perform input validation - const prNumber = await getPrNumber(octokit); const commitSHA = getCommitSHA(); + // Get the user-defined pull-request number and perform input validation + const prNumber = await getPullRequestNumber(octokit); return { fileCoverageMode, @@ -75,45 +75,6 @@ async function readOptions(octokit: Octokit): Promise { }; } -async function getPrNumber(octokit: Octokit): Promise { - // Get the user-defined pull-request number and perform input validation - const prNumberFromInput = core.getInput("pr-number"); - const processedPrNumber: number | undefined = Number(prNumberFromInput); - - // Check if it is a full integer. Check for non-null as qhen the option is not set, the parsed input will be an empty string - // which becomes 0 when parsed to a number. - if (Number.isSafeInteger(processedPrNumber) && processedPrNumber !== 0) { - core.debug(`Received pull-request number: ${processedPrNumber}`); - return processedPrNumber; - } - - if (github.context.payload.pull_request) { - core.debug( - `Found pull-request number in payload.pull_request context: ${github.context.payload.pull_request.number}`, - ); - return github.context.payload.pull_request.number; - } - - if (github.context.eventName === "workflow_run") { - // Workflow_runs triggered from non-forked PRs will have the PR number in the payload - if (github.context.payload.workflow_run.pull_requests.length > 0) { - core.debug( - `Found pull-request number in payload.workflow_run context: ${github.context.payload.workflow_run.pull_requests[0].number}`, - ); - return github.context.payload.workflow_run.pull_requests[0].number; - } - - // ... in all other cases, we have to call the API to get a matching PR number - core.debug( - "Trying to find pull-request number in payload.workflow_run context by calling the API", - ); - return await getPullRequestNumberFromTriggeringWorkflow(octokit); - } - - core.debug("No pull-request number found."); - return undefined; -} - function getCommitSHA(): string { if ( github.context.eventName === "pull_request" || diff --git a/src/report/generateCommitSHAUrl.test.ts b/src/report/generateCommitSHAUrl.test.ts index 7f2bbb5..724dddb 100644 --- a/src/report/generateCommitSHAUrl.test.ts +++ b/src/report/generateCommitSHAUrl.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { generateCommitSHAUrl } from "./generateCommitSHAUrl"; const mockContext = vi.hoisted(() => ({ @@ -6,7 +6,7 @@ const mockContext = vi.hoisted(() => ({ owner: "owner", repo: "repo", }, - serverUrl: "https://github.com", + serverUrl: "https://github.com", payload: {}, })); vi.mock("@actions/github", () => ({ diff --git a/src/writeSummaryToPR.test.ts b/src/writeSummaryToPR.test.ts index 79578a1..a8000e8 100644 --- a/src/writeSummaryToPR.test.ts +++ b/src/writeSummaryToPR.test.ts @@ -1,8 +1,8 @@ -import { beforeEach, describe, expect, it, vi, Mock } from "vitest"; import * as core from "@actions/core"; import * as github from "@actions/github"; -import { writeSummaryToPR } from "./writeSummaryToPR"; +import { Mock, beforeEach, describe, expect, it, vi } from "vitest"; import type { Octokit } from "./octokit"; +import { writeSummaryToPR } from "./writeSummaryToPR"; vi.mock("@actions/core");