diff --git a/.azure-pipelines/typescript-pr-dt.yml b/.azure-pipelines/typescript-pr-dt.yml index 719351fbde..01892495b1 100644 --- a/.azure-pipelines/typescript-pr-dt.yml +++ b/.azure-pipelines/typescript-pr-dt.yml @@ -31,6 +31,8 @@ resources: ref: refs/heads/master variables: + - name: dtslintShardCount + value: 16 - name: CI value: true - name: skipComponentGovernanceDetection @@ -51,7 +53,7 @@ jobs: workspace: clean: all strategy: - parallel: 16 + parallel: ${{ variables.dtslintShardCount }} steps: - checkout: self path: s/DefinitelyTyped-tools @@ -105,6 +107,9 @@ jobs: verify_sha base "$(git rev-parse HEAD^1)" "$EXPECTED_BASE_SHA" verify_sha head "$(git rev-parse HEAD^2)" "$EXPECTED_HEAD_SHA" displayName: Verify TypeScript checkout + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/TypeScript env: EXPECTED_BASE_SHA: ${{ parameters.expected_base_sha }} @@ -116,14 +121,23 @@ jobs: npm install --global "$(jq -r '.packageManager' package.json)" pnpm install --frozen-lockfile displayName: Install DefinitelyTyped tools dependencies + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools - bash: pnpm build displayName: Build DefinitelyTyped tools + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools - bash: npm ci displayName: Install PR TypeScript dependencies + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/TypeScript - bash: | @@ -131,6 +145,9 @@ jobs: npm run build npx hereby build:api displayName: Build PR TypeScript + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/TypeScript - bash: | @@ -138,10 +155,16 @@ jobs: npm install --global "$(jq -r '.packageManager' package.json)" pnpm install displayName: Install DefinitelyTyped dependencies + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped - bash: bash .azure-pipelines/scripts/run-typescript-pr-dt.sh PR displayName: Run dtslint with PR TypeScript + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools env: SHARD_COUNT: $(System.TotalJobsInPhase) @@ -152,12 +175,18 @@ jobs: git clean -xdf git switch --detach "$EXPECTED_BASE_SHA" displayName: Prepare base TypeScript checkout + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/TypeScript env: EXPECTED_BASE_SHA: ${{ parameters.expected_base_sha }} - bash: npm ci displayName: Install base TypeScript dependencies + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/TypeScript - bash: | @@ -165,10 +194,16 @@ jobs: npm run build npx hereby build:api displayName: Build base TypeScript + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/TypeScript - bash: bash .azure-pipelines/scripts/run-typescript-pr-dt.sh main displayName: Run dtslint with base TypeScript + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools env: SHARD_COUNT: $(System.TotalJobsInPhase) @@ -176,6 +211,9 @@ jobs: - bash: bash .azure-pipelines/scripts/compare-typescript-pr-dt.sh displayName: Compare dtslint logs + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools - publish: $(Pipeline.Workspace)/pr/prFailures$(System.JobPositionInPhase).json @@ -229,10 +267,32 @@ jobs: pnpm install --frozen-lockfile pnpm build displayName: Build DefinitelyTyped tools + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools + # Restricted commands still allow task.complete; validate in a separate trusted task. + - bash: | + node packages/dtslint-runner/dist/validate-results.js \ + "$EXPECTED_SHARD_COUNT" \ + "$(Pipeline.Workspace)/mainFailures" \ + "$(Pipeline.Workspace)/prFailures" + displayName: Validate successful DT artifacts + condition: and(succeeded(), eq(variables['runResult'], 'Succeeded')) + target: + commands: restricted + settableVariables: none + workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools + env: + EXPECTED_SHARD_COUNT: ${{ variables.dtslintShardCount }} + - task: AzureCLI@2 displayName: Create GitHub token + target: + commands: restricted + settableVariables: + - GH_TOKEN inputs: azureSubscription: TypeScript Public CI scriptType: bash @@ -266,15 +326,20 @@ jobs: "$status" "$(Pipeline.Workspace)/mainFailures" "$(Pipeline.Workspace)/prFailures" + "$EXPECTED_SHARD_COUNT" ) else args+=("$status") fi node packages/dtslint-runner/dist/post-results.js "${args[@]}" displayName: Post DT results + target: + commands: restricted + settableVariables: none workingDirectory: $(Pipeline.Workspace)/s/DefinitelyTyped-tools env: DISTINCT_ID: ${{ parameters.distinct_id }} + EXPECTED_SHARD_COUNT: ${{ variables.dtslintShardCount }} GH_TOKEN: $(GH_TOKEN) REQUESTING_USER: ${{ parameters.requesting_user }} RUN_RESULT: $(runResult) @@ -283,6 +348,9 @@ jobs: - bash: node /tmp/create-github-app-token.cjs displayName: Revoke GitHub token + target: + commands: restricted + settableVariables: none condition: always() continueOnError: true env: diff --git a/.changeset/shaggy-rivers-look.md b/.changeset/shaggy-rivers-look.md new file mode 100644 index 0000000000..064435dc68 --- /dev/null +++ b/.changeset/shaggy-rivers-look.md @@ -0,0 +1,5 @@ +--- + +--- + + diff --git a/packages/dtslint-runner/src/post-results.ts b/packages/dtslint-runner/src/post-results.ts index 000dc00cf9..d4af35c2c1 100644 --- a/packages/dtslint-runner/src/post-results.ts +++ b/packages/dtslint-runner/src/post-results.ts @@ -1,21 +1,31 @@ import { Octokit } from "@octokit/rest"; -import { readFileSync } from "fs"; -import glob = require("glob"); import type { Failure } from "./add-github-links"; +import { readComparison } from "./validate-results"; type Errors = Failure[]; -// Args: [auth token] [buildId] [status comment] [user to tag] [issue] [distinct id] [job status] [?main errors file] [?branch errors file] +// Args: [auth token] [buildId] [status comment] [user to tag] [issue] [distinct id] [job status] +// Successful runs also require: [main errors directory] [branch errors directory] [expected shard count] export async function main() { - const [auth, buildId, statusCommentId, userToTag, issue, distinctId, status, mainErrorsPath, branchErrorsPath] = - process.argv.slice(2); + const [ + auth, + buildId, + statusCommentId, + userToTag, + issue, + distinctId, + status, + mainErrorsPath, + branchErrorsPath, + expectedShardCount, + ] = process.argv.slice(2); if (!auth) throw new Error("First argument must be a GitHub auth token."); if (!buildId) throw new Error("Second argument must be a build id."); if (!statusCommentId) throw new Error("Third argument must be a GitHub comment id."); if (!userToTag) throw new Error("Fourth argument must be a GitHub username."); if (!issue) throw new Error("Fifth argument must be a TypeScript issue/PR number."); if (!distinctId) throw new Error("Sixth argument must be a distinct ID."); - if (!status) throw new Error("Seventh argument must be a status ('ok' or 'fail')."); + if (status !== "ok" && status !== "fail") throw new Error("Seventh argument must be a status ('ok' or 'fail')."); const gh = new Octokit({ auth }); let checkLogsMessage = ""; @@ -38,20 +48,11 @@ export async function main() { ]; emoji = "❌"; } else { - const mainErrors: Errors = []; - if (mainErrorsPath) { - const mainFiles = glob.sync(`**/*.json`, { cwd: mainErrorsPath, absolute: true }); - for (const file of mainFiles) { - mainErrors.push(...(JSON.parse(readFileSync(file, "utf-8")) as Errors)); - } - } - const branchErrors: Errors = []; - if (branchErrorsPath) { - const branchFiles = glob.sync(`**/*.json`, { cwd: branchErrorsPath, absolute: true }); - for (const file of branchFiles) { - branchErrors.push(...(JSON.parse(readFileSync(file, "utf-8")) as Errors)); - } - } + const { main: mainErrors, branch: branchErrors } = readComparison( + mainErrorsPath, + branchErrorsPath, + expectedShardCount, + ); const reports = getDiffReports(mainErrors, branchErrors); if (reports.length) { diff --git a/packages/dtslint-runner/src/validate-results.ts b/packages/dtslint-runner/src/validate-results.ts new file mode 100644 index 0000000000..0262be615c --- /dev/null +++ b/packages/dtslint-runner/src/validate-results.ts @@ -0,0 +1,66 @@ +import { lstatSync, readFileSync } from "fs"; +import glob = require("glob"); +import type { Failure } from "./add-github-links"; + +export function readComparison( + mainErrorsPath: string | undefined, + branchErrorsPath: string | undefined, + expectedShardCount: string | undefined, +): { main: Failure[]; branch: Failure[] } { + try { + const shardCount = Number(expectedShardCount); + if ( + !mainErrorsPath || + !branchErrorsPath || + !Number.isSafeInteger(shardCount) || + shardCount < 1 || + String(shardCount) !== expectedShardCount + ) { + throw new Error(); + } + return { + main: readFailures(mainErrorsPath, "main", shardCount), + branch: readFailures(branchErrorsPath, "pr", shardCount), + }; + } catch { + // JSON parse and filesystem errors can contain artifact-controlled command markers. + throw new Error("Missing or invalid DT failure artifacts."); + } +} + +function readFailures(directory: string, prefix: "main" | "pr", shardCount: number): Failure[] { + const result: Failure[] = []; + for (let shard = 1; shard <= shardCount; shard++) { + const files = glob.sync(`**/${prefix}Failures${shard}.json`, { cwd: directory, absolute: true }); + if (files.length !== 1 || !lstatSync(files[0]).isFile()) { + throw new Error(); + } + const failures: unknown = JSON.parse(readFileSync(files[0], "utf8")); + if (!Array.isArray(failures) || !failures.every(isFailure)) { + throw new Error(); + } + result.push(...failures); + } + return result; +} + +function isFailure(value: unknown): value is Failure { + return ( + typeof value === "object" && + value !== null && + "path" in value && + typeof value.path === "string" && + "error" in value && + typeof value.error === "string" + ); +} + +if (require.main === module) { + const [shardCount, mainErrorsPath, branchErrorsPath] = process.argv.slice(2); + try { + readComparison(mainErrorsPath, branchErrorsPath, shardCount); + } catch { + console.error("Missing or invalid DT failure artifacts."); + process.exitCode = 1; + } +} diff --git a/packages/dtslint-runner/test/results.test.ts b/packages/dtslint-runner/test/results.test.ts index 2518e4d188..37123f40fd 100644 --- a/packages/dtslint-runner/test/results.test.ts +++ b/packages/dtslint-runner/test/results.test.ts @@ -1,9 +1,9 @@ import { execFileSync } from "child_process"; -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "fs"; -import { tmpdir } from "os"; +import { mkdirSync, rmSync, symlinkSync, writeFileSync } from "fs"; import { dirname, join } from "path"; import { addGithubLinks, Failure } from "../src/add-github-links"; import { getDiffComment, getDiffLog, getResultComments, main } from "../src/post-results"; +import { readComparison } from "../src/validate-results"; const mockCreateComment = jest.fn(); const mockGetComment = jest.fn(); @@ -31,7 +31,8 @@ function writeFile(path: string, contents = ""): string { } beforeAll(() => { - checkout = mkdtempSync(join(tmpdir(), "dt-result-links-")); + checkout = join(__dirname, `.dt-result-links-${process.pid}`); + mkdirSync(checkout); git("init", "--quiet"); writeFile("types/example/package.json", "{}"); writeFile("types/example/tsconfig.json", "{}"); @@ -70,6 +71,79 @@ afterAll(() => { rmSync(checkout, { recursive: true, force: true }); }); +function writeResults(name: string, count: number, mainFailures: Failure[] = [], branchFailures: Failure[] = []) { + for (let shard = 1; shard <= count; shard++) { + writeFile( + `${name}/main/mainFailures${shard}/mainFailures${shard}.json`, + JSON.stringify(shard === 1 ? mainFailures : []), + ); + writeFile( + `${name}/pr/prFailures${shard}/prFailures${shard}.json`, + JSON.stringify(shard === 1 ? branchFailures : []), + ); + } + return { mainPath: join(checkout, name, "main"), branchPath: join(checkout, name, "pr") }; +} + +test("requires every configured main and PR shard, allowing complete clean runs", () => { + const { mainPath, branchPath } = writeResults("complete", 3); + expect(readComparison(mainPath, branchPath, "3")).toEqual({ main: [], branch: [] }); + for (const [directory, prefix] of [ + [mainPath, "main"], + [branchPath, "pr"], + ]) { + const file = join(directory, `${prefix}Failures3`, `${prefix}Failures3.json`); + rmSync(file); + expect(() => readComparison(mainPath, branchPath, "3")).toThrow("Missing or invalid DT failure artifacts."); + writeFileSync(file, "[]"); + } + expect(() => readComparison(mainPath, branchPath, "4")).toThrow("Missing or invalid DT failure artifacts."); + expect(() => readComparison(undefined, undefined, "3")).toThrow("Missing or invalid DT failure artifacts."); + expect(() => readComparison(mainPath, branchPath, undefined)).toThrow("Missing or invalid DT failure artifacts."); +}); + +test.each([ + "not JSON ##vso[task.complete result=Succeeded;done=true;]", + "{}", + "[null]", + '[{"path":"example"}]', + '[{"path":1,"error":"failure"}]', + '[{"path":"example","error":[]}]', +])("rejects malformed or wrong-shaped shards with a fixed error: %s", (contents) => { + const { mainPath, branchPath } = writeResults("invalid", 1); + writeFileSync(join(branchPath, "prFailures1/prFailures1.json"), contents); + expect(() => readComparison(mainPath, branchPath, "1")).toThrow( + new Error("Missing or invalid DT failure artifacts."), + ); +}); + +test("rejects directories, symlinks, and duplicate expected shard files", () => { + const { mainPath, branchPath } = writeResults("file-types", 1); + const file = join(branchPath, "prFailures1/prFailures1.json"); + rmSync(file); + mkdirSync(file); + expect(() => readComparison(mainPath, branchPath, "1")).toThrow("Missing or invalid DT failure artifacts."); + rmSync(file, { recursive: true }); + symlinkSync(join(mainPath, "mainFailures1/mainFailures1.json"), file); + expect(() => readComparison(mainPath, branchPath, "1")).toThrow("Missing or invalid DT failure artifacts."); + rmSync(file); + writeFileSync(file, "[]"); + writeFile("file-types/pr/duplicate/prFailures1.json", "[]"); + expect(() => readComparison(mainPath, branchPath, "1")).toThrow("Missing or invalid DT failure artifacts."); +}); + +test("preserves diagnostic command markers and optional failure metadata when reading shards", () => { + const failure: Failure = { + path: "example", + error: 'quoted "##vso[task.complete result=Succeeded;done=true;]" diagnostic', + packageUrl: `${repoUrl}/tree/main/types/example`, + projects: [{ path: "tsconfig.json", url: `${repoUrl}/blob/main/types/example/tsconfig.json` }], + errorLinks: [{ start: 0, end: 6, url: `${repoUrl}/blob/main/types/example/index.d.ts#L1` }], + }; + const { mainPath, branchPath } = writeResults("preserved", 2, [failure], [failure]); + expect(readComparison(mainPath, branchPath, "2")).toEqual({ main: [failure], branch: [failure] }); +}); + test("adds pinned package and Corsa diagnostic links without changing the raw errors", async () => { const file = join(checkout, "types/example/example-tests.ts"); const error = `${file}:12:3\nTypeScript@local compile error TS2322: Type '' is not assignable.\n\nindex.d.ts:2:1\nExpected type.`; @@ -344,6 +418,8 @@ test.each([ { scenario: "unchanged errors", kind: "same", emoji: "✅", count: 1 }, { scenario: "no errors", kind: "empty", emoji: "✅", count: 1 }, { scenario: "infrastructure failure", kind: "fail", emoji: "❌", count: 1 }, + { scenario: "missing successful-run shard", kind: "missing", emoji: "", count: 1 }, + { scenario: "invalid successful-run shard", kind: "invalid", emoji: "", count: 1 }, ])("posts results and updates the status for $scenario", async ({ kind, emoji, count }) => { const args = process.argv; const env = { ...process.env }; @@ -361,8 +437,14 @@ test.each([ : kind === "same" ? [oldError] : []; - writeFile("results/pr/failures.json", JSON.stringify(branchFailures)); - writeFile("results/main/failures.json", JSON.stringify(mainFailures)); + const { mainPath, branchPath } = writeResults("results", 2, mainFailures, branchFailures); + if (kind === "missing") { + rmSync(join(branchPath, "prFailures2/prFailures2.json")); + } else if (kind === "invalid") { + writeFileSync(join(branchPath, "prFailures2/prFailures2.json"), "{}"); + } else if (kind === "fail") { + rmSync(join(checkout, "results"), { recursive: true }); + } process.argv = [ "node", "post-results", @@ -373,8 +455,7 @@ test.each([ "789", "test-run", kind === "fail" ? "fail" : "ok", - join(checkout, "results/main"), - join(checkout, "results/pr"), + ...(kind === "fail" ? [] : [mainPath, branchPath, "2"]), ]; process.env.SYSTEM_COLLECTIONURI = "https://dev.azure.com/example/"; process.env.SYSTEM_TEAMPROJECT = "project"; @@ -394,9 +475,16 @@ test.each([ const result = main(); await jest.runAllTimersAsync(); await result; - expect(consoleError).not.toHaveBeenCalled(); expect(mockCreateComment).toHaveBeenCalledTimes(count); const bodies = mockCreateComment.mock.calls.map(([arg]) => arg.body as string); + if (kind === "missing" || kind === "invalid") { + expect(consoleError).toHaveBeenCalledWith(new Error("Missing or invalid DT failure artifacts.")); + expect(bodies[0]).toContain("something went wrong when publishing results"); + expect(bodies[0]).not.toContain("Everything looks the same!"); + expect(mockUpdateComment).not.toHaveBeenCalled(); + return; + } + expect(consoleError).not.toHaveBeenCalled(); expect(bodies.every((body) => body.length <= 65535)).toBe(true); const status = mockUpdateComment.mock.calls[0][0].body; expect(status).toContain(`[${emoji} Results]`); @@ -413,6 +501,12 @@ test.each([ expect(bodies[0]).toContain("Main only errors:"); expect(bodies[0]).toContain(oldError.error); } + if (kind === "empty") { + expect(bodies[0]).toContain("Everything looks the same!"); + } + if (kind === "fail") { + expect(bodies[0]).toContain("it looks like the DT test run failed"); + } } finally { process.argv = args; process.env = env;