Compare PR deltas against the merge commit base

README.md

@@ -3,6 +3,7 @@
 Download and compile popular open source repos in order to compare new versions of the TypeScript compiler with the current version.
 For example, this project will clone the prettier repo and compile it with the current version of TypeScript.
 Then it will compile it with a version of TypeScript from a pull request.
+For pull requests, the baseline is the first parent of the PR merge commit, not the current `main` branch.
 Afterward it will compile new errors that are issued only with the new version and post them as a comment on the pull request.
 
 There is no comparison of types, errors, symbols or language service output.
@@ -25,6 +26,8 @@ node dist/checkReposTriggered.js <ts-entrypoint> <old-ts-repo-url> <old-head-ref
 
 ```
 
+The triggered command's `<old-head-ref>` argument is retained for compatibility but ignored; both builds use the same PR merge snapshot.
+
 You can view example usage of these commands from how they're currently triggered on Azure Pipelines:
 
 * [New Error Detector](https://github.com/microsoft/typescript-error-deltas/blob/main/azure-pipelines-gitTests.yml)

azure-pipelines-userTests.yml

@@ -20,7 +20,7 @@ parameters:
     type: string
     default: https://github.com/microsoft/TypeScript.git
   - name: OLD_HEAD_REF
-    displayName: Old head reference
+    displayName: Legacy baseline ref (ignored; PR baseline is the merge commit's first parent)
     type: string
     default: main
   - name: DISTINCT_ID

src/checkReposTriggered.ts

@@ -8,14 +8,13 @@ if (argv.length < 13) {
     process.exit(-1);
 }
 
-const [,, entrypoint, oldTsRepoUrl, oldHeadRef, prNumber, buildWithNewWhenOldFails, repoListPath, workerCount, workerNumber, resultDirName, diagnosticOutput, prngSeed, useOverlayFs, expectedHeadSha, expectedBaseSha, expectedMergeSha] = argv;
+const [,, entrypoint, oldTsRepoUrl, , prNumber, buildWithNewWhenOldFails, repoListPath, workerCount, workerNumber, resultDirName, diagnosticOutput, prngSeed, useOverlayFs, expectedHeadSha, expectedBaseSha, expectedMergeSha] = argv;
 
 mainAsync({
     testType: "triggered",
     useOverlayFs: useOverlayFs && useOverlayFs.toLowerCase() === "false" ? false : true,
     entrypoint: entrypoint as TsEntrypoint,
     oldTsRepoUrl,
-    oldHeadRef,
     prNumber: +prNumber,
     buildWithNewWhenOldFails: buildWithNewWhenOldFails.toLowerCase() !== "true",
     repoListPath,

src/main.ts

@@ -69,7 +69,6 @@ interface PrSnapshot {
 export interface TriggeredParams extends Params {
     testType: "triggered";
     oldTsRepoUrl: string;
-    oldHeadRef: string;
     prNumber: number;
     expectedPrSnapshot: PrSnapshot | undefined;
 }
@@ -1294,9 +1293,9 @@ async function downloadTsAsync(cwd: string, params: ScheduledParams | TriggeredP
         if (params.entrypoint === "fuzzer") {
             throw new Error("Not implemented");
         }
-        const { tsEntrypointPath: oldTsEntrypointPath, resolvedVersion: oldTsResolvedVersion, implementation: oldImplementation } = await downloadTsRepoAsync(cwd, params.oldTsRepoUrl, params.oldHeadRef, entrypoint);
-        // We need to handle the ref/pull/*/merge differently as it is not a branch and cannot be pulled during clone.
-        const { tsEntrypointPath: newTsEntrypointPath, resolvedVersion: newTsResolvedVersion, implementation } = await downloadTsPrAsync(cwd, params.oldTsRepoUrl, params.prNumber, entrypoint, params.expectedPrSnapshot);
+        const { baseline, candidate } = await downloadTsPrAsync(cwd, params.oldTsRepoUrl, params.prNumber, entrypoint, params.expectedPrSnapshot);
+        const { tsEntrypointPath: oldTsEntrypointPath, resolvedVersion: oldTsResolvedVersion, implementation: oldImplementation } = baseline;
+        const { tsEntrypointPath: newTsEntrypointPath, resolvedVersion: newTsResolvedVersion, implementation } = candidate;
 
         if (entrypoint === "tsserver" && oldImplementation !== implementation) {
             throw new Error("Cannot compare tsserver refs across the TypeScript-to-tsgo migration boundary");
@@ -1356,7 +1355,7 @@ export async function downloadTsRepoAsync(cwd: string, repoUrl: string, headRef:
     };
 }
 
-async function downloadTsPrAsync(cwd: string, repoUrl: string, prNumber: number, target: TsEntrypoint, expectedPrSnapshot: PrSnapshot | undefined): Promise<DownloadedTs> {
+export async function downloadTsPrAsync(cwd: string, repoUrl: string, prNumber: number, target: TsEntrypoint, expectedPrSnapshot: PrSnapshot | undefined): Promise<{ baseline: DownloadedTs, candidate: DownloadedTs }> {
     console.log(`Cloning ${repoUrl} at pull ${prNumber}`);
 
     const repoName = getTsRepoDownloadName(repoUrl, prNumber.toString());
@@ -1368,36 +1367,42 @@ async function downloadTsPrAsync(cwd: string, repoUrl: string, prNumber: number,
     const headRef = `refs/pull/${prNumber}/merge`;
 
     await git.checkout(repoPath, headRef);
-    await verifyPrSnapshot(repoPath, expectedPrSnapshot);
-    const { tsEntrypointPath, implementation } = await buildTs(repoPath, target);
+    const { baseSha } = await verifyPrSnapshot(repoPath, expectedPrSnapshot);
+    const basePath = path.resolve(cwd, `${repoName}-base`);
+    if (await pu.exists(basePath)) {
+        await execFileAsync(basePath, "git", ["checkout", "--detach", baseSha]);
+    }
+    else {
+        await execFileAsync(repoPath, "git", ["worktree", "add", "--detach", basePath, baseSha]);
+    }
+    await execFileAsync(basePath, "git", ["submodule", "update", "--init", "--recursive", "--depth=1"]);
 
+    const { tsEntrypointPath: oldTsEntrypointPath, implementation: oldImplementation } = await buildTs(basePath, target);
+    const { tsEntrypointPath: newTsEntrypointPath, implementation } = await buildTs(repoPath, target);
     return {
-        tsEntrypointPath,
-        resolvedVersion: headRef,
-        implementation
+        baseline: { tsEntrypointPath: oldTsEntrypointPath, resolvedVersion: baseSha, implementation: oldImplementation },
+        candidate: { tsEntrypointPath: newTsEntrypointPath, resolvedVersion: headRef, implementation },
     };
 }
 
-async function verifyPrSnapshot(repoPath: string, expected: PrSnapshot | undefined): Promise<void> {
-    if (!expected) {
-        return;
-    }
-    if (!expected.headSha || !expected.baseSha || !expected.mergeSha) {
+async function verifyPrSnapshot(repoPath: string, expected: PrSnapshot | undefined): Promise<{ baseSha: string }> {
+    if (expected && (!expected.headSha || !expected.baseSha || !expected.mergeSha)) {
         throw new Error("Expected PR snapshot must include head, base, and merge SHAs");
     }
 
     const [actualMergeSha, actualBaseSha, actualHeadSha] =
         (await execFileAsync(repoPath, "git", ["rev-parse", "HEAD", "HEAD^1", "HEAD^2"])).trim().split(/\s+/);
-    if (
+    if (expected && (
         actualMergeSha !== expected.mergeSha
         || actualBaseSha !== expected.baseSha
         || actualHeadSha !== expected.headSha
-    ) {
+    )) {
         throw new Error(
             `PR snapshot changed: expected merge ${expected.mergeSha}, base ${expected.baseSha}, head ${expected.headSha}; `
             + `got merge ${actualMergeSha}, base ${actualBaseSha}, head ${actualHeadSha}`,
         );
     }
+    return { baseSha: actualBaseSha };
 }
 
 export function detectTypeScriptImplementation(packageJson: { name?: string }): TypeScriptImplementation {

test/main.test.ts

@@ -1,11 +1,14 @@
-import { detectTypeScriptImplementation, detectTypeScriptNpmImplementation, downloadTsRepoAsync, mainAsync, reduceSpew } from '../src/main.js'
+import { detectTypeScriptImplementation, detectTypeScriptNpmImplementation, downloadTsPrAsync, downloadTsRepoAsync, mainAsync, reduceSpew } from '../src/main.js'
 import * as path from "node:path"
+import { execFileAsync } from '../src/utils/execUtils.js';
 import type { SpawnResult } from '../src/utils/execUtils.js';
 import { describe, expect, it, vi } from "vitest";
 
 const testState = vi.hoisted(() => ({
     typeScriptSpawnResult: undefined as ((args: readonly string[]) => SpawnResult) | undefined,
     writeFile: vi.fn(),
+    prRefs: undefined as string | undefined,
+    baseWorktreeExists: undefined as boolean | undefined,
 }));
 
 vi.mock('random-seed', () => ({
@@ -23,7 +26,9 @@ vi.mock("../src/utils/packageUtils", async () => {
     const fs = await vi.importActual<typeof import("node:fs")>("node:fs");
     return {
         exists: vi.fn((filePath: string) => filePath.includes("testDownloads")
-            ? fs.existsSync(filePath)
+            ? filePath.endsWith("typescript-123-base") && testState.baseWorktreeExists !== undefined
+                ? testState.baseWorktreeExists
+                : fs.existsSync(filePath)
             : Promise.resolve(true)),
         getMonorepoOrder: vi.fn().mockResolvedValue([
             "./dirA/package.json",
@@ -41,17 +46,17 @@ vi.mock("../src/utils/execUtils", () => ({
 
         return testState.typeScriptSpawnResult!(args);
     }),
-    execFileAsync: async (cwd: string, command: string, args: readonly string[] = []) => {
+    execFileAsync: vi.fn(async (cwd: string, command: string, args: readonly string[] = []) => {
         if (command === "npm" && args[0] === "pack" && args[1] === "typescript@latest") {
             return ' typescript-0.0.0.tgz';
         } else if (command === "npm" && args[0] === "pack" && args[1] === "typescript@next") {
             return ' typescript-1.1.1.tgz';
         } else if (command === "git" && args[0] === "rev-parse") {
-            return '57b462387e88aa7e363af0daf867a5dc1e83a935';
+            return testState.prRefs ?? '57b462387e88aa7e363af0daf867a5dc1e83a935';
         }
 
         return '';
-    },
+    }),
     execFileWithRetryAsync: vi.fn().mockResolvedValue(''),
 
 }));
@@ -158,6 +163,69 @@ describe("main", () => {
         }
     });
 
+    it.each([undefined, { mergeSha: 'merge', baseSha: 'base', headSha: 'head' }])(
+        "compares a PR merge with its first parent (expected snapshot: %s)",
+        async expectedPrSnapshot => {
+            const actualFs = await vi.importActual<typeof import("node:fs")>("node:fs");
+            const repoPath = path.join("./testDownloads/main", "typescript-123");
+            const basePath = path.resolve("./testDownloads/main", "typescript-123-base");
+            testState.prRefs = "merge\nbase\nhead\n";
+            testState.baseWorktreeExists = false;
+            for (const dir of [repoPath, basePath]) {
+                actualFs.mkdirSync(dir, { recursive: true });
+                actualFs.writeFileSync(path.join(dir, "package.json"), JSON.stringify({ name: "typescript" }));
+            }
+            vi.mocked(execFileAsync).mockClear();
+            try {
+                const result = await downloadTsPrAsync("./testDownloads/main", "https://github.com/microsoft/TypeScript", 123, "tsc", expectedPrSnapshot);
+                expect(result.baseline).toEqual({
+                    tsEntrypointPath: path.join(basePath, "built/local/tsc.js"),
+                    resolvedVersion: "base",
+                    implementation: "strada",
+                });
+                expect(result.candidate.tsEntrypointPath).toBe(path.join(repoPath, "built/local/tsc.js"));
+                expect(execFileAsync).toHaveBeenCalledWith(repoPath, "git", ["rev-parse", "HEAD", "HEAD^1", "HEAD^2"]);
+                expect(execFileAsync).toHaveBeenCalledWith(repoPath, "git", ["worktree", "add", "--detach", basePath, "base"]);
+                expect(execFileAsync).toHaveBeenCalledWith(basePath, "git", ["submodule", "update", "--init", "--recursive", "--depth=1"]);
+                expect(vi.mocked(execFileAsync).mock.calls.some(([, command, args]) => command === "git" && args?.includes("main"))).toBe(false);
+
+                testState.baseWorktreeExists = true;
+                testState.prRefs = "next-merge\nnext-base\nnext-head\n";
+                vi.mocked(execFileAsync).mockClear();
+                const next = await downloadTsPrAsync("./testDownloads/main", "https://github.com/microsoft/TypeScript", 123, "tsc", expectedPrSnapshot && {
+                    mergeSha: "next-merge", baseSha: "next-base", headSha: "next-head",
+                });
+                expect(next.baseline.resolvedVersion).toBe("next-base");
+                expect(execFileAsync).toHaveBeenCalledWith(basePath, "git", ["checkout", "--detach", "next-base"]);
+                expect(vi.mocked(execFileAsync).mock.calls.some(([, , args]) => args?.[0] === "worktree")).toBe(false);
+            }
+            finally {
+                testState.prRefs = undefined;
+                testState.baseWorktreeExists = undefined;
+                actualFs.rmSync(basePath, { recursive: true });
+                actualFs.rmSync(repoPath, { recursive: true });
+            }
+        },
+    );
+
+    it("rejects a changed PR snapshot before building either compiler", async () => {
+        const actualFs = await vi.importActual<typeof import("node:fs")>("node:fs");
+        const repoPath = "./testDownloads/main/typescript-123";
+        actualFs.mkdirSync(repoPath, { recursive: true });
+        testState.prRefs = "merge\nbase\nhead\n";
+        vi.mocked(execFileAsync).mockClear();
+        try {
+            await expect(downloadTsPrAsync("./testDownloads/main", "https://github.com/microsoft/TypeScript", 123, "tsc", {
+                mergeSha: "merge", baseSha: "different-base", headSha: "head",
+            })).rejects.toThrow("PR snapshot changed");
+            expect(vi.mocked(execFileAsync).mock.calls.some(([, , args]) => args?.[0] === "worktree" || args?.[0] === "ci")).toBe(false);
+        }
+        finally {
+            testState.prRefs = undefined;
+            actualFs.rmSync(repoPath, { recursive: true });
+        }
+    });
+
     it("outputs server errors", async () => {
         testState.typeScriptSpawnResult = () => ({
             stdout: errorStdout,