From e24823661f5fb0839b1c1142e7515afd6a0caa70 Mon Sep 17 00:00:00 2001 From: ray chen Date: Fri, 30 May 2025 17:13:31 +0000 Subject: [PATCH 1/4] enabled required for go and python data plane cases --- .../src/command-helpers.ts | 24 ++- eng/tools/spec-gen-sdk-runner/src/types.ts | 44 ++++- .../__snapshots__/commands.test.ts.snap | 0 .../test/{src => }/command-helpers.test.ts | 170 ++++++++++++------ .../test/{src => }/commands.test.ts | 16 +- .../test/{src => }/log.test.ts | 2 +- .../test/{src => }/spec-helpers.test.ts | 12 +- 7 files changed, 193 insertions(+), 75 deletions(-) rename eng/tools/spec-gen-sdk-runner/test/{src => }/__snapshots__/commands.test.ts.snap (100%) rename eng/tools/spec-gen-sdk-runner/test/{src => }/command-helpers.test.ts (81%) rename eng/tools/spec-gen-sdk-runner/test/{src => }/commands.test.ts (98%) rename eng/tools/spec-gen-sdk-runner/test/{src => }/log.test.ts (99%) rename eng/tools/spec-gen-sdk-runner/test/{src => }/spec-helpers.test.ts (98%) diff --git a/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts b/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts index 4b924ce3fc02..16cf7f94694b 100644 --- a/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts +++ b/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts @@ -305,13 +305,16 @@ export function generateArtifact( if (!fs.existsSync(specGenSdkArtifactAbsoluteFolder)) { fs.mkdirSync(specGenSdkArtifactAbsoluteFolder, { recursive: true }); } + const isSpecGenSdkCheckRequired = getRequiredSettingValue( + hasManagementPlaneSpecs, + commandInput.sdkLanguage as SdkName, + ); // Write artifact const artifactInfo: SpecGenSdkArtifactInfo = { language: commandInput.sdkLanguage, result, labelAction: hasBreakingChange, - isSpecGenSdkCheckRequired: - hasManagementPlaneSpecs && SpecGenSdkRequiredSettings[commandInput.sdkLanguage as SdkName], + isSpecGenSdkCheckRequired, apiViewRequestData: apiViewRequestData, }; fs.writeFileSync( @@ -348,3 +351,20 @@ export function getServiceFolderPath(specConfigPath: string): string { } return specConfigPath; } + +/** + * Get the required setting value for the SDK check based on the spec PR types. + * @param hasManagementPlaneSpecs - A flag indicating whether there are management plane specs. + * @param sdkName - The SDK name. + * @returns boolean indicating whether the SDK check is required. + */ +export function getRequiredSettingValue( + hasManagementPlaneSpecs: boolean, + sdkName: SdkName, +): boolean { + if (hasManagementPlaneSpecs) { + return SpecGenSdkRequiredSettings[sdkName].managementPlane; + } else { + return SpecGenSdkRequiredSettings[sdkName].dataPlane; + } +} diff --git a/eng/tools/spec-gen-sdk-runner/src/types.ts b/eng/tools/spec-gen-sdk-runner/src/types.ts index dc8841e8596f..9d1689a9a101 100644 --- a/eng/tools/spec-gen-sdk-runner/src/types.ts +++ b/eng/tools/spec-gen-sdk-runner/src/types.ts @@ -46,7 +46,7 @@ export interface SpecGenSdkArtifactInfo { result: string; labelAction?: boolean; isSpecGenSdkCheckRequired: boolean; - apiViewRequestData: APIViewRequestData []; + apiViewRequestData: APIViewRequestData[]; } /** @@ -59,13 +59,43 @@ export type SdkName = | "azure-sdk-for-net" | "azure-sdk-for-python"; +/** + * Represents the plane types for SDK generation settings + */ +export interface PlaneTypeSettings { + /** + * Whether spec-gen-sdk check is required for data plane + */ + dataPlane: boolean; + + /** + * Whether spec-gen-sdk check is required for management plane + */ + managementPlane: boolean; +} + /** * Required check settings for all languages. */ -export const SpecGenSdkRequiredSettings: Record = { - "azure-sdk-for-go": true, - "azure-sdk-for-java": true, - "azure-sdk-for-js": false, - "azure-sdk-for-net": false, - "azure-sdk-for-python": true, +export const SpecGenSdkRequiredSettings: Record = { + "azure-sdk-for-go": { + dataPlane: true, + managementPlane: true, + }, + "azure-sdk-for-java": { + dataPlane: false, + managementPlane: true, + }, + "azure-sdk-for-js": { + dataPlane: false, + managementPlane: false, + }, + "azure-sdk-for-net": { + dataPlane: false, + managementPlane: false, + }, + "azure-sdk-for-python": { + dataPlane: true, + managementPlane: true, + }, }; diff --git a/eng/tools/spec-gen-sdk-runner/test/src/__snapshots__/commands.test.ts.snap b/eng/tools/spec-gen-sdk-runner/test/__snapshots__/commands.test.ts.snap similarity index 100% rename from eng/tools/spec-gen-sdk-runner/test/src/__snapshots__/commands.test.ts.snap rename to eng/tools/spec-gen-sdk-runner/test/__snapshots__/commands.test.ts.snap diff --git a/eng/tools/spec-gen-sdk-runner/test/src/command-helpers.test.ts b/eng/tools/spec-gen-sdk-runner/test/command-helpers.test.ts similarity index 81% rename from eng/tools/spec-gen-sdk-runner/test/src/command-helpers.test.ts rename to eng/tools/spec-gen-sdk-runner/test/command-helpers.test.ts index e95b6e8bf534..15a44a8eb6b8 100644 --- a/eng/tools/spec-gen-sdk-runner/test/src/command-helpers.test.ts +++ b/eng/tools/spec-gen-sdk-runner/test/command-helpers.test.ts @@ -1,21 +1,22 @@ import { describe, test, expect, vi, beforeEach } from "vitest"; -import * as log from "../../src/log.js"; -import * as utils from "../../src/utils.js"; -import * as specHelpers from "../../src/spec-helpers.js"; +import * as log from "../src/log.js"; +import * as utils from "../src/utils.js"; +import * as specHelpers from "../src/spec-helpers.js"; import { fileURLToPath } from "node:url"; import fs from "node:fs"; import path from "node:path"; import { getBreakingChangeInfo, + getRequiredSettingValue, getSpecPaths, logIssuesToPipeline, parseArguments, prepareSpecGenSdkCommand, generateArtifact, setPipelineVariables, -} from "../../src/command-helpers.js"; -import { LogLevel } from "../../src/log.js"; -import { APIViewRequestData } from "../../src/types.js"; +} from "../src/command-helpers.js"; +import { LogLevel } from "../src/log.js"; +import { APIViewRequestData } from "../src/types.js"; // Get the absolute path to the repo root const currentFilePath = fileURLToPath(import.meta.url); @@ -39,10 +40,7 @@ describe("commands.ts", () => { "Configurations: 'specification/contosowidgetmanager/resource-manager/readme.md', and CommitSHA: 'commitsha', in SpecRepo: 'https://github.com/Azure/azure-rest-api-specs'", ); - expect(log.setVsoVariable).toHaveBeenCalledWith( - "StagedArtifactsFolder", - "path-to-artifact", - ); + expect(log.setVsoVariable).toHaveBeenCalledWith("StagedArtifactsFolder", "path-to-artifact"); expect(log.setVsoVariable).toHaveBeenCalledWith( "PrBranch", "sdkauto/sdk-security/keyvault/azcertificates", @@ -61,15 +59,10 @@ describe("commands.ts", () => { // mock implementation intentionally left blank }); - setPipelineVariables( - "path-to-artifact" - ); + setPipelineVariables("path-to-artifact"); expect(log.setVsoVariable).toHaveBeenCalledOnce(); - expect(log.setVsoVariable).toHaveBeenCalledWith( - "StagedArtifactsFolder", - "path-to-artifact", - ); + expect(log.setVsoVariable).toHaveBeenCalledWith("StagedArtifactsFolder", "path-to-artifact"); }); }); @@ -180,14 +173,18 @@ describe("commands.ts", () => { { tspconfigPath: "typespec1", readmePath: undefined }, { tspconfigPath: "typespec2", readmePath: undefined }, { tspconfigPath: undefined, readmePath: "readme1" }, - { tspconfigPath: undefined, readmePath: "readme2" } + { tspconfigPath: undefined, readmePath: "readme2" }, ]); const result = getSpecPaths("all-specs", "/spec/path"); - + expect(utils.getAllTypeSpecPaths).toHaveBeenCalledWith("/spec/path"); expect(utils.findReadmeFiles).toHaveBeenCalledWith(path.join("/spec/path", "specification")); - expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith(["typespec1", "typespec2"], ["readme1", "readme2"], false); + expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith( + ["typespec1", "typespec2"], + ["readme1", "readme2"], + false, + ); expect(result).toHaveLength(4); }); @@ -195,13 +192,17 @@ describe("commands.ts", () => { vi.spyOn(utils, "findReadmeFiles").mockReturnValue(["readme1", "readme2"]); vi.spyOn(specHelpers, "groupSpecConfigPaths").mockReturnValue([ { tspconfigPath: undefined, readmePath: "readme1" }, - { tspconfigPath: undefined, readmePath: "readme2" } + { tspconfigPath: undefined, readmePath: "readme2" }, ]); const result = getSpecPaths("all-openapis", "/spec/path"); - + expect(utils.findReadmeFiles).toHaveBeenCalledWith(path.join("/spec/path", "specification")); - expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith([], ["readme1", "readme2"], false); + expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith( + [], + ["readme1", "readme2"], + false, + ); expect(result).toHaveLength(2); }); @@ -212,70 +213,108 @@ describe("commands.ts", () => { { tspconfigPath: "typespec1", readmePath: undefined }, { tspconfigPath: "typespec2", readmePath: undefined }, { tspconfigPath: undefined, readmePath: "readme1" }, - { tspconfigPath: undefined, readmePath: "readme2" } + { tspconfigPath: undefined, readmePath: "readme2" }, ]); const result = getSpecPaths("all-typespecs", "/spec/path"); - + expect(utils.getAllTypeSpecPaths).toHaveBeenCalledWith("/spec/path"); expect(utils.findReadmeFiles).toHaveBeenCalledWith(path.join("/spec/path", "specification")); - expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith(["typespec1", "typespec2"], ["readme1", "readme2"], true); + expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith( + ["typespec1", "typespec2"], + ["readme1", "readme2"], + true, + ); expect(result).toHaveLength(4); }); test("should return sample TypeSpec paths for 'sample-typespecs' batch type", () => { vi.spyOn(utils, "getAllTypeSpecPaths").mockReturnValue(["typespec1", "typespec2"]); vi.spyOn(specHelpers, "groupSpecConfigPaths").mockReturnValue([ - { tspconfigPath: "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", readmePath: undefined }, - { tspconfigPath: "specification/contosowidgetmanager/Contoso.WidgetManager/tspconfig.yaml", readmePath: undefined } + { + tspconfigPath: "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", + readmePath: undefined, + }, + { + tspconfigPath: "specification/contosowidgetmanager/Contoso.WidgetManager/tspconfig.yaml", + readmePath: undefined, + }, ]); const result = getSpecPaths("sample-typespecs", "/spec/path"); - - expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith([ - "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", - "specification/contosowidgetmanager/Contoso.WidgetManager/tspconfig.yaml" - ], [], false); + + expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith( + [ + "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", + "specification/contosowidgetmanager/Contoso.WidgetManager/tspconfig.yaml", + ], + [], + false, + ); expect(result).toHaveLength(2); }); - + test("should return management plane TypeSpec and resource-manager readme paths for 'all-mgmtplane-typespecs'", () => { const managementTypespecs = ["typespec1.Management", "typespec2.Management"]; const resourceManagerReadmes = ["resource-manager/readme-rm1", "resource-manager/readme-rm2"]; - - vi.spyOn(utils, "getAllTypeSpecPaths").mockReturnValue([...managementTypespecs, "typespec3", "typespec4"]); - vi.spyOn(utils, "findReadmeFiles").mockReturnValue([...resourceManagerReadmes, "readme-dp1", "readme-dp2"]); + + vi.spyOn(utils, "getAllTypeSpecPaths").mockReturnValue([ + ...managementTypespecs, + "typespec3", + "typespec4", + ]); + vi.spyOn(utils, "findReadmeFiles").mockReturnValue([ + ...resourceManagerReadmes, + "readme-dp1", + "readme-dp2", + ]); vi.spyOn(specHelpers, "groupSpecConfigPaths").mockReturnValue([ { tspconfigPath: "typespec1.Management", readmePath: "resource-manager/readme-rm1" }, { tspconfigPath: "typespec2.Management", readmePath: "resource-manager/readme-rm2" }, ]); const result = getSpecPaths("all-mgmtplane-typespecs", "/spec/path"); - + expect(utils.getAllTypeSpecPaths).toHaveBeenCalledWith("/spec/path"); expect(utils.findReadmeFiles).toHaveBeenCalledWith(path.join("/spec/path", "specification")); - expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith(managementTypespecs, resourceManagerReadmes, true); + expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith( + managementTypespecs, + resourceManagerReadmes, + true, + ); expect(result).toHaveLength(2); }); - + test("should return data plane TypeSpec and data-plane readme paths for 'all-dataplane-typespecs'", () => { const dataPlaneTypespecs = ["typespec3", "typespec4"]; const dataPlaneReadmes = ["data-plane/readme-dp1", "data-plane/readme-dp2"]; - - vi.spyOn(utils, "getAllTypeSpecPaths").mockReturnValue([...dataPlaneTypespecs, "typespec1.Management", "typespec2.Management"]); - vi.spyOn(utils, "findReadmeFiles").mockReturnValue([...dataPlaneReadmes, "readme-rm1", "readme-rm2"]); + + vi.spyOn(utils, "getAllTypeSpecPaths").mockReturnValue([ + ...dataPlaneTypespecs, + "typespec1.Management", + "typespec2.Management", + ]); + vi.spyOn(utils, "findReadmeFiles").mockReturnValue([ + ...dataPlaneReadmes, + "readme-rm1", + "readme-rm2", + ]); vi.spyOn(specHelpers, "groupSpecConfigPaths").mockReturnValue([ { tspconfigPath: "typespec3", readmePath: undefined }, { tspconfigPath: "typespec4", readmePath: undefined }, { tspconfigPath: undefined, readmePath: "data-plane/readme-dp1" }, - { tspconfigPath: undefined, readmePath: "data-plane/readme-dp2" } + { tspconfigPath: undefined, readmePath: "data-plane/readme-dp2" }, ]); const result = getSpecPaths("all-dataplane-typespecs", "/spec/path"); - + expect(utils.getAllTypeSpecPaths).toHaveBeenCalledWith("/spec/path"); expect(utils.findReadmeFiles).toHaveBeenCalledWith(path.join("/spec/path", "specification")); - expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith(dataPlaneTypespecs, dataPlaneReadmes, true); + expect(specHelpers.groupSpecConfigPaths).toHaveBeenCalledWith( + dataPlaneTypespecs, + dataPlaneReadmes, + true, + ); expect(result).toHaveLength(4); }); }); @@ -373,9 +412,12 @@ describe("commands.ts", () => { // mock implementation intentionally left blank }); + // No need to mock getRequiredSettingValue for this test + // We'll just verify the output structure instead + const mockCommandInput = { workingFolder: "/working/folder", - sdkLanguage: "javascript", + sdkLanguage: "azure-sdk-for-js", runMode: "", localSpecRepoPath: "", localSdkRepoPath: "", @@ -388,7 +430,7 @@ describe("commands.ts", () => { const mockhasBreakingChange = false; const mockhasManagementPlaneSpecs = false; const mockStagedArtifactsFolder = "mockStagedArtifactsFolder"; - const mockApiViewRequestData: APIViewRequestData [] = []; + const mockApiViewRequestData: APIViewRequestData[] = []; const result = generateArtifact( mockCommandInput, mockResult, @@ -396,7 +438,7 @@ describe("commands.ts", () => { mockhasBreakingChange, mockhasManagementPlaneSpecs, mockStagedArtifactsFolder, - mockApiViewRequestData + mockApiViewRequestData, ); const breakingChangeLabelArtifactPath = path.normalize( @@ -407,15 +449,17 @@ describe("commands.ts", () => { expect(fs.mkdirSync).toHaveBeenCalledWith(breakingChangeLabelArtifactPath, { recursive: true, }); + // Since we're not mocking getRequiredSettingValue properly in this test, + // we'll just verify the output contains the expected isSpecGenSdkCheckRequired value expect(fs.writeFileSync).toHaveBeenCalledWith( path.join(breakingChangeLabelArtifactPath, "spec-gen-sdk-artifact.json"), JSON.stringify( { - language: "javascript", + language: "azure-sdk-for-js", result: "succeeded", labelAction: false, isSpecGenSdkCheckRequired: false, - apiViewRequestData: [] + apiViewRequestData: [], }, undefined, 2, @@ -466,7 +510,7 @@ describe("commands.ts", () => { const mockhasBreakingChange = false; const mockhasManagementPlaneSpecs = false; const mockStagedArtifactsFolder = ""; - const mockApiViewRequestData: APIViewRequestData [] = []; + const mockApiViewRequestData: APIViewRequestData[] = []; const result = generateArtifact( mockCommandInput, mockResult, @@ -481,4 +525,26 @@ describe("commands.ts", () => { expect(log.logMessage).toHaveBeenCalledWith("ending group logging", LogLevel.EndGroup); }); }); + + describe("getRequiredSettingValue", () => { + test("should return managementPlane setting when hasManagementPlaneSpecs is true", () => { + const result = getRequiredSettingValue(true, "azure-sdk-for-go"); + // Based on the constants in types.ts, Go SDK requires check for management plane + expect(result).toBe(true); + + const result2 = getRequiredSettingValue(true, "azure-sdk-for-js"); + // Based on the constants in types.ts, JS SDK does not require check for management plane + expect(result2).toBe(false); + }); + + test("should return dataPlane setting when hasManagementPlaneSpecs is false", () => { + const result = getRequiredSettingValue(false, "azure-sdk-for-go"); + // Based on the constants in types.ts, Go SDK requires check for data plane + expect(result).toBe(true); + + const result2 = getRequiredSettingValue(false, "azure-sdk-for-js"); + // Based on the constants in types.ts, JS SDK does not require check for data plane + expect(result2).toBe(false); + }); + }); }); diff --git a/eng/tools/spec-gen-sdk-runner/test/src/commands.test.ts b/eng/tools/spec-gen-sdk-runner/test/commands.test.ts similarity index 98% rename from eng/tools/spec-gen-sdk-runner/test/src/commands.test.ts rename to eng/tools/spec-gen-sdk-runner/test/commands.test.ts index 1851cb890d32..2ccf14432398 100644 --- a/eng/tools/spec-gen-sdk-runner/test/src/commands.test.ts +++ b/eng/tools/spec-gen-sdk-runner/test/commands.test.ts @@ -1,16 +1,16 @@ import { describe, test, expect, vi, beforeEach, type Mock } from "vitest"; -import * as utils from "../../src/utils.js"; +import * as utils from "../src/utils.js"; import { generateSdkForBatchSpecs, generateSdkForSingleSpec, generateSdkForSpecPr, -} from "../../src/commands.js"; -import * as commandHelpers from "../../src/command-helpers.js"; -import * as log from "../../src/log.js"; -import * as changeFiles from "../../src/spec-helpers.js"; +} from "../src/commands.js"; +import * as commandHelpers from "../src/command-helpers.js"; +import * as log from "../src/log.js"; +import * as changeFiles from "../src/spec-helpers.js"; import fs from "node:fs"; import path from "node:path"; -import { LogLevel } from "../../src/log.js"; +import { LogLevel } from "../src/log.js"; function getNormalizedFsCalls(mockFn: Mock): unknown[][] { return mockFn.mock.calls.map((args: unknown[]) => { @@ -219,7 +219,9 @@ describe("generateSdkForSpecPr", () => { }); const statusCode = await generateSdkForSpecPr(); - const serviceFolderPath = commandHelpers.getServiceFolderPath(mockChangedSpecs[0].typespecProject); + const serviceFolderPath = commandHelpers.getServiceFolderPath( + mockChangedSpecs[0].typespecProject, + ); expect(statusCode).toBe(0); expect(log.logMessage).toHaveBeenCalledWith( `Generating SDK from ${serviceFolderPath}`, diff --git a/eng/tools/spec-gen-sdk-runner/test/src/log.test.ts b/eng/tools/spec-gen-sdk-runner/test/log.test.ts similarity index 99% rename from eng/tools/spec-gen-sdk-runner/test/src/log.test.ts rename to eng/tools/spec-gen-sdk-runner/test/log.test.ts index b26d2992756d..60c3fb569262 100644 --- a/eng/tools/spec-gen-sdk-runner/test/src/log.test.ts +++ b/eng/tools/spec-gen-sdk-runner/test/log.test.ts @@ -6,7 +6,7 @@ import { vsoAddAttachment, vsoLogIssue, setVsoVariable, -} from "../../src/log.js"; +} from "../src/log.js"; const logSpy = vi.spyOn(console, "log").mockImplementation(() => { // mock implementation intentionally left blank diff --git a/eng/tools/spec-gen-sdk-runner/test/src/spec-helpers.test.ts b/eng/tools/spec-gen-sdk-runner/test/spec-helpers.test.ts similarity index 98% rename from eng/tools/spec-gen-sdk-runner/test/src/spec-helpers.test.ts rename to eng/tools/spec-gen-sdk-runner/test/spec-helpers.test.ts index c2d3253b25fc..9a419aff3680 100644 --- a/eng/tools/spec-gen-sdk-runner/test/src/spec-helpers.test.ts +++ b/eng/tools/spec-gen-sdk-runner/test/spec-helpers.test.ts @@ -1,6 +1,6 @@ import { describe, test, expect, vi, beforeEach } from "vitest"; -import { detectChangedSpecConfigFiles, groupSpecConfigPaths } from "../../src/spec-helpers.js"; -import { SpecGenSdkCmdInput } from "../../src/types.js"; +import { detectChangedSpecConfigFiles, groupSpecConfigPaths } from "../src/spec-helpers.js"; +import { SpecGenSdkCmdInput } from "../src/types.js"; import { fileURLToPath } from "node:url"; import path from "node:path"; import { @@ -8,10 +8,10 @@ import { type SpecConfigs, normalizePath, getChangedFiles, -} from "../../src/utils.js"; +} from "../src/utils.js"; -vi.mock("../../src/utils.js", async () => { - const actual = await vi.importActual("../../src/utils.js"); +vi.mock("../src/utils.js", async () => { + const actual = await vi.importActual("../src/utils.js"); return { ...actual, @@ -43,7 +43,7 @@ function normalizeSpecConfigsArray(configsArray: SpecConfigs[]): SpecConfigs[] { describe("detectChangedSpecConfigFiles", () => { const currentFilePath = fileURLToPath(import.meta.url); - const repoRoot = path.resolve(path.dirname(currentFilePath), "../fixtures/"); + const repoRoot = path.resolve(path.dirname(currentFilePath), "fixtures/"); const mockCommandInput: SpecGenSdkCmdInput = { localSpecRepoPath: repoRoot, From 7a05b6bce69db249d1d13af5d6512600d050e525 Mon Sep 17 00:00:00 2001 From: ray chen Date: Fri, 30 May 2025 20:39:27 +0000 Subject: [PATCH 2/4] support no-op in pipeline without spec changes --- eng/pipelines/spec-gen-sdk.yml | 5 +- .../src/command-helpers.ts | 16 +++- eng/tools/spec-gen-sdk-runner/src/commands.ts | 7 ++ .../spec-gen-sdk-runner/src/spec-helpers.ts | 4 + .../test/command-helpers.test.ts | 73 +++++++++++++++++++ .../spec-gen-sdk-runner/test/commands.test.ts | 65 ++++++++++++++++- .../test/spec-helpers.test.ts | 19 +++++ 7 files changed, 180 insertions(+), 9 deletions(-) diff --git a/eng/pipelines/spec-gen-sdk.yml b/eng/pipelines/spec-gen-sdk.yml index 8aa835d11d91..5f87e95a0baf 100644 --- a/eng/pipelines/spec-gen-sdk.yml +++ b/eng/pipelines/spec-gen-sdk.yml @@ -31,10 +31,7 @@ parameters: displayName: 'Skip SDK pull request creation' trigger: none -pr: - paths: - include: - - specification/** +pr: true extends: template: /eng/pipelines/templates/stages/archetype-spec-gen-sdk.yml diff --git a/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts b/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts index 16cf7f94694b..4a295e5c4cd6 100644 --- a/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts +++ b/eng/tools/spec-gen-sdk-runner/src/command-helpers.ts @@ -283,6 +283,9 @@ export function getBreakingChangeInfo(executionReport: any): [boolean, string] { * @param breakingChangeLabel - The breaking change label. * @param hasBreakingChange - A flag indicating whether there are breaking changes. * @param hasManagementPlaneSpecs - A flag indicating whether there are management plane specs. + * @param stagedArtifactsFolder - The staged artifacts folder. + * @param apiViewRequestData - The API view request data. + * @param sdkGenerationExecuted - A flag indicating whether the SDK generation was executed. * @returns the run status code. */ export function generateArtifact( @@ -293,6 +296,7 @@ export function generateArtifact( hasManagementPlaneSpecs: boolean, stagedArtifactsFolder: string, apiViewRequestData: APIViewRequestData[], + sdkGenerationExecuted: boolean = true, ): number { const specGenSdkArtifactName = "spec-gen-sdk-artifact"; const specGenSdkArtifactFileName = specGenSdkArtifactName + ".json"; @@ -305,10 +309,14 @@ export function generateArtifact( if (!fs.existsSync(specGenSdkArtifactAbsoluteFolder)) { fs.mkdirSync(specGenSdkArtifactAbsoluteFolder, { recursive: true }); } - const isSpecGenSdkCheckRequired = getRequiredSettingValue( - hasManagementPlaneSpecs, - commandInput.sdkLanguage as SdkName, - ); + let isSpecGenSdkCheckRequired = false; + if (sdkGenerationExecuted) { + isSpecGenSdkCheckRequired = getRequiredSettingValue( + hasManagementPlaneSpecs, + commandInput.sdkLanguage as SdkName, + ); + } + // Write artifact const artifactInfo: SpecGenSdkArtifactInfo = { language: commandInput.sdkLanguage, diff --git a/eng/tools/spec-gen-sdk-runner/src/commands.ts b/eng/tools/spec-gen-sdk-runner/src/commands.ts index e5323513a0ce..66a254c5b41f 100644 --- a/eng/tools/spec-gen-sdk-runner/src/commands.ts +++ b/eng/tools/spec-gen-sdk-runner/src/commands.ts @@ -90,11 +90,17 @@ export async function generateSdkForSpecPr(): Promise { let hasManagementPlaneSpecs = false; let overallRunHasBreakingChange = false; let currentRunHasBreakingChange = false; + let sdkGenerationExecuted = true; let overallExecutionResult = ""; let currentExecutionResult = ""; let stagedArtifactsFolder = ""; const apiViewRequestData: APIViewRequestData[] = []; + if (changedSpecs.length === 0) { + sdkGenerationExecuted = false; + overallExecutionResult = "succeeded"; + } + for (const changedSpec of changedSpecs) { if (!changedSpec.typespecProject && !changedSpec.readmeMd) { logMessage("Runner: no spec config file found in the changed files", LogLevel.Warn); @@ -179,6 +185,7 @@ export async function generateSdkForSpecPr(): Promise { hasManagementPlaneSpecs, stagedArtifactsFolder, apiViewRequestData, + sdkGenerationExecuted, ) || statusCode; return statusCode; } diff --git a/eng/tools/spec-gen-sdk-runner/src/spec-helpers.ts b/eng/tools/spec-gen-sdk-runner/src/spec-helpers.ts index 127a6872a361..8e2596800855 100644 --- a/eng/tools/spec-gen-sdk-runner/src/spec-helpers.ts +++ b/eng/tools/spec-gen-sdk-runner/src/spec-helpers.ts @@ -31,6 +31,10 @@ export function detectChangedSpecConfigFiles(commandInput: SpecGenSdkCmdInput): .filter((p) => p.startsWith("specification/")) .filter((p) => !p.includes("/scenarios/")); + if (fileList.length === 0) { + logMessage("No relevant files changed under 'specification' folder in the PR"); + return []; + } logMessage(`Related readme.md and typespec project list:`); const changedSpecs: ChangedSpecs[] = []; diff --git a/eng/tools/spec-gen-sdk-runner/test/command-helpers.test.ts b/eng/tools/spec-gen-sdk-runner/test/command-helpers.test.ts index 15a44a8eb6b8..c433ead24894 100644 --- a/eng/tools/spec-gen-sdk-runner/test/command-helpers.test.ts +++ b/eng/tools/spec-gen-sdk-runner/test/command-helpers.test.ts @@ -480,6 +480,7 @@ describe("commands.ts", () => { ); expect(log.setVsoVariable).toHaveBeenCalledWith("BreakingChangeLabelAction", "remove"); expect(log.setVsoVariable).toHaveBeenCalledWith("BreakingChangeLabel", "breaking-change"); + expect(log.setVsoVariable).toHaveBeenCalledWith("HasAPIViewArtifact", "false"); }); test("should handle errors during artifact generation", () => { @@ -524,6 +525,78 @@ describe("commands.ts", () => { expect(result).toBe(1); expect(log.logMessage).toHaveBeenCalledWith("ending group logging", LogLevel.EndGroup); }); + + test("should set isSpecGenSdkCheckRequired to false when sdkGenerationExecuted is false", () => { + vi.spyOn(fs, "existsSync").mockReturnValue(false); + vi.spyOn(fs, "mkdirSync").mockImplementation(() => undefined); + vi.spyOn(fs, "writeFileSync").mockImplementation(() => { + // mock implementation intentionally left blank + }); + vi.spyOn(log, "setVsoVariable").mockImplementation(() => { + // mock implementation intentionally left blank + }); + + // Mock getRequiredSettingValue to verify it's not called when sdkGenerationExecuted is false + const getRequiredSettingValueSpy = vi.spyOn( + { getRequiredSettingValue }, + "getRequiredSettingValue", + ); + + const mockCommandInput = { + workingFolder: "/working/folder", + sdkLanguage: "azure-sdk-for-go", + runMode: "", + localSpecRepoPath: "", + localSdkRepoPath: "", + sdkRepoName: "", + specCommitSha: "", + specRepoHttpsUrl: "", + }; + const mockResult = "succeeded"; + const mockBreakingchangeLabel = "breaking-change"; + const mockhasBreakingChange = false; + // Using true for hasManagementPlaneSpecs, which would normally make isSpecGenSdkCheckRequired=true + // for Go SDK (as tested in the getRequiredSettingValue tests) + const mockhasManagementPlaneSpecs = true; + const mockStagedArtifactsFolder = "mockStagedArtifactsFolder"; + const mockApiViewRequestData: APIViewRequestData[] = []; + + // Explicitly passing false for sdkGenerationExecuted + const result = generateArtifact( + mockCommandInput, + mockResult, + mockBreakingchangeLabel, + mockhasBreakingChange, + mockhasManagementPlaneSpecs, + mockStagedArtifactsFolder, + mockApiViewRequestData, + false, // sdkGenerationExecuted = false + ); + + const breakingChangeLabelArtifactPath = path.normalize( + "/working/folder/out/spec-gen-sdk-artifact", + ); + + expect(result).toBe(0); + // Verify getRequiredSettingValue was not called + expect(getRequiredSettingValueSpy).not.toHaveBeenCalled(); + + // Verify isSpecGenSdkCheckRequired is false in the written file + expect(fs.writeFileSync).toHaveBeenCalledWith( + path.join(breakingChangeLabelArtifactPath, "spec-gen-sdk-artifact.json"), + JSON.stringify( + { + language: "azure-sdk-for-go", + result: "succeeded", + labelAction: false, + isSpecGenSdkCheckRequired: false, // This should be false when sdkGenerationExecuted is false + apiViewRequestData: [], + }, + undefined, + 2, + ), + ); + }); }); describe("getRequiredSettingValue", () => { diff --git a/eng/tools/spec-gen-sdk-runner/test/commands.test.ts b/eng/tools/spec-gen-sdk-runner/test/commands.test.ts index 2ccf14432398..6301859d5e70 100644 --- a/eng/tools/spec-gen-sdk-runner/test/commands.test.ts +++ b/eng/tools/spec-gen-sdk-runner/test/commands.test.ts @@ -193,7 +193,7 @@ describe("generateSdkForSpecPr", () => { specs: [ "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/preview/2021-10-01-preview/examples/Employees_Get.json", ], - typespecProject: "specification/contosowidgetmanager/Contoso.WidgetManager/tspconfig.yaml", + typespecProject: "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", readmeMd: "specification/contosowidgetmanager/resource-manager/readme.md", }, ]; @@ -236,6 +236,59 @@ describe("generateSdkForSpecPr", () => { mockExecutionReport.vsoLogPath, serviceFolderPath, ); + expect(commandHelpers.generateArtifact).toHaveBeenCalledWith( + mockCommandInput, + "succeeded", // overallExecutionResult + "", // breakingChangeLabel + false, // overallRunHasBreakingChange + true, // hasManagementPlaneSpecs + "", // stagedArtifactsFolder + [], // apiViewRequestData + true, // sdkGenerationExecuted + ); + }); + + test("should handle the case when there are no changed specs", async () => { + // Set up mocks + const mockCommandInput = { + localSdkRepoPath: "path/to/local/repo", + localSpecRepoPath: "/spec/path", + workingFolder: "/working/folder", + runMode: "spec-pull-request", + sdkRepoName: "azure-sdk-for-js", + sdkLanguage: "javascript", + specCommitSha: "", + specRepoHttpsUrl: "", + }; + + // Return empty array for changedSpecs + vi.spyOn(changeFiles, "detectChangedSpecConfigFiles").mockReturnValue([]); + vi.spyOn(commandHelpers, "parseArguments").mockReturnValue(mockCommandInput); + vi.spyOn(commandHelpers, "prepareSpecGenSdkCommand").mockReturnValue(["mock-command"]); + + // Spy on generateArtifact to verify parameters + const generateArtifactSpy = vi.spyOn(commandHelpers, "generateArtifact").mockReturnValue(0); + + vi.spyOn(log, "logMessage").mockImplementation(() => { + // mock implementation intentionally left blank + }); + + const statusCode = await generateSdkForSpecPr(); + + expect(statusCode).toBe(0); + // Verify runSpecGenSdkCommand is not called when there are no changed specs + expect(utils.runSpecGenSdkCommand).not.toHaveBeenCalled(); + // Verify correct parameters are passed to generateArtifact + expect(generateArtifactSpy).toHaveBeenCalledWith( + mockCommandInput, + "succeeded", // overallExecutionResult should be set to "succeeded" + "", // breakingChangeLabel + false, // overallRunHasBreakingChange + false, // hasManagementPlaneSpecs + "", // stagedArtifactsFolder + [], // apiViewRequestData + false, // sdkGenerationExecuted should be set to false + ); }); test("should skip specs with no valid config files", async () => { @@ -265,6 +318,16 @@ describe("generateSdkForSpecPr", () => { "Runner: no spec config file found in the changed files", LogLevel.Warn, ); + expect(commandHelpers.generateArtifact).toHaveBeenCalledWith( + mockCommandInput, + "", // overallExecutionResult is empty because no spec was actually processed + "", // breakingChangeLabel + false, // overallRunHasBreakingChange + false, // hasManagementPlaneSpecs + "", // stagedArtifactsFolder + [], // apiViewRequestData + true, // sdkGenerationExecuted is true because there were some changed specs but they had no valid config + ); }); test("should handle errors during fail run runSpecGenSdkCommand for a changed spec", async () => { diff --git a/eng/tools/spec-gen-sdk-runner/test/spec-helpers.test.ts b/eng/tools/spec-gen-sdk-runner/test/spec-helpers.test.ts index 9a419aff3680..2eff1d38de36 100644 --- a/eng/tools/spec-gen-sdk-runner/test/spec-helpers.test.ts +++ b/eng/tools/spec-gen-sdk-runner/test/spec-helpers.test.ts @@ -66,6 +66,25 @@ describe("detectChangedSpecConfigFiles", () => { expect(result).toEqual([]); }); + test("case with changed files but none under specification folder", () => { + vi.mocked(getChangedFiles).mockReturnValue([ + "eng/tools/script1.js", + "documentation/README.md", + "profile/2020-09-01-hybrid.json", + ]); + const result = detectChangedSpecConfigFiles(mockCommandInput); + expect(result).toEqual([]); + }); + + test("case with changed files only under scenarios folder", () => { + vi.mocked(getChangedFiles).mockReturnValue([ + "specification/storage/scenarios/test1.json", + "specification/compute/scenarios/test2.json", + ]); + const result = detectChangedSpecConfigFiles(mockCommandInput); + expect(result).toEqual([]); + }); + test("case with readme files", () => { vi.mocked(getChangedFiles).mockReturnValue([ "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/preview/2021-10-01-preview/examples/Employees_Get.json", From 91c9c99cc76d369e21a5775b6d03af922906bb82 Mon Sep 17 00:00:00 2001 From: ray chen Date: Fri, 30 May 2025 21:16:02 +0000 Subject: [PATCH 3/4] include all branches --- eng/pipelines/spec-gen-sdk.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/eng/pipelines/spec-gen-sdk.yml b/eng/pipelines/spec-gen-sdk.yml index 5f87e95a0baf..567c47cfee67 100644 --- a/eng/pipelines/spec-gen-sdk.yml +++ b/eng/pipelines/spec-gen-sdk.yml @@ -31,7 +31,10 @@ parameters: displayName: 'Skip SDK pull request creation' trigger: none -pr: true +pr: + branches: + include: + - '*' extends: template: /eng/pipelines/templates/stages/archetype-spec-gen-sdk.yml From 2ee4f15b8a65251dee98f8d088a49fdc480ff64e Mon Sep 17 00:00:00 2001 From: ray chen Date: Tue, 3 Jun 2025 17:44:46 +0000 Subject: [PATCH 4/4] removed explicit settings for pr trigger --- eng/pipelines/spec-gen-sdk.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/eng/pipelines/spec-gen-sdk.yml b/eng/pipelines/spec-gen-sdk.yml index 567c47cfee67..0b5b5e849d6e 100644 --- a/eng/pipelines/spec-gen-sdk.yml +++ b/eng/pipelines/spec-gen-sdk.yml @@ -31,10 +31,6 @@ parameters: displayName: 'Skip SDK pull request creation' trigger: none -pr: - branches: - include: - - '*' extends: template: /eng/pipelines/templates/stages/archetype-spec-gen-sdk.yml