diff --git a/.github/workflows/arm-auto-signoff-code.yaml b/.github/workflows/arm-auto-signoff-code.yaml index 632d9a2a6061..06dc456812bf 100644 --- a/.github/workflows/arm-auto-signoff-code.yaml +++ b/.github/workflows/arm-auto-signoff-code.yaml @@ -28,7 +28,8 @@ jobs: steps: - uses: actions/checkout@v6 with: - # Required to detect changed files in PR + # We compare HEAD (PR merge commit) against HEAD^ (base) via git diff, + # so we only need enough history to include the merge commit and its parent. fetch-depth: 2 # Check only needs to view contents of changed files as a string, so can use # "git show" on specific files instead of cloning whole repo @@ -38,19 +39,25 @@ jobs: - name: Install dependencies for github-script actions uses: ./.github/actions/install-deps-github-script - # Output is "true" if PR contains only incremental changes to an existing TypeSpec RP - - id: incremental-typespec + - id: arm-auto-signoff-code name: ARM Auto SignOff - Analyze Code uses: actions/github-script@v8 with: - result-encoding: string + result-encoding: json script: | - const { default: incrementalTypeSpec } = - await import('${{ github.workspace }}/.github/workflows/src/arm-auto-signoff/arm-incremental-typespec.js'); - return await incrementalTypeSpec({ github, context, core }); + const { default: armAutoSignoffCode } = + await import('${{ github.workspace }}/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-code.js'); + return await armAutoSignoffCode({ github, context, core }); - - name: Upload artifact with results + # Upload artifacts with individual results + - name: Upload artifact - ARM Auto Signoff Code Result (incremental) uses: ./.github/actions/add-empty-artifact with: name: "incremental-typespec" - value: ${{ steps.incremental-typespec.outputs.result }} + value: ${{ fromJSON(steps.arm-auto-signoff-code.outputs.result).incremental }} + + - name: Upload artifact - ARM Auto Signoff Code Result (trivial) + uses: ./.github/actions/add-empty-artifact + with: + name: "trivial-changes" + value: ${{ fromJSON(steps.arm-auto-signoff-code.outputs.result).trivial }} diff --git a/.github/workflows/arm-auto-signoff-status.yaml b/.github/workflows/arm-auto-signoff-status.yaml index 7554492b1790..15578381d4af 100644 --- a/.github/workflows/arm-auto-signoff-status.yaml +++ b/.github/workflows/arm-auto-signoff-status.yaml @@ -45,6 +45,8 @@ jobs: github.event.action == 'unlabeled') && (github.event.label.name == 'Approved-Suppression' || github.event.label.name == 'ARMAutoSignedOff' || + github.event.label.name == 'ARMAutoSignedOff-IncrementalTSP' || + github.event.label.name == 'ARMAutoSignedOff-Trivial-Test' || github.event.label.name == 'ARMReview' || github.event.label.name == 'ARMSignedOff' || github.event.label.name == 'NotReadyForARMReview' || @@ -69,8 +71,13 @@ jobs: # Output: # { - # labelAction: LabelAction, ("none" for no-op, "add" to add label, and "remove" to remove label) - # issueNumber: number + # headSha: string, + # issueNumber: number, + # labelActions: { + # 'ARMSignedOff': 'none'|'add'|'remove', + # 'ARMAutoSignedOff-IncrementalTSP': 'none'|'add'|'remove', + # 'ARMAutoSignedOff-Trivial-Test': 'none'|'add'|'remove' + # } # } - id: get-label-action name: ARM Auto SignOff - Set Status @@ -81,25 +88,28 @@ jobs: await import('${{ github.workspace }}/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-status.js'); return await getLabelAction({ github, context, core }); - - if: | - fromJson(steps.get-label-action.outputs.result).labelAction == 'add' || - fromJson(steps.get-label-action.outputs.result).labelAction == 'remove' - name: Upload artifact with results + # Add/remove specific auto sign-off labels based on analysis results. + # The action is explicit: 'add' | 'remove' | 'none'. + - if: fromJson(steps.get-label-action.outputs.result).labelActions['ARMSignedOff'] != 'none' + name: Upload artifact for ARMSignedOff label uses: ./.github/actions/add-label-artifact with: - name: "ARMAutoSignedOff" - # Convert "add/remove" to "true/false" - value: "${{ fromJson(steps.get-label-action.outputs.result).labelAction == 'add' }}" + name: "ARMSignedOff" + value: "${{ fromJson(steps.get-label-action.outputs.result).labelActions['ARMSignedOff'] == 'add' }}" - - if: | - fromJson(steps.get-label-action.outputs.result).labelAction == 'add' || - fromJson(steps.get-label-action.outputs.result).labelAction == 'remove' - name: Upload artifact with results + - if: fromJson(steps.get-label-action.outputs.result).labelActions['ARMAutoSignedOff-IncrementalTSP'] != 'none' + name: Upload artifact for ARMAutoSignedOff-IncrementalTSP label uses: ./.github/actions/add-label-artifact with: - name: "ARMSignedOff" - # Convert "add/remove" to "true/false" - value: "${{ fromJson(steps.get-label-action.outputs.result).labelAction == 'add' }}" + name: "ARMAutoSignedOff-IncrementalTSP" + value: "${{ fromJson(steps.get-label-action.outputs.result).labelActions['ARMAutoSignedOff-IncrementalTSP'] == 'add' }}" + + - if: fromJson(steps.get-label-action.outputs.result).labelActions['ARMAutoSignedOff-Trivial-Test'] != 'none' + name: Upload artifact for ARMAutoSignedOff-Trivial-Test label + uses: ./.github/actions/add-label-artifact + with: + name: "ARMAutoSignedOff-Trivial-Test" + value: "${{ fromJson(steps.get-label-action.outputs.result).labelActions['ARMAutoSignedOff-Trivial-Test'] == 'add' }}" # Required for consumers to identify the head SHA associated with this workflow run. # Output can be trusted, because it was uploaded from a workflow that is trusted, diff --git a/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-code.js b/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-code.js new file mode 100644 index 000000000000..f391644e190d --- /dev/null +++ b/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-code.js @@ -0,0 +1,46 @@ +import { incrementalTypeSpec } from "./arm-incremental-typespec.js"; +import { checkTrivialChanges } from "./trivial-changes-check.js"; + +/** @typedef {import("./pr-changes.js").PullRequestChanges} PullRequestChanges */ + +/** + * Main entry point for ARM Auto Signoff Code workflow. + * + * This module orchestrates all checks to determine if a PR qualifies for auto sign-off. + * It combines results from: + * - Incremental TypeSpec check + * - Trivial changes check + * + * @param {import("@actions/github-script").AsyncFunctionArguments} args + * @returns {Promise<{ incremental: boolean, trivial: boolean }>} + */ +export default async function armAutoSignoffCode({ core }) { + core.info("Starting ARM Auto Signoff Code analysis."); + + // Run incremental TypeSpec check + core.startGroup("Checking for incremental TypeSpec changes"); + const incrementalTypeSpecResult = await incrementalTypeSpec(core); + core.info(`Incremental TypeSpec result: ${incrementalTypeSpecResult}`); + core.endGroup(); + + // Run trivial changes check + core.startGroup("Checking for trivial changes"); + const trivialChangesResult = await checkTrivialChanges(core); + core.endGroup(); + + const isTrivial = trivialChangesResult.isTrivial(); + + // Combine results + const combined = { + incrementalTypeSpec: incrementalTypeSpecResult, + trivialChanges: trivialChangesResult, + }; + + core.info(`Combined result: ${JSON.stringify(combined, null, 2)}`); + + // Return a JSON object for downstream workflow logic. + return { + incremental: incrementalTypeSpecResult, + trivial: isTrivial, + }; +} diff --git a/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-labels.js b/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-labels.js new file mode 100644 index 000000000000..c0e7ba1f66cc --- /dev/null +++ b/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-labels.js @@ -0,0 +1,11 @@ +/** + * ARM auto-signoff label names. + * @readonly + * @enum {string} + */ +export const ArmAutoSignoffLabel = Object.freeze({ + ArmSignedOff: "ARMSignedOff", + ArmAutoSignedOff: "ARMAutoSignedOff", + ArmAutoSignedOffIncrementalTSP: "ARMAutoSignedOff-IncrementalTSP", + ArmAutoSignedOffTrivialTest: "ARMAutoSignedOff-Trivial-Test", +}); diff --git a/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-status.js b/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-status.js index cf598f5495f3..63a01865463b 100644 --- a/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-status.js +++ b/.github/workflows/src/arm-auto-signoff/arm-auto-signoff-status.js @@ -4,12 +4,56 @@ import { equals } from "../../../shared/src/set.js"; import { byDate, invert } from "../../../shared/src/sort.js"; import { extractInputs } from "../context.js"; import { LabelAction } from "../label.js"; +import { ArmAutoSignoffLabel } from "./arm-auto-signoff-labels.js"; + +/** @typedef {import('@octokit/plugin-rest-endpoint-methods').RestEndpointMethodTypes} RestEndpointMethodTypes */ +/** @typedef {RestEndpointMethodTypes["issues"]["listLabelsOnIssue"]["response"]["data"][number]} IssueLabel */ +/** @typedef {RestEndpointMethodTypes["actions"]["listWorkflowRunsForRepo"]["response"]["data"]["workflow_runs"][number]} WorkflowRun */ +/** @typedef {RestEndpointMethodTypes["repos"]["listCommitStatusesForRef"]["response"]["data"][number]} CommitStatus */ +/** @typedef {RestEndpointMethodTypes["actions"]["listWorkflowRunArtifacts"]["response"]["data"]["artifacts"][number]} Artifact */ + +/** + * The workflow contract is intentionally a fixed set of keys. + * @typedef {{ + * "ARMSignedOff": LabelAction, + * "ARMAutoSignedOff-IncrementalTSP": LabelAction, + * "ARMAutoSignedOff-Trivial-Test": LabelAction, + * }} ManagedLabelActions + */ + +/** @returns {ManagedLabelActions} */ +function createNoneLabelActions() { + return { + [ArmAutoSignoffLabel.ArmSignedOff]: LabelAction.None, + [ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP]: LabelAction.None, + [ArmAutoSignoffLabel.ArmAutoSignedOffTrivialTest]: LabelAction.None, + }; +} + +/** + * The analyze-code workflow uploads empty artifacts named `${name}=${value}`. + * We treat missing artifacts or non-boolean values as a failure (no auto-signoff). + * + * @param {string[]} artifactNames + * @param {string} key + * @returns {boolean | null} + */ +function readBooleanArtifactValue(artifactNames, key) { + const prefix = `${key}=`; + const match = artifactNames.find((name) => name.startsWith(prefix)); + if (!match) { + return null; + } + + const value = match.substring(prefix.length).trim().toLowerCase(); + return value === "true" ? true : value === "false" ? false : null; +} // TODO: Add tests /* v8 ignore start */ /** * @param {import('@actions/github-script').AsyncFunctionArguments} AsyncFunctionArguments - * @returns {Promise<{labelAction: LabelAction, issueNumber: number}>} + * @returns {Promise<{headSha: string, issueNumber: number, labelActions: ManagedLabelActions}>} */ export default async function getLabelAction({ github, context, core }) { const { owner, repo, issue_number, head_sha } = await extractInputs(github, context, core); @@ -33,29 +77,20 @@ export default async function getLabelAction({ github, context, core }) { * @param {string} params.head_sha * @param {(import("@octokit/core").Octokit & import("@octokit/plugin-rest-endpoint-methods/dist-types/types.js").Api & { paginate: import("@octokit/plugin-paginate-rest").PaginateInterface; })} params.github * @param {typeof import("@actions/core")} params.core - * @returns {Promise<{labelAction: LabelAction, headSha: string, issueNumber: number}>} + * @returns {Promise<{headSha: string, issueNumber: number, labelActions: ManagedLabelActions}>} */ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha, github, core }) { - const labelActions = { - [LabelAction.None]: { - labelAction: LabelAction.None, - headSha: head_sha, - issueNumber: issue_number, - }, - [LabelAction.Add]: { - labelAction: LabelAction.Add, - headSha: head_sha, - issueNumber: issue_number, - }, - [LabelAction.Remove]: { - labelAction: LabelAction.Remove, - headSha: head_sha, - issueNumber: issue_number, - }, + /** @type {{headSha: string, issueNumber: number}} */ + const baseResult = { + headSha: head_sha, + issueNumber: issue_number, }; + const noneLabelActions = createNoneLabelActions(); + // TODO: Try to extract labels from context (when available) to avoid unnecessary API call // permissions: { issues: read, pull-requests: read } + /** @type {IssueLabel[]} */ const labels = await github.paginate(github.rest.issues.listLabelsOnIssue, { owner: owner, repo: repo, @@ -64,15 +99,17 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha, }); const labelNames = labels.map((label) => label.name); - // Only remove labels "ARMSignedOff" and "ARMAutoSignedOff", if "ARMAutoSignedOff" is currently present. - // Necessary to prevent removing "ARMSignedOff" if added by a human reviewer. - const removeAction = labelNames.includes("ARMAutoSignedOff") - ? labelActions[LabelAction.Remove] - : labelActions[LabelAction.None]; - + // Check if any auto sign-off labels are currently present + // Only proceed with auto sign-off logic if auto labels exist or we're about to add them + const hasAutoSignedOffLabels = + labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOff) || + labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP) || + labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOffTrivialTest); core.info(`Labels: ${inspect(labelNames)}`); + core.info(`Has auto signed-off labels: ${hasAutoSignedOffLabels}`); // permissions: { actions: read } + /** @type {WorkflowRun[]} */ const workflowRuns = await github.paginate(github.rest.actions.listWorkflowRunsForRepo, { owner, repo, @@ -86,54 +123,33 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha, core.info(`- ${wf.name}: ${wf.conclusion || wf.status}`); }); - const wfName = "ARM Auto SignOff - Analyze Code"; - const incrementalTspRuns = workflowRuns - .filter((wf) => wf.name == wfName) - // Sort by "updated_at" descending - .sort((a, b) => new Date(b.updated_at).getTime() - new Date(a.updated_at).getTime()); + // Check ARM Auto SignOff - Analyze Code workflow results + const armAnalysisResult = await checkArmAnalysisWorkflow(workflowRuns, github, owner, repo, core); - if (incrementalTspRuns.length == 0) { - core.info( - `Found no runs for workflow '${wfName}'. Assuming workflow trigger was skipped, which should be treated equal to "completed false".`, - ); - return removeAction; - } else { - // Sorted by "updated_at" descending, so most recent run is at index 0 - const run = incrementalTspRuns[0]; - - if (run.status == "completed") { - if (run.conclusion != "success") { - core.info(`Run for workflow '${wfName}' did not succeed: '${run.conclusion}'`); - return removeAction; - } - - // permissions: { actions: read } - const artifacts = await github.paginate(github.rest.actions.listWorkflowRunArtifacts, { - owner, - repo, - run_id: run.id, - per_page: PER_PAGE_MAX, - }); - const artifactNames = artifacts.map((a) => a.name); - - core.info(`artifactNames: ${JSON.stringify(artifactNames)}`); - - if (artifactNames.includes("incremental-typespec=false")) { - core.info("Spec is not an incremental change to an existing TypeSpec RP"); - return removeAction; - } else if (artifactNames.includes("incremental-typespec=true")) { - core.info("Spec is an incremental change to an existing TypeSpec RP"); - // Continue checking other requirements - } else { - // If workflow succeeded, it should have one workflow or the other - throw new Error( - `Workflow artifacts did not contain 'incremental-typespec': ${JSON.stringify(artifactNames)}`, - ); - } - } else { - core.info(`Workflow '${wfName}' is still in-progress: status='${run.status}'`); - return labelActions[LabelAction.None]; + const noneResult = { + ...baseResult, + labelActions: noneLabelActions, + }; + + const removeAutoSignedOffLabelsIfPresent = () => { + if (!hasAutoSignedOffLabels) { + return noneResult; } + + return { + ...noneResult, + labelActions: { + ...noneLabelActions, + [ArmAutoSignoffLabel.ArmSignedOff]: LabelAction.Remove, + [ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP]: LabelAction.Remove, + [ArmAutoSignoffLabel.ArmAutoSignedOffTrivialTest]: LabelAction.Remove, + }, + }; + }; + + // If workflow indicates auto-signoff should not be applied + if (!armAnalysisResult.qualifiesForAutoSignoff) { + return removeAutoSignedOffLabelsIfPresent(); } const allLabelsMatch = @@ -144,10 +160,11 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha, if (!allLabelsMatch) { core.info("Labels do not meet requirement for auto-signoff"); - return removeAction; + return removeAutoSignedOffLabelsIfPresent(); } // permissions: { statuses: read } + /** @type {CommitStatus[]} */ const statuses = await github.paginate(github.rest.repos.listCommitStatusesForRef, { owner: owner, repo: repo, @@ -162,14 +179,12 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha, const requiredStatusNames = ["Swagger LintDiff", "Swagger Avocado"]; - /** - * @type {typeof statuses} - */ + /** @type {CommitStatus[]} */ let requiredStatuses = []; for (const statusName of requiredStatusNames) { // The "statuses" array may contain multiple statuses with the same "context" (aka "name"), - // but different states and update times. We only care about the latest. + // but different states and update times. We only care about the latest. const matchingStatuses = statuses .filter((status) => status.context.toLowerCase() === statusName.toLowerCase()) .sort(invert(byDate((status) => status.updated_at))); @@ -185,7 +200,7 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha, matchingStatus.state === CommitStatusState.FAILURE) ) { core.info(`Status '${matchingStatus.context}' did not succeed`); - return removeAction; + return removeAutoSignedOffLabelsIfPresent(); } if (matchingStatus) { @@ -201,10 +216,138 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha, requiredStatuses.every((status) => status.state === CommitStatusState.SUCCESS) ) { core.info("All requirements met for auto-signoff"); - return labelActions[LabelAction.Add]; + const autoIncrementalTSP = armAnalysisResult.incrementalTypeSpec; + const autoTrivialTest = armAnalysisResult.isTrivial; + + // Add ARMSignOff label only when the PR is identified as an incremental typespec + // As the trivial changes sign-off is being released in test mode + const armSignOffAction = autoIncrementalTSP + ? LabelAction.Add + : hasAutoSignedOffLabels + ? LabelAction.Remove + : LabelAction.None; + const autoIncrementalTSPAction = autoIncrementalTSP ? LabelAction.Add : LabelAction.Remove; + const testTrivialAction = autoTrivialTest ? LabelAction.Add : LabelAction.Remove; + + // Keep labels in sync with current analysis results. + // When a label is not desired, emit Remove so it gets cleaned up if previously set. + return { + ...baseResult, + labelActions: { + ...noneLabelActions, + [ArmAutoSignoffLabel.ArmSignedOff]: armSignOffAction, + [ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP]: autoIncrementalTSPAction, + [ArmAutoSignoffLabel.ArmAutoSignedOffTrivialTest]: testTrivialAction, + }, + }; } // If any statuses are missing or pending, no-op to prevent frequent remove/add label as checks re-run core.info("One or more statuses are still pending"); - return labelActions[LabelAction.None]; + return noneResult; +} + +/** + * Check ARM Analysis workflow results (combines incremental TypeSpec and trivial changes). + * + * @param {WorkflowRun[]} workflowRuns + * @param {import('@actions/github-script').AsyncFunctionArguments['github']} github + * @param {string} owner + * @param {string} repo + * @param {import('@actions/github-script').AsyncFunctionArguments['core']} core + * @returns {Promise<{qualifiesForAutoSignoff: boolean, incrementalTypeSpec: boolean, isTrivial: boolean}>} + */ +async function checkArmAnalysisWorkflow(workflowRuns, github, owner, repo, core) { + const wfName = "ARM Auto SignOff - Analyze Code"; + const armAnalysisRuns = workflowRuns + .filter((wf) => wf.name == wfName) + // Sort by "updated_at" descending + .sort((a, b) => new Date(b.updated_at).getTime() - new Date(a.updated_at).getTime()); + + if (armAnalysisRuns.length == 0) { + core.info( + `Found no runs for workflow '${wfName}'. Assuming workflow trigger was skipped, which should be treated equal to "completed false".`, + ); + return { + qualifiesForAutoSignoff: false, + incrementalTypeSpec: false, + isTrivial: false, + }; + } + + // Sorted by "updated_at" descending, so most recent run is at index 0 + const run = armAnalysisRuns[0]; + + if (run.status != "completed") { + core.info(`Workflow '${wfName}' is still in-progress: status='${run.status}'`); + return { + qualifiesForAutoSignoff: false, + incrementalTypeSpec: false, + isTrivial: false, + }; + } + + if (run.conclusion != "success") { + core.info(`Run for workflow '${wfName}' did not succeed: '${run.conclusion}'`); + return { + qualifiesForAutoSignoff: false, + incrementalTypeSpec: false, + isTrivial: false, + }; + } + + // permissions: { actions: read } + /** @type {Artifact[]} */ + const artifacts = await github.paginate(github.rest.actions.listWorkflowRunArtifacts, { + owner, + repo, + run_id: run.id, + per_page: PER_PAGE_MAX, + }); + + /** @type {string[]} */ + const artifactNames = artifacts.map((a) => a.name); + core.info(`${wfName} artifactNames: ${JSON.stringify(artifactNames)}`); + + const incrementalTypeSpec = readBooleanArtifactValue(artifactNames, "incremental-typespec"); + const isTrivial = readBooleanArtifactValue(artifactNames, "trivial-changes"); + + // ARM-Auto-SignOff-Code uploads 2 distinct artifacts. + // If either artifact is missing (or invalid), fail closed: no auto-signoff. + if (incrementalTypeSpec === null || isTrivial === null) { + const missing = [ + incrementalTypeSpec === null ? "incremental-typespec" : null, + isTrivial === null ? "trivial-changes" : null, + ] + .filter(Boolean) + .join(", "); + + core.info(`Missing/invalid ARM analysis artifact(s): ${missing}`); + return { + qualifiesForAutoSignoff: false, + incrementalTypeSpec: false, + isTrivial: false, + }; + } + + core.info( + `ARM analysis results: incrementalTypeSpec=${incrementalTypeSpec}, isTrivial=${isTrivial}`, + ); + + const qualifiesForAutoSignoff = incrementalTypeSpec || isTrivial; + if (!qualifiesForAutoSignoff) { + core.info("PR does not qualify for auto sign-off based on ARM analysis"); + return { + qualifiesForAutoSignoff: false, + incrementalTypeSpec: false, + isTrivial: false, + }; + } + + core.info(`PR qualifies for auto sign-off based on ARM analysis.`); + return { + qualifiesForAutoSignoff: true, + incrementalTypeSpec: incrementalTypeSpec, + isTrivial: isTrivial, + }; } diff --git a/.github/workflows/src/arm-auto-signoff/arm-incremental-typespec.js b/.github/workflows/src/arm-auto-signoff/arm-incremental-typespec.js index f69cef97efb5..40e95fe53457 100644 --- a/.github/workflows/src/arm-auto-signoff/arm-incremental-typespec.js +++ b/.github/workflows/src/arm-auto-signoff/arm-incremental-typespec.js @@ -17,10 +17,10 @@ import { CoreLogger } from "../core-logger.js"; debug.enable("simple-git"); /** - * @param {import('@actions/github-script').AsyncFunctionArguments} AsyncFunctionArguments + * @param {import('@actions/github-script').AsyncFunctionArguments['core']} core * @returns {Promise} */ -export default async function incrementalTypeSpec({ core }) { +export async function incrementalTypeSpec(core) { const options = { cwd: process.env.GITHUB_WORKSPACE, paths: ["specification"], diff --git a/.github/workflows/src/arm-auto-signoff/pr-changes.js b/.github/workflows/src/arm-auto-signoff/pr-changes.js new file mode 100644 index 000000000000..b3c5784ac06c --- /dev/null +++ b/.github/workflows/src/arm-auto-signoff/pr-changes.js @@ -0,0 +1,53 @@ +/** + * Represents the types of changes present in a pull request. + */ +export class PullRequestChanges { + /** @type {boolean} */ + rmDocumentation = false; + + /** @type {boolean} */ + rmExamples = false; + + /** @type {boolean} */ + rmFunctional = false; + + /** @type {boolean} */ + rmOther = false; + + /** @type {boolean} */ + other = false; + + /** + * A PR is trivial if it contains only: + * - Documentation changes + * - Example changes + * and does NOT contain: + * - Functional spec changes + * - Other file types + * + * @returns {boolean} + */ + isTrivial() { + const hasNoBlockingChanges = !this.rmFunctional && !this.rmOther && !this.other; + const hasTrivialChanges = this.rmDocumentation || this.rmExamples; + return hasNoBlockingChanges && hasTrivialChanges; + } + + /** + * @returns {boolean} + */ + isDocumentationOnly() { + return ( + this.rmDocumentation && !this.rmExamples && !this.rmFunctional && !this.rmOther && !this.other + ); + } + + /** + * @returns {boolean} + */ + isExamplesOnly() { + return ( + !this.rmDocumentation && this.rmExamples && !this.rmFunctional && !this.rmOther && !this.other + ); + } +} diff --git a/.github/workflows/src/arm-auto-signoff/trivial-changes-check.js b/.github/workflows/src/arm-auto-signoff/trivial-changes-check.js new file mode 100644 index 000000000000..7c8214fd8dad --- /dev/null +++ b/.github/workflows/src/arm-auto-signoff/trivial-changes-check.js @@ -0,0 +1,435 @@ +// For now, treat all paths as posix, since this is the format returned from git commands +import debug from "debug"; +import { simpleGit } from "simple-git"; +import { inspect } from "util"; +import { + example, + getChangedFilesStatuses, + markdown, + resourceManager, + swagger, +} from "../../../shared/src/changed-files.js"; +import { CoreLogger } from "../core-logger.js"; +import { PullRequestChanges } from "./pr-changes.js"; + +/** @typedef {import("./pr-changes.js").PullRequestChanges} PullRequestChangesType */ + +// Enable simple-git debug logging to improve console output +debug.enable("simple-git"); + +/** + * Non-functional JSON property keys that are safe to change without affecting API behavior + */ +const NON_FUNCTIONAL_PROPERTIES = new Set([ + "description", + "title", + "summary", + "x-ms-summary", + "x-ms-description", + "externalDocs", + "x-ms-examples", + "x-example", + "example", + "examples", + "x-ms-client-name", + "tags", +]); + +/** + * Analyzes a PR to determine what types of changes it contains + * @param {import('@actions/github-script').AsyncFunctionArguments['core']} core + * @returns {Promise} Object with categorized change types + */ +export async function checkTrivialChanges(core) { + // Create result object once at the top + const changes = new PullRequestChanges(); + + // Compare the pull request merge commit (HEAD) against its first parent (HEAD^). + const options = { + cwd: process.env.GITHUB_WORKSPACE, + logger: new CoreLogger(core), + }; + + const changedFilesStatuses = await getChangedFilesStatuses(options); + const changedFiles = getFlattenedChangedFilesFromStatuses(changedFilesStatuses); + const git = simpleGit(options.cwd); + + // Filter to only resource-manager files for ARM auto-signoff + const changedRmFiles = changedFiles.filter(resourceManager); + const changedRmFilesStatuses = { + additions: changedFilesStatuses.additions.filter(resourceManager), + modifications: changedFilesStatuses.modifications.filter(resourceManager), + deletions: changedFilesStatuses.deletions.filter(resourceManager), + renames: changedFilesStatuses.renames.filter( + (r) => resourceManager(r.to) || resourceManager(r.from), + ), + }; + + // Check if PR contains non-resource-manager changes + const hasNonRmChanges = changedFiles.some((file) => !resourceManager(file)); + if (hasNonRmChanges) { + const nonRmFiles = changedFiles.filter((file) => !resourceManager(file)); + core.info( + `PR contains ${nonRmFiles.length} non-resource-manager changes - not eligible for ARM auto-signoff. Also, no further validation to resource manager files is performed.`, + ); + core.info( + `Non-RM files: ${nonRmFiles.slice(0, 5).join(", ")}${nonRmFiles.length > 5 ? ` ... and ${nonRmFiles.length - 5} more` : ""}`, + ); + // This workflow reports resource-manager scoped change categories. + // Mark as non-trivial/blocked because the PR touches files outside resource-manager. + changes.other = true; + core.info(`PR Changes: ${JSON.stringify(changes)}`); + core.info(`Is trivial: ${changes.isTrivial()}`); + return changes; + } + + // Early exit if no resource-manager changes + if (changedRmFiles.length === 0) { + core.info("No resource-manager changes detected in PR"); + core.info(`PR Changes: ${JSON.stringify(changes)}`); + core.info(`Is trivial: ${changes.isTrivial()}`); + return changes; + } + + // Check for significant file operations (additions, deletions, renames) + const hasSignificantFileOperationsInPr = hasSignificantFileOperations( + changedRmFilesStatuses, + core, + ); + + if (hasSignificantFileOperationsInPr) { + core.info("Significant file operations detected (new spec files, deletions, or renames)"); + // These are functional changes by policy + changes.rmFunctional = true; + core.info(`PR Changes: ${JSON.stringify(changes)}`); + core.info(`Is trivial: ${changes.isTrivial()}`); + return changes; + } + + // Analyze what types of changes are present and update the changes object + await analyzeAndUpdatePullRequestChanges(changedRmFiles, git, core, changes); + + core.info(`PR Changes: ${JSON.stringify(changes)}`); + core.info(`Is trivial: ${changes.isTrivial()}`); + + return changes; +} + +/** + * Checks for significant file operations: additions/deletions of spec files, or any renames + * @param {{additions: string[], modifications: string[], deletions: string[], renames: {from: string, to: string}[]}} changedFilesStatuses - File status information + * @param {import('@actions/github-script').AsyncFunctionArguments['core']} core - Core logger + * @returns {boolean} - True if significant operations detected + */ +function hasSignificantFileOperations(changedFilesStatuses, core) { + // New spec files (non-example JSON) are non-trivial + const newSpecFiles = changedFilesStatuses.additions.filter(swagger); + if (newSpecFiles.length > 0) { + core.info(`Significant: New spec files detected: ${newSpecFiles.join(", ")}`); + return true; + } + + // Deleted spec files (non-example JSON) are non-trivial + const deletedSpecFiles = changedFilesStatuses.deletions.filter(swagger); + if (deletedSpecFiles.length > 0) { + core.info(`Significant: Deleted spec files detected: ${deletedSpecFiles.join(", ")}`); + return true; + } + + // Any file renames/moves are non-trivial (conservative approach) + if (changedFilesStatuses.renames.length > 0) { + core.info( + `Significant: File renames detected: ${changedFilesStatuses.renames.map((r) => `${r.from} → ${r.to}`).join(", ")}`, + ); + return true; + } + + // Deletions of docs/examples are OK, but already filtered above + return false; +} + +/** + * Analyzes a PR to determine what types of changes it contains and updates the changes object + * @param {string[]} changedFiles - Array of changed file paths + * @param {import('simple-git').SimpleGit} git - Git instance + * @param {import('@actions/github-script').AsyncFunctionArguments['core']} core - Core logger + * @param {PullRequestChangesType} changes - Changes object to update + * @returns {Promise} + */ +async function analyzeAndUpdatePullRequestChanges(changedFiles, git, core, changes) { + // Categorize files by type + const documentationFiles = changedFiles.filter(markdown); + const exampleFiles = changedFiles.filter(example); + const specFiles = changedFiles.filter(swagger); + const otherFiles = changedFiles.filter( + (file) => !markdown(file) && !example(file) && !swagger(file), + ); + + core.info( + `File breakdown: ${documentationFiles.length} docs, ${exampleFiles.length} examples, ${specFiles.length} specs, ${otherFiles.length} other`, + ); + + // Set flags for file types present + if (documentationFiles.length > 0) { + changes.rmDocumentation = true; + } + + if (exampleFiles.length > 0) { + changes.rmExamples = true; + } + + if (otherFiles.length > 0) { + changes.rmOther = true; + } + + // Analyze spec files to determine if functional or non-functional + if (specFiles.length > 0) { + core.info(`Analyzing ${specFiles.length} spec files...`); + + for (const file of specFiles) { + core.info(`Analyzing file: ${file}`); + + try { + const hasFunctionalChanges = await hasFunctionalChangesInSpecFile(file, git, core); + + if (hasFunctionalChanges) { + changes.rmFunctional = true; + core.info(`File ${file} contains functional changes`); + return; + } + + core.info(`File ${file} contains only non-functional changes`); + } catch (error) { + core.warning(`Failed to analyze ${file}: ${inspect(error)}`); + // On error, treat as functional to be conservative + changes.rmFunctional = true; + return; + } + } + } +} + +/** + * Analyzes changes in a spec file to determine if they are functional. + * Note: New files and deletions should already be filtered by hasSignificantFileOperations. + * @param {string} file + * @param {import('simple-git').SimpleGit} git - Git instance + * @param {import('@actions/github-script').AsyncFunctionArguments['core']} core - Core logger + * @returns {Promise} True if changes are functional, false if only non-functional changes are detected + */ +async function hasFunctionalChangesInSpecFile(file, git, core) { + let baseContent, headContent; + + try { + // Get file content from base commit (merge commit parent) + baseContent = await git.show([`HEAD^:${file}`]); + } catch (e) { + if (e instanceof Error && e.message.includes("does not exist")) { + // New file - should have been caught earlier, but treat as functional to be safe + core.info(`File ${file} is new - treating as functional`); + return true; + } + throw e; + } + + try { + headContent = await git.show([`HEAD:${file}`]); + } catch (e) { + if (e instanceof Error && e.message.includes("does not exist")) { + // File deleted - should have been caught earlier, but treat as functional to be safe + core.info(`File ${file} was deleted - treating as functional`); + return true; + } + throw e; + } + + /** @type {unknown} */ + let baseJson; + + /** @type {unknown} */ + let headJson; + + try { + baseJson = /** @type {unknown} */ (JSON.parse(baseContent)); + headJson = /** @type {unknown} */ (JSON.parse(headContent)); + } catch (error) { + core.warning(`Failed to parse JSON for ${file}: ${inspect(error)}`); + // Be conservative: if we can't parse, assume functional changes. + return true; + } + + return hasFunctionalDifferences(baseJson, headJson, "", core); +} + +/** + * Creates a flat, de-duplicated list of changed files from the status results. + * Includes both rename endpoints (`from` and `to`) to ensure path-based filters work as expected. + * @param {{additions: string[], modifications: string[], deletions: string[], renames: {from: string, to: string}[]}} statuses + * @returns {string[]} + */ +function getFlattenedChangedFilesFromStatuses(statuses) { + /** @type {Set} */ + const seen = new Set(); + + /** @param {string} file */ + const add = (file) => { + if (typeof file !== "string" || file.length === 0) return; + seen.add(file); + }; + + for (const file of statuses.additions) add(file); + for (const file of statuses.modifications) add(file); + for (const file of statuses.deletions) add(file); + for (const rename of statuses.renames) { + add(rename.from); + add(rename.to); + } + + return Array.from(seen); +} + +/** + * Recursively analyzes differences between two JSON objects + * @param {unknown} baseObj - Base object + * @param {unknown} headObj - Head object + * @param {string} path - Current path in the object + * * @param {import('@actions/github-script').AsyncFunctionArguments['core']} core - Core logger + * @returns {boolean} - True if any functional differences are detected + */ +function hasFunctionalDifferences(baseObj, headObj, path, core) { + // If types differ, it's a functional change + if (typeof baseObj !== typeof headObj) { + core.info(`Type change at ${path || "root"}: ${typeof baseObj} -> ${typeof headObj}`); + return true; + } + + // Handle null values + if (baseObj === null || headObj === null) { + if (baseObj !== headObj) { + core.info(`Null value change at ${path || "root"}`); + return true; + } + return false; + } + + // Handle arrays + if (Array.isArray(baseObj) && Array.isArray(headObj)) { + // For arrays, we need to be careful about order and additions/removals + if (baseObj.length !== headObj.length) { + // Check if the change is just adding/removing non-functional properties + const arrayPath = path || "root"; + // For now, treat any array length change as functional unless we can prove otherwise + core.info(`Array length change at ${arrayPath}: ${baseObj.length} -> ${headObj.length}`); + + // Special case: if arrays contain only strings and changes are in non-functional properties like tags + return isFunctionalPath(path); + } + + // Check each element + for (let i = 0; i < baseObj.length; i++) { + if (hasFunctionalDifferences(baseObj[i], headObj[i], `${path}[${i}]`, core)) { + return true; + } + } + return false; + } + + // Handle objects + if ( + typeof baseObj === "object" && + baseObj !== null && + !Array.isArray(baseObj) && + typeof headObj === "object" && + headObj !== null && + !Array.isArray(headObj) + ) { + /** @type {Record} */ + const baseRecord = /** @type {Record} */ (baseObj); + + /** @type {Record} */ + const headRecord = /** @type {Record} */ (headObj); + + const baseKeys = new Set(Object.keys(baseRecord)); + const headKeys = new Set(Object.keys(headRecord)); + + // Check for added or removed keys + for (const key of baseKeys) { + if (!headKeys.has(key)) { + const fullPath = path ? `${path}.${key}` : key; + if (isFunctionalPath(fullPath)) { + core.info(`Property removed at ${fullPath} (functional)`); + return true; + } + core.info(`Property removed at ${fullPath} (non-functional)`); + } + } + + for (const key of headKeys) { + if (!baseKeys.has(key)) { + const fullPath = path ? `${path}.${key}` : key; + if (isFunctionalPath(fullPath)) { + core.info(`Property added at ${fullPath} (functional)`); + return true; + } + core.info(`Property added at ${fullPath} (non-functional)`); + } + } + + // Check changed properties + for (const key of baseKeys) { + if (headKeys.has(key)) { + const fullPath = path ? `${path}.${key}` : key; + const childHasFunctionalDifferences = hasFunctionalDifferences( + baseRecord[key], + headRecord[key], + fullPath, + core, + ); + + if (childHasFunctionalDifferences) { + // If the change is under a non-functional path, treat as non-functional. + if (isFunctionalPath(fullPath)) { + return true; + } + core.info(`Property changed at ${fullPath} (non-functional)`); + continue; + } + } + } + + return false; + } + + // Handle primitive values + if (baseObj !== headObj) { + if (isFunctionalPath(path)) { + core.info( + `Value changed at ${path || "root"} (functional): ${inspect(baseObj)} -> ${inspect(headObj)}`, + ); + return true; + } + core.info( + `Value changed at ${path || "root"} (non-functional): ${inspect(baseObj)} -> ${inspect(headObj)}`, + ); + return false; + } + return false; +} + +/** + * Returns true if the current path is under a functional property. + * Handles array indices (e.g. `tags[0]` -> `tags`). + * @param {string} path + * @returns {boolean} + */ +function isFunctionalPath(path) { + if (typeof path !== "string" || path.length === 0) return true; + + for (const segment of path.split(".")) { + if (segment.length === 0) continue; + const key = segment.split("[")[0]; + if (NON_FUNCTIONAL_PROPERTIES.has(key)) return false; + } + + return true; +} diff --git a/.github/workflows/test/arm-auto-signoff/arm-auto-signoff-status.test.js b/.github/workflows/test/arm-auto-signoff/arm-auto-signoff-status.test.js index 7cb86e27c324..e1857dd82539 100644 --- a/.github/workflows/test/arm-auto-signoff/arm-auto-signoff-status.test.js +++ b/.github/workflows/test/arm-auto-signoff/arm-auto-signoff-status.test.js @@ -6,11 +6,74 @@ import { createMockCore, createMockGithub as createMockGithubBase } from "../moc const core = createMockCore(); +const managedLabels = Object.freeze({ + armSignedOff: "ARMSignedOff", + autoSignedOffIncrementalTsp: "ARMAutoSignedOff-IncrementalTSP", + autoSignedOffTrivialTest: "ARMAutoSignedOff-Trivial-Test", +}); + +function createNoneLabelActions() { + return { + [managedLabels.armSignedOff]: LabelAction.None, + [managedLabels.autoSignedOffIncrementalTsp]: LabelAction.None, + [managedLabels.autoSignedOffTrivialTest]: LabelAction.None, + }; +} + +/** + * @param {string} headSha + * @param {number} issueNumber + */ +function createNoneResult(headSha, issueNumber) { + return { + headSha, + issueNumber, + labelActions: createNoneLabelActions(), + }; +} + /** - * @param {Object} param0 - * @param {boolean} param0.incrementalTypeSpec + * @param {string} headSha + * @param {number} issueNumber */ -function createMockGithub({ incrementalTypeSpec }) { +function createRemoveManagedLabelsResult(headSha, issueNumber) { + return { + headSha, + issueNumber, + labelActions: { + ...createNoneLabelActions(), + [managedLabels.armSignedOff]: LabelAction.Remove, + [managedLabels.autoSignedOffIncrementalTsp]: LabelAction.Remove, + [managedLabels.autoSignedOffTrivialTest]: LabelAction.Remove, + }, + }; +} + +/** + * @param {{ + * headSha: string, + * issueNumber: number, + * incrementalTypeSpec: boolean, + * isTrivial: boolean, + * }} params + */ +function createSuccessResult({ headSha, issueNumber, incrementalTypeSpec, isTrivial }) { + return { + headSha, + issueNumber, + labelActions: { + ...createNoneLabelActions(), + [managedLabels.armSignedOff]: incrementalTypeSpec ? LabelAction.Add : LabelAction.None, + [managedLabels.autoSignedOffIncrementalTsp]: incrementalTypeSpec + ? LabelAction.Add + : LabelAction.Remove, + [managedLabels.autoSignedOffTrivialTest]: isTrivial ? LabelAction.Add : LabelAction.Remove, + }, + }; +} + +/** @param {{ incrementalTypeSpec: boolean, isTrivial?: boolean }} param0 */ +function createMockGithub({ incrementalTypeSpec, isTrivial = false }) { const github = createMockGithubBase(); github.rest.actions.listWorkflowRunsForRepo.mockResolvedValue({ @@ -32,9 +95,19 @@ function createMockGithub({ incrementalTypeSpec }) { }, }); + // Analyze-code uploads 2 empty artifacts named `${name}=${value}`. + const incremental = incrementalTypeSpec ? "true" : "false"; + const trivial = isTrivial ? "true" : "false"; github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ data: { - artifacts: [{ name: `incremental-typespec=${incrementalTypeSpec}` }], + artifacts: [ + { + name: `incremental-typespec=${incremental}`, + }, + { + name: `trivial-changes=${trivial}`, + }, + ], }, }); @@ -48,7 +121,7 @@ describe("getLabelActionImpl", () => { ).rejects.toThrow(); }); - it("throws if no artifact from incremental typespec", async () => { + it("no-ops if no artifact from incremental typespec", async () => { const github = createMockGithub({ incrementalTypeSpec: false }); github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ data: { artifacts: [] }, @@ -63,7 +136,7 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).rejects.toThrow(); + ).resolves.toEqual(createNoneResult("abc123", 123)); }); it("no-ops if no current label ARMAutoSignedOff", async () => { @@ -81,13 +154,56 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.None, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createNoneResult("abc123", 123)); }); it("removes label if not incremental typespec", async () => { const github = createMockGithub({ incrementalTypeSpec: false }); github.rest.issues.listLabelsOnIssue.mockResolvedValue({ - data: [{ name: "ARMAutoSignedOff" }], + data: [{ name: "ARMAutoSignedOff-IncrementalTSP" }], + }); + + await expect( + getLabelActionImpl({ + owner: "TestOwner", + repo: "TestRepo", + issue_number: 123, + head_sha: "abc123", + github: github, + core: core, + }), + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); + }); + + it("removes ARMAutoSignedOff-Trivial-Test if analysis no longer trivial", async () => { + const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: false }); + github.rest.issues.listLabelsOnIssue.mockResolvedValue({ + data: [{ name: managedLabels.autoSignedOffTrivialTest }], + }); + + await expect( + getLabelActionImpl({ + owner: "TestOwner", + repo: "TestRepo", + issue_number: 123, + head_sha: "abc123", + github: github, + core: core, + }), + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); + }); + + it("removes label if analysis artifacts missing (fail closed)", async () => { + const github = createMockGithub({ incrementalTypeSpec: true }); + github.rest.issues.listLabelsOnIssue.mockResolvedValue({ + data: [{ name: "ARMAutoSignedOff-IncrementalTSP" }], + }); + + // Only one of the required artifacts is present. + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [{ name: "incremental-typespec=true" }], + }, }); await expect( @@ -99,7 +215,7 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.Remove, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); }); it("no-ops if incremental typespec in progress", async () => { @@ -126,13 +242,13 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.None, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createNoneResult("abc123", 123)); }); it("removes label if no runs of incremental typespec", async () => { const github = createMockGithubBase(); github.rest.issues.listLabelsOnIssue.mockResolvedValue({ - data: [{ name: "ARMAutoSignedOff" }], + data: [{ name: "ARMAutoSignedOff-IncrementalTSP" }], }); github.rest.actions.listWorkflowRunsForRepo.mockResolvedValue({ data: { @@ -149,13 +265,13 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.Remove, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); }); it("uses latest run of incremental typespec", async () => { const github = createMockGithubBase(); github.rest.issues.listLabelsOnIssue.mockResolvedValue({ - data: [{ name: "ARMAutoSignedOff" }], + data: [{ name: "ARMAutoSignedOff-IncrementalTSP" }], }); github.rest.actions.listWorkflowRunsForRepo.mockResolvedValue({ data: { @@ -187,13 +303,13 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.Remove, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); }); it.each([ - { labels: ["ARMAutoSignedOff"] }, - { labels: ["ARMAutoSignedOff", "ARMReview", "NotReadyForARMReview"] }, - { labels: ["ARMAutoSignedOff", "ARMReview", "SuppressionReviewRequired"] }, + { labels: ["ARMAutoSignedOff-IncrementalTSP"] }, + { labels: ["ARMAutoSignedOff-IncrementalTSP", "ARMReview", "NotReadyForARMReview"] }, + { labels: ["ARMAutoSignedOff-IncrementalTSP", "ARMReview", "SuppressionReviewRequired"] }, ])("removes label if not all labels match ($labels)", async ({ labels }) => { const github = createMockGithub({ incrementalTypeSpec: true }); @@ -210,7 +326,7 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.Remove, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); }); it.each(["Swagger Avocado", "Swagger LintDiff"])( @@ -219,7 +335,7 @@ describe("getLabelActionImpl", () => { const github = createMockGithub({ incrementalTypeSpec: true }); github.rest.issues.listLabelsOnIssue.mockResolvedValue({ - data: [{ name: "ARMAutoSignedOff" }, { name: "ARMReview" }], + data: [{ name: "ARMAutoSignedOff-IncrementalTSP" }, { name: "ARMReview" }], }); github.rest.repos.listCommitStatusesForRef.mockResolvedValue({ data: [ @@ -239,16 +355,33 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.Remove, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); }, ); it.each([ - [CommitStatusState.ERROR, ["ARMReview", "ARMAutoSignedOff"]], - [CommitStatusState.FAILURE, ["ARMReview", "ARMAutoSignedOff"]], - [CommitStatusState.PENDING, ["ARMReview"]], - [CommitStatusState.SUCCESS, ["ARMReview"]], - ])("uses latest status if multiple (%o)", async (state, labels) => { + [ + CommitStatusState.ERROR, + ["ARMReview", "ARMAutoSignedOff-IncrementalTSP"], + createRemoveManagedLabelsResult("abc123", 123), + ], + [ + CommitStatusState.FAILURE, + ["ARMReview", "ARMAutoSignedOff-IncrementalTSP"], + createRemoveManagedLabelsResult("abc123", 123), + ], + [CommitStatusState.PENDING, ["ARMReview"], createNoneResult("abc123", 123)], + [ + CommitStatusState.SUCCESS, + ["ARMReview"], + createSuccessResult({ + headSha: "abc123", + issueNumber: 123, + incrementalTypeSpec: true, + isTrivial: false, + }), + ], + ])("uses latest status if multiple (%o)", async (state, labels, expectedResult) => { const github = createMockGithub({ incrementalTypeSpec: true }); github.rest.issues.listLabelsOnIssue.mockResolvedValue({ @@ -277,20 +410,6 @@ describe("getLabelActionImpl", () => { ], }); - let expectedAction; - switch (state) { - case CommitStatusState.ERROR: - case CommitStatusState.FAILURE: - expectedAction = LabelAction.Remove; - break; - case CommitStatusState.SUCCESS: - expectedAction = LabelAction.Add; - break; - case CommitStatusState.PENDING: - expectedAction = LabelAction.None; - break; - } - await expect( getLabelActionImpl({ owner: "TestOwner", @@ -300,7 +419,7 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: expectedAction, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(expectedResult); }); it("no-ops if check not found or not completed", async () => { @@ -322,7 +441,7 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.None, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createNoneResult("abc123", 123)); github.rest.repos.listCommitStatusesForRef.mockResolvedValue({ data: [{ context: "Swagger LintDiff", state: CommitStatusState.PENDING }], @@ -336,7 +455,7 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.None, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual(createNoneResult("abc123", 123)); }); it("adds label if incremental tsp, labels match, and check succeeded", async () => { @@ -367,6 +486,78 @@ describe("getLabelActionImpl", () => { github: github, core: core, }), - ).resolves.toEqual({ labelAction: LabelAction.Add, headSha: "abc123", issueNumber: 123 }); + ).resolves.toEqual( + createSuccessResult({ + headSha: "abc123", + issueNumber: 123, + incrementalTypeSpec: true, + isTrivial: false, + }), + ); + }); + + it("adds trivial test label when PR qualifies only by trivial changes", async () => { + const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: true }); + + github.rest.issues.listLabelsOnIssue.mockResolvedValue({ + data: [{ name: "ARMReview" }], + }); + + github.rest.repos.listCommitStatusesForRef.mockResolvedValue({ + data: [ + { + context: "Swagger LintDiff", + state: CommitStatusState.SUCCESS, + }, + { + context: "Swagger Avocado", + state: CommitStatusState.SUCCESS, + }, + ], + }); + + await expect( + getLabelActionImpl({ + owner: "TestOwner", + repo: "TestRepo", + issue_number: 123, + head_sha: "abc123", + github: github, + core: core, + }), + ).resolves.toEqual( + createSuccessResult({ + headSha: "abc123", + issueNumber: 123, + incrementalTypeSpec: false, + isTrivial: true, + }), + ); + }); + + it("removes label if analysis artifacts have invalid boolean value (fail closed)", async () => { + const github = createMockGithub({ incrementalTypeSpec: true }); + + github.rest.issues.listLabelsOnIssue.mockResolvedValue({ + data: [{ name: "ARMAutoSignedOff-IncrementalTSP" }], + }); + + // Invalid boolean value should be treated as missing/invalid. + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [{ name: "incremental-typespec=maybe" }, { name: "trivial-changes=false" }], + }, + }); + + await expect( + getLabelActionImpl({ + owner: "TestOwner", + repo: "TestRepo", + issue_number: 123, + head_sha: "abc123", + github: github, + core: core, + }), + ).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123)); }); }); diff --git a/.github/workflows/test/arm-auto-signoff/arm-incremental-typespec.test.js b/.github/workflows/test/arm-auto-signoff/arm-incremental-typespec.test.js index 327d4b66955d..d2d16a05ef7f 100644 --- a/.github/workflows/test/arm-auto-signoff/arm-incremental-typespec.test.js +++ b/.github/workflows/test/arm-auto-signoff/arm-incremental-typespec.test.js @@ -2,11 +2,15 @@ import { relative, resolve } from "path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { repoRoot } from "../../../shared/test/repo.js"; -/** @type {import("vitest").MockedFunction} */ -const mockRaw = vi.hoisted(() => vi.fn().mockResolvedValue("")); +const mockRaw = vi.hoisted( + /** @returns {import("vitest").MockedFunction} */ + () => vi.fn().mockResolvedValue(""), +); -/** @type {import("vitest").MockedFunction} */ -const mockShow = vi.hoisted(() => vi.fn().mockResolvedValue("")); +const mockShow = vi.hoisted( + /** @returns {import("vitest").MockedFunction} */ + () => vi.fn().mockResolvedValue(""), +); vi.mock("simple-git", () => ({ simpleGit: vi.fn().mockReturnValue({ @@ -22,33 +26,24 @@ import { swaggerHandWritten, swaggerTypeSpecGenerated, } from "../../../shared/test/examples.js"; -import incrementalTypeSpecImpl from "../../src/arm-auto-signoff/arm-incremental-typespec.js"; +import { incrementalTypeSpec } from "../../src/arm-auto-signoff/arm-incremental-typespec.js"; import { createMockCore } from "../mocks.js"; const core = createMockCore(); -/** - * @param {unknown} asyncFunctionArgs - */ -function incrementalTypeSpec(asyncFunctionArgs) { - return incrementalTypeSpecImpl( - /** @type {import("@actions/github-script").AsyncFunctionArguments} */ (asyncFunctionArgs), - ); -} - describe("incrementalTypeSpec", () => { afterEach(() => { vi.clearAllMocks(); }); it("rejects if inputs null", async () => { - await expect(incrementalTypeSpec({})).rejects.toThrow(); + await expect(incrementalTypeSpec(/** @type {any} */ (undefined))).rejects.toThrow(); }); it("returns false if no changed RM files", async () => { vi.spyOn(changedFiles, "getChangedFiles").mockResolvedValue([]); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); }); it("returns false if a changed file is not typespec-generated", async () => { @@ -59,7 +54,7 @@ describe("incrementalTypeSpec", () => { const showSpy = vi.mocked(mockShow).mockResolvedValue(swaggerHandWritten); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); expect(showSpy).toBeCalledWith([`HEAD:${swaggerPath}`]); }); @@ -75,7 +70,7 @@ describe("incrementalTypeSpec", () => { // "git ls-tree" returns "" if the spec folder doesn't exist in the base branch const rawSpy = vi.mocked(mockRaw).mockResolvedValue(""); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); expect(showSpy).toBeCalledWith([`HEAD:${swaggerPath}`]); @@ -92,7 +87,7 @@ describe("incrementalTypeSpec", () => { .mocked(mockShow) .mockRejectedValue(new Error("path contoso.json does not exist in 'HEAD'")); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); expect(showSpy).toBeCalledWith([`HEAD:${swaggerPath}`]); }); @@ -106,7 +101,7 @@ describe("incrementalTypeSpec", () => { .mocked(mockShow) .mockRejectedValue(new Error("path readme.md does not exist in 'HEAD'")); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); expect(showSpy).toBeCalledWith([`HEAD:${readmePath}`]); }); @@ -118,7 +113,7 @@ describe("incrementalTypeSpec", () => { const showSpy = vi.mocked(mockShow).mockResolvedValue(""); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); expect(showSpy).toBeCalledWith([`HEAD:${readmePath}`]); }); @@ -131,7 +126,7 @@ describe("incrementalTypeSpec", () => { const showSpy = vi.mocked(mockShow).mockResolvedValue("not } valid { json"); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); expect(showSpy).toBeCalledWith([`HEAD:${swaggerPath}`]); }); @@ -169,7 +164,7 @@ describe("incrementalTypeSpec", () => { const lsTreeSpy = vi.mocked(mockRaw).mockResolvedValue(swaggerPath); - await expect(incrementalTypeSpec({ core })).resolves.toBe(false); + await expect(incrementalTypeSpec(core)).resolves.toBe(false); expect(showSpy).toHaveBeenCalledWith([`HEAD:${swaggerPath}`]); expect(showSpy).toHaveBeenCalledWith([`HEAD^:${swaggerPath}`]); @@ -185,7 +180,7 @@ describe("incrementalTypeSpec", () => { const showSpy = vi.mocked(mockShow).mockRejectedValue("string error"); - await expect(incrementalTypeSpec({ core })).rejects.toThrowError(); + await expect(incrementalTypeSpec(core)).rejects.toThrowError(); expect(showSpy).toBeCalledWith([`HEAD:${swaggerPath}`]); }); @@ -197,7 +192,7 @@ describe("incrementalTypeSpec", () => { const showSpy = vi.mocked(mockShow).mockRejectedValue("string error"); - await expect(incrementalTypeSpec({ core })).rejects.toThrowError(); + await expect(incrementalTypeSpec(core)).rejects.toThrowError(); expect(showSpy).toBeCalledWith([`HEAD:${readmePath}`]); }); @@ -212,7 +207,7 @@ describe("incrementalTypeSpec", () => { const lsTreeSpy = vi.mocked(mockRaw).mockResolvedValue(swaggerPath); - await expect(incrementalTypeSpec({ core })).resolves.toBe(true); + await expect(incrementalTypeSpec(core)).resolves.toBe(true); expect(showSpy).toBeCalledWith([`HEAD:${swaggerPath}`]); expect(showSpy).toBeCalledWith([`HEAD^:${swaggerPath}`]); @@ -263,7 +258,7 @@ describe("incrementalTypeSpec", () => { const lsTreeSpy = vi.mocked(mockRaw).mockResolvedValue(swaggerPath); - await expect(incrementalTypeSpec({ core })).resolves.toBe(true); + await expect(incrementalTypeSpec(core)).resolves.toBe(true); expect(showSpy).toBeCalledWith([`HEAD:${readmePath}`]); expect(showSpy).toBeCalledWith([`HEAD^:${swaggerPath}`]); @@ -289,7 +284,7 @@ describe("incrementalTypeSpec", () => { const lsTreeSpy = vi.mocked(mockRaw).mockResolvedValue(swaggerPath); - await expect(incrementalTypeSpec({ core })).resolves.toBe(true); + await expect(incrementalTypeSpec(core)).resolves.toBe(true); expect(showSpy).toBeCalledWith([`HEAD^:${swaggerPath}`]); diff --git a/.github/workflows/test/arm-auto-signoff/trivial-changes-check.test.js b/.github/workflows/test/arm-auto-signoff/trivial-changes-check.test.js new file mode 100644 index 000000000000..e9f81583d556 --- /dev/null +++ b/.github/workflows/test/arm-auto-signoff/trivial-changes-check.test.js @@ -0,0 +1,819 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import * as changedFiles from "../../../shared/src/changed-files.js"; +import { checkTrivialChanges } from "../../src/arm-auto-signoff/trivial-changes-check.js"; +import { createMockCore } from "../mocks.js"; + +/** @type {import("vitest").Mock<(args: string[]) => Promise>} */ +const mockShow = vi.hoisted(() => vi.fn().mockResolvedValue("")); + +vi.mock("simple-git", () => ({ + simpleGit: vi.fn().mockReturnValue({ + show: mockShow, + }), +})); + +const core = createMockCore(); + +/** @typedef {import("../../src/arm-auto-signoff/pr-changes.js").PullRequestChanges} PullRequestChanges */ + +/** + * The tests mock `simple-git` to return a minimal object with only `show`. + * Return that mock with a precise type so ESLint doesn't treat it as an unbound method. + * @returns {{ show: import("vitest").Mock<(args: string[]) => Promise> }} + */ +function getMockGit() { + return { show: mockShow }; +} + +describe("checkTrivialChanges", () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + it("returns empty change flags when no files are changed", async () => { + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: [], + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("marks PR as 'other' when non-resource-manager files are changed", async () => { + const mixedFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + "specification/someservice/data-plane/Service/stable/2021-01-01/service.json", // data-plane (non-RM) + "README.md", // root level file (non-RM) + ".github/workflows/ci.yml", // workflow file (non-RM) + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: mixedFiles, + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: true, + }); + }); + + it("marks PR as 'other' when only non-resource-manager files are changed", async () => { + const nonRmFiles = [ + "specification/someservice/data-plane/Service/stable/2021-01-01/service.json", + "package.json", + "tsconfig.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: nonRmFiles, + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: true, // Non-RM changes block ARM auto-signoff even when there are no RM changes + }); + }); + + it("detects functional changes when new spec files are added", async () => { + const filesWithAdditions = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/newservice.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: filesWithAdditions, // New spec file + modifications: [], + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, // New spec files are functional + rmOther: false, + other: false, + }); + }); + + it("detects functional changes when spec files are deleted", async () => { + const deletedFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/oldservice.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: [], + deletions: deletedFiles, // Deleted spec file + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, // Deleted spec files are functional + rmOther: false, + other: false, + }); + }); + + it("detects functional changes when spec files are renamed", async () => { + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: [], + deletions: [], + renames: [ + { + from: "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/oldname.json", + to: "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/newname.json", + }, + ], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, // Renamed spec files are functional + rmOther: false, + other: false, + }); + }); + + it("detects documentation-only changes when only .md files are modified", async () => { + const mdFiles = [ + "specification/someservice/resource-manager/readme.md", + "specification/someservice/resource-manager/Microsoft.Service/preview/2021-01-01/README.md", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: mdFiles, + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: true, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("detects examples-only changes when only example JSON files are modified", async () => { + const exampleFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/examples/Get.json", + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/examples/Create.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: exampleFiles, + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: true, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("marks rmOther when other resource-manager files are changed", async () => { + const rmOtherFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/readme.md", + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/notes.txt", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: rmOtherFiles, + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: true, + rmExamples: false, + rmFunctional: false, + rmOther: true, + other: false, + }); + }); + + it("identifies non-functional changes when only description and summary properties are modified", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + const oldJson = JSON.stringify({ + info: { title: "Service API", version: "1.0" }, + paths: { "/test": { get: { summary: "Test endpoint" } } }, + }); + + const newJson = JSON.stringify({ + info: { + title: "Service API", + version: "1.0", + description: "New description", // Non-functional change + }, + paths: { "/test": { get: { summary: "Test endpoint updated" } } }, // Non-functional change + }); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + const showSpy = getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: false, + }); + + expect(showSpy).toHaveBeenCalled(); + }); + + it("detects functional changes when new API endpoints are added", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + const oldJson = JSON.stringify({ + info: { title: "Service API", version: "1.0" }, + paths: { "/test": { get: { summary: "Test endpoint" } } }, + }); + + const newJson = JSON.stringify({ + info: { title: "Service API", version: "1.0" }, + paths: { + "/test": { get: { summary: "Test endpoint" } }, + "/new": { post: { summary: "New endpoint" } }, // Functional change + }, + }); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); + + it("detects functional changes even when mixed with documentation and example changes", async () => { + const mixedFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/README.md", // Documentation (trivial) + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/examples/Get.json", // Examples (trivial) + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", // Functional change (non-trivial) + ]; + + const oldJson = JSON.stringify({ + paths: { "/test": { get: { summary: "Test endpoint" } } }, + }); + + const newJson = JSON.stringify({ + paths: { + "/test": { get: { summary: "Test endpoint" } }, + "/new": { post: { summary: "New endpoint" } }, // Functional change + }, + }); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: mixedFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.includes("service.json")) { + if (ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + // Should detect functional changes, so overall not trivial + expect(result).toMatchObject({ + rmDocumentation: true, + rmExamples: true, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); + + it("treats JSON parsing errors as functional changes to be conservative", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve("{ invalid json"); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve("{ also invalid }"); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + // Should treat JSON parsing errors as non-trivial changes + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); + + it("treats git operation errors as functional changes to be conservative", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockRejectedValue(new Error("Git operation failed")); + + const result = await checkTrivialChanges(core); + // Should treat git errors as non-trivial changes + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); + + it("correctly identifies non-functional changes in nested object properties", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + const oldJson = JSON.stringify({ + info: { title: "Service API", version: "1.0" }, + paths: { + "/test": { + get: { + summary: "Test endpoint", + description: "Gets test data", + operationId: "Test_Get", + }, + }, + }, + }); + + const newJson = JSON.stringify({ + info: { + title: "Service API", + version: "1.0", + description: "API description", // Non-functional change + }, + paths: { + "/test": { + get: { + summary: "Updated test endpoint", // Non-functional change + description: "Gets test data", + operationId: "Test_Get", + }, + }, + }, + }); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("detects functional changes when parameters are added to API operations", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + const oldJson = JSON.stringify({ + info: { title: "Service API", version: "1.0" }, + paths: { + "/test": { + get: { + parameters: [{ name: "id", in: "path", required: true, type: "string" }], + }, + }, + }, + }); + + const newJson = JSON.stringify({ + info: { title: "Service API", version: "1.0" }, + paths: { + "/test": { + get: { + parameters: [ + { name: "id", in: "path", required: true, type: "string" }, + { name: "filter", in: "query", required: false, type: "string" }, // Functional change + ], + }, + }, + }, + }); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); + + it("correctly identifies multiple trivial change types together (documentation + examples)", async () => { + const mixedTrivialFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/README.md", + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/examples/Get.json", + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/examples/Create.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: mixedTrivialFiles, + deletions: [], + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: true, + rmExamples: true, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("ignores empty/invalid changed file entries", async () => { + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [""], + modifications: /** @type {any} */ ([null]), + deletions: /** @type {any} */ ([undefined]), + renames: [], + total: 0, + }); + + const result = await checkTrivialChanges(core); + + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("treats missing base content for a modified spec file as functional", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.reject(new Error("does not exist")); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(JSON.stringify({ openapi: "3.0.0", info: { title: "t" } })); + } + return Promise.reject(new Error("unexpected ref")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); + + it("treats missing head content for a modified spec file as functional", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(JSON.stringify({ openapi: "3.0.0", info: { title: "t" } })); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.reject(new Error("does not exist")); + } + return Promise.reject(new Error("unexpected ref")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); + + it("treats array length changes under non-functional properties (tags) as non-functional", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + const oldJson = JSON.stringify({ + openapi: "3.0.0", + info: { title: "Service API", version: "1.0" }, + tags: ["a"], + }); + + const newJson = JSON.stringify({ + openapi: "3.0.0", + info: { title: "Service API", version: "1.0" }, + tags: ["a", "b"], + }); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("treats type changes under non-functional properties (description) as non-functional", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + const oldJson = JSON.stringify({ + openapi: "3.0.0", + info: { title: "Service API", version: "1.0" }, + description: "old", + }); + + const newJson = JSON.stringify({ + openapi: "3.0.0", + info: { title: "Service API", version: "1.0" }, + description: 123, + }); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: false, + rmOther: false, + other: false, + }); + }); + + it("treats a root array length change as functional (conservative)", async () => { + const jsonFiles = [ + "specification/someservice/resource-manager/Microsoft.Service/stable/2021-01-01/service.json", + ]; + + const oldJson = JSON.stringify([]); + const newJson = JSON.stringify([1]); + + vi.spyOn(changedFiles, "getChangedFilesStatuses").mockResolvedValue({ + additions: [], + modifications: jsonFiles, + deletions: [], + renames: [], + total: 0, + }); + + getMockGit().show.mockImplementation((args) => { + const ref = args[0]; + if (typeof ref === "string" && ref.startsWith("HEAD^:")) { + return Promise.resolve(oldJson); + } + if (typeof ref === "string" && ref.startsWith("HEAD:")) { + return Promise.resolve(newJson); + } + return Promise.reject(new Error("does not exist")); + }); + + const result = await checkTrivialChanges(core); + expect(result).toMatchObject({ + rmDocumentation: false, + rmExamples: false, + rmFunctional: true, + rmOther: false, + other: false, + }); + }); +});