Merge pull request #105 from armanio123/GroupErrors

Group concurrent errors

Author: Armando Aguirre

src/main.ts

@@ -9,6 +9,7 @@ import fs = require("fs");
 import path = require("path");
 import mdEscape = require("markdown-escape");
 import randomSeed = require("random-seed");
+import { getErrorMessageFromStack, getHash, getHashForStack } from "./utils/hashStackTrace";
 
 interface Params {
     /**
@@ -84,9 +85,31 @@ export type RepoStatus =
     | "Detected no interesting changes"
     ;
 
+interface TSServerResult {
+    oldServerFailed: boolean;
+    oldSpawnResult?: SpawnResult;
+    newServerFailed: boolean;
+    newSpawnResult: SpawnResult;
+    replayScriptPath: string;
+    installCommands: ip.InstallCommand[];
+}
+
+interface Summary {
+    tsServerResult: TSServerResult;
+    repo: git.Repo;
+    oldTsEntrypointPath: string;
+    rawErrorArtifactPath: string;
+    replayScriptPath: string;
+    repoDir: string;
+    downloadDir: string;
+    replayScriptArtifactPath: string;
+    replayScriptName: string;
+}
+
 interface RepoResult {
     readonly status: RepoStatus;
     readonly summary?: string;
+    readonly tsServerResult?: TSServerResult;
     readonly replayScriptPath?: string;
     readonly rawErrorPath?: string;
 }
@@ -203,8 +226,6 @@ async function getTsServerRepoResult(
         ? (await installPackagesAndGetCommands(repo, downloadDir, repoDir, monorepoPackages, /*cleanOnFailure*/ true, diagnosticOutput))!
         : [];
 
-    const isUserTestRepo = !repo.url;
-
     const replayScriptName = path.basename(replayScriptArtifactPath);
     const replayScriptPath = path.join(downloadDir, replayScriptName);
 
@@ -302,110 +323,173 @@ async function getTsServerRepoResult(
             }
         }
 
-        const owner = repo.owner ? `${mdEscape(repo.owner)}/` : "";
-        const url = repo.url ? `(${repo.url})` : "";
+        const tsServerResult = {
+            oldServerFailed,
+            oldSpawnResult,
+            newServerFailed,
+            newSpawnResult,
+            replayScriptPath,
+            installCommands,
+        };
+
+        if (oldServerFailed && !newServerFailed) {
+            return { status: "Detected interesting changes", tsServerResult }
+        }
+        if (!newServerFailed) {
+            return { status: "Detected no interesting changes" };
+        }
 
-        let summary = `## [${owner}${mdEscape(repo.name)}]${url}\n`;
+        return { status: "Detected interesting changes", tsServerResult, replayScriptPath, rawErrorPath };
+    }
+    catch (err) {
+        reportError(err, `Error running tsserver on ${repo.url ?? repo.name}`);
+        return { status: "Unknown failure" };
+    }
+    finally {
+        console.log(`Done ${repo.url ?? repo.name}`);
+        logStepTime(diagnosticOutput, repo, "language service", lsStart);
+    }
+}
 
-        if (oldServerFailed) {
-            const oldServerError = oldSpawnResult?.stdout
-                ? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ true)
+function groupErrors(summaries: Summary[]) {
+    const groupedOldErrors = new Map<string, Summary[]>();
+    const groupedNewErrors = new Map<string, Summary[]>();
+    let group: Map<string, Summary[]>;
+    let error: ServerHarnessOutput | string;
+    for (const summary of summaries) {
+        if (summary.tsServerResult.newServerFailed) {
+            // Group new errors
+            error = parseServerHarnessOutput(summary.tsServerResult.newSpawnResult!.stdout);
+            group = groupedNewErrors;
+        }
+        else if (summary.tsServerResult.oldServerFailed) {
+            // Group old errors
+            const { oldSpawnResult } = summary.tsServerResult;
+            error = oldSpawnResult?.stdout
+                ? parseServerHarnessOutput(oldSpawnResult.stdout)
                 : `Timed out after ${executionTimeout} ms`;
-            summary += `
+
+            group = groupedOldErrors;
+        }
+        else {
+            continue;
+        }
+
+        const key = typeof error === "string" ? getHash([error]) : getHashForStack(error.message);
+        const value = group.get(key) ?? [];
+        value.push(summary);
+        group.set(key, value);
+    }
+
+    return { groupedOldErrors, groupedNewErrors }
+}
+
+function getErrorMessage(output: string): string {
+    const error = parseServerHarnessOutput(output);
+
+    return typeof error === "string" ? error : getErrorMessageFromStack(error.message);
+}
+
+function createOldErrorSummary(summaries: Summary[]): string {
+    const { oldSpawnResult } = summaries[0].tsServerResult;
+
+    const oldServerError = oldSpawnResult?.stdout
+        ? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ true)
+        : `Timed out after ${executionTimeout} ms`;
+
+    const errorMessage = oldSpawnResult?.stdout ? getErrorMessage(oldSpawnResult.stdout) : oldServerError;
+
+    let text = `
 <details>
-<summary>:warning: Note that ${path.basename(path.dirname(path.dirname(oldTsServerPath)))} had errors :warning:</summary>
+<summary>${errorMessage}</summary>
 
 \`\`\`
 ${oldServerError}
 \`\`\`
 
-</details>
-
+<h4>Repos no longer reporting the error</h4>
+<ul>
 `;
-            if (!newServerFailed) {
-                summary += `
-:tada: New server no longer has errors :tada:
+
+    for (const summary of summaries) {
+        const owner = summary.repo.owner ? `${mdEscape(summary.repo.owner)}/` : "";
+        const url = summary.repo.url ?? "";
+
+        text += `<li><a href="${url}">${owner + mdEscape(summary.repo.name)}</a></li>\n`
+    }
+
+    text += `
+</ul>
+</details>
 `;
-                return { status: "Detected interesting changes", summary }
-            }
-        }
-        
-        if (!newServerFailed) {
-            return { status: "Detected no interesting changes" };
-        }
 
-        summary += `
+    return text;
+}
+
+async function createNewErrorSummaryAsync(summaries: Summary[]): Promise<string> {
+    let text = `<h2>${getErrorMessage(summaries[0].tsServerResult.newSpawnResult.stdout)}</h2>
 
 \`\`\`
-${prettyPrintServerHarnessOutput(newSpawnResult.stdout, /*filter*/ true)}
+${prettyPrintServerHarnessOutput(summaries[0].tsServerResult.newSpawnResult.stdout, /*filter*/ true)}
 \`\`\`
-That is a filtered view of the text. To see the raw error text, go to ${rawErrorArtifactPath}</code> in the <a href="${artifactFolderUrlPlaceholder}">artifact folder</a></li>\n
-`;
 
-        summary += `
+<h4>Affected repos</h4>`;
+
+    for (const summary of summaries) {
+        const owner = summary.repo.owner ? `${mdEscape(summary.repo.owner)}/` : "";
+        const url = summary.repo.url ?? "";
+
+        text += `
 <details>
-<summary><h3>Last few requests</h3></summary>
+<summary><a href="${url}">${owner + mdEscape(summary.repo.name)}</a></summary>
+Raw error text: <code>${summary.rawErrorArtifactPath}</code> in the <a href="${artifactFolderUrlPlaceholder}">artifact folder</a>
+<h4>Last few requests</h4>
 
 \`\`\`json
-${fs.readFileSync(replayScriptPath, { encoding: "utf-8" }).split(/\r?\n/).slice(-5).join("\n")}
+${fs.readFileSync(summary.replayScriptPath, { encoding: "utf-8" }).split(/\r?\n/).slice(-5).join("\n")}
 \`\`\`
 
-</details>
-
-`;
-
-        // Markdown doesn't seem to support a <details> list item, so this chunk is in HTML
-
-        summary += `<details>
-<summary><h3>Repro Steps</h3></summary>
+<h4>Repro steps</h4>
 <ol>
 `;
-        if (isUserTestRepo) {
-            summary += `<li>Download user test <code>${repo.name}</code></li>\n`;
+        // No url means is user test repo
+        if (!summary.repo.url) {
+            text += `<li>Download user test <code>${summary.repo.name}</code></li>\n`;
         }
         else {
-            summary += `<li><code>git clone ${repo.url!} --recurse-submodules</code></li>\n`;
+            text += `<li><code>git clone ${summary.repo.url} --recurse-submodules</code></li>\n`;
 
             try {
                 console.log("Extracting commit SHA for repro steps");
-                const commit = (await execAsync(repoDir, `git rev-parse @`)).trim();
-                summary += `<li>In dir <code>${repo.name}</code>, run <code>git reset --hard ${commit}</code></li>\n`;
+                const commit = (await execAsync(summary.repoDir, `git rev-parse @`)).trim();
+                text += `<li>In dir <code>${summary.repo.name}</code>, run <code>git reset --hard ${commit}</code></li>\n`;
             }
             catch {
             }
         }
 
-        if (installCommands.length > 1) {
-            summary += "<li><details><summary>Install packages (exact steps are below, but it might be easier to follow the repo readme)</summary><ol>\n";
+        if (summary.tsServerResult.installCommands.length > 1) {
+            text += "<li><details><summary>Install packages (exact steps are below, but it might be easier to follow the repo readme)</summary><ol>\n";
         }
-        for (const command of installCommands) {
-            summary += `  <li>In dir <code>${path.relative(downloadDir, command.directory)}</code>, run <code>${command.tool} ${command.arguments.join(" ")}</code></li>\n`;
+        for (const command of summary.tsServerResult.installCommands) {
+            text += `  <li>In dir <code>${path.relative(summary.downloadDir, command.directory)}</code>, run <code>${command.tool} ${command.arguments.join(" ")}</code></li>\n`;
         }
-        if (installCommands.length > 1) {
-            summary += "</ol></details>\n";
+        if (summary.tsServerResult.installCommands.length > 1) {
+            text += "</ol></details>\n";
         }
 
         // The URL of the artifact can be determined via AzDO REST APIs, but not until after the artifact is published
-        summary += `<li>Back in the initial folder, download <code>${replayScriptArtifactPath}</code> from the <a href="${artifactFolderUrlPlaceholder}">artifact folder</a></li>\n`;
-        summary += `<li><code>npm install --no-save @typescript/server-replay</code></li>\n`;
-        summary += `<li><code>npx tsreplay ./${repo.name} ./${replayScriptName} path/to/tsserver.js</code></li>\n`;
-        summary += `<li><code>npx tsreplay --help</code> to learn about helpful switches for debugging, logging, etc</li>\n`;
+        text += `<li>Back in the initial folder, download <code>${summary.replayScriptArtifactPath}</code> from the <a href="${artifactFolderUrlPlaceholder}">artifact folder</a></li>\n`;
+        text += `<li><code>npm install --no-save @typescript/server-replay</code></li>\n`;
+        text += `<li><code>npx tsreplay ./${summary.repo.name} ./${summary.replayScriptName} path/to/tsserver.js</code></li>\n`;
+        text += `<li><code>npx tsreplay --help</code> to learn about helpful switches for debugging, logging, etc</li>\n`;
 
-        summary += `</ol>
+        text += `</ol>
 </details>
-
 `;
-
-        return { status: "Detected interesting changes", summary, replayScriptPath, rawErrorPath };
-    }
-    catch (err) {
-        reportError(err, `Error running tsserver on ${repo.url ?? repo.name}`);
-        return { status: "Unknown failure" };
-    }
-    finally {
-        console.log(`Done ${repo.url ?? repo.name}`);
-        logStepTime(diagnosticOutput, repo, "language service", lsStart);
     }
+
+    return text;
 }
 
 // Exported for testing
@@ -658,6 +742,8 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
 
     const isPr = params.testType === "user" && !!params.prNumber
 
+    var summaries: Summary[] = [];
+
     let i = 1;
     for (const repo of repos) {
         console.log(`Starting #${i++} / ${repos.length}: ${repo.url ?? repo.name}`);
@@ -672,16 +758,36 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
                 : repo.name;
             const replayScriptFileName = `${repoPrefix}.${replayScriptFileNameSuffix}`;
             const rawErrorFileName = `${repoPrefix}.${rawErrorFileNameSuffix}`;
-            const { status, summary, replayScriptPath, rawErrorPath } = params.entrypoint === "tsc"
+
+            const rawErrorArtifactPath = path.join(params.resultDirName, rawErrorFileName);
+            const replayScriptArtifactPath = path.join(params.resultDirName, replayScriptFileName);
+
+            const { status, summary, tsServerResult, replayScriptPath, rawErrorPath } = params.entrypoint === "tsc"
                 ? await getTscRepoResult(repo, userTestsDir, oldTsEntrypointPath, newTsEntrypointPath, params.buildWithNewWhenOldFails, downloadDir, diagnosticOutput)
-                : await getTsServerRepoResult(repo, userTestsDir, oldTsEntrypointPath, newTsEntrypointPath, downloadDir, path.join(params.resultDirName, replayScriptFileName), path.join(params.resultDirName, rawErrorFileName), diagnosticOutput, isPr);
+                : await getTsServerRepoResult(repo, userTestsDir, oldTsEntrypointPath, newTsEntrypointPath, downloadDir, replayScriptArtifactPath, rawErrorArtifactPath, diagnosticOutput, isPr);
             console.log(`Repo ${repo.url ?? repo.name} had status "${status}"`);
             statusCounts[status] = (statusCounts[status] ?? 0) + 1;
-            if (summary) {
 
+            if (summary) {
                 const resultFileName = `${repoPrefix}.${resultFileNameSuffix}`;
                 await fs.promises.writeFile(path.join(resultDirPath, resultFileName), summary, { encoding: "utf-8" });
+            }
+
+            if (tsServerResult) {
+                summaries.push({
+                    tsServerResult,
+                    repo,
+                    oldTsEntrypointPath,
+                    rawErrorArtifactPath,
+                    replayScriptPath: path.join(downloadDir, path.basename(replayScriptArtifactPath)),
+                    repoDir: path.join(downloadDir, repo.name),
+                    downloadDir,
+                    replayScriptArtifactPath,
+                    replayScriptName: path.basename(replayScriptArtifactPath),
+                });
+            }
 
+            if (summary || tsServerResult) {
                 // In practice, there will only be a replay script when the entrypoint is tsserver
                 // There can be replay steps without a summary, but then they're not interesting
                 if (replayScriptPath) {
@@ -690,9 +796,7 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
                 if (rawErrorPath) {
                     await fs.promises.copyFile(rawErrorPath, path.join(resultDirPath, rawErrorFileName));
                 }
-
             }
-
         }
         finally {
             // Throw away the repo so we don't run out of space
@@ -727,6 +831,25 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
         }
     }
 
+    // Group errors and create summary files.
+    if (summaries.length > 0) {
+        const { groupedOldErrors, groupedNewErrors } = groupErrors(summaries);
+
+        for (let [key, value] of groupedOldErrors) {
+            const summary = createOldErrorSummary(value);
+            const resultFileName = `!${key}.${resultFileNameSuffix}`; // Exclamation point makes the file to be put first when ordering.
+
+            await fs.promises.writeFile(path.join(resultDirPath, resultFileName), summary, { encoding: "utf-8" });
+        }
+
+        for (let [key, value] of groupedNewErrors) {
+            const summary = await createNewErrorSummaryAsync(value);
+            const resultFileName = `${key}.${resultFileNameSuffix}`;
+
+            await fs.promises.writeFile(path.join(resultDirPath, resultFileName), summary, { encoding: "utf-8" });
+        }
+    }
+
     if (params.tmpfs) {
         await execAsync(processCwd, "sudo rm -rf " + downloadDir);
         await execAsync(processCwd, "sudo rm -rf " + oldTscDirPath);
@@ -993,4 +1116,4 @@ async function downloadTsNpmAsync(cwd: string, version: string, entrypoint: TsEn
     }
 
     return { tsEntrypointPath, resolvedVersion };
-}
+}

src/postGithubComments.ts

@@ -55,6 +55,9 @@ else {
     summary = `Everything looks good!`;
 }
 
+// Files starting with an exclamation point are old server errors.
+const hasOldErrors = pu.glob(resultDirPath, `**/!*.${resultFileNameSuffix}`).length !== 0;
+
 const resultPaths = pu.glob(resultDirPath, `**/*.${resultFileNameSuffix}`).sort((a, b) => path.basename(a).localeCompare(path.basename(b)));
 const outputs = resultPaths.map(p => fs.readFileSync(p, { encoding: "utf-8" }).replace(new RegExp(artifactFolderUrlPlaceholder, "g"), artifactsUri));
 
@@ -67,9 +70,10 @@ if (!outputs.length) {
     git.createComment(+prNumber, +commentNumber, postResult, [header]);
 }
 else {
+    const oldErrorHeader = `<h2>:warning: Old server errors :warning:</h2>`;
     const openDetails = `\n\n<details>\n<summary>Details</summary>\n\n`;
     const closeDetails = `\n</details>`;
-    const initialHeader = header + openDetails;
+    const initialHeader = header + openDetails + (hasOldErrors ? oldErrorHeader : '');
     const continuationHeader = `@${userToTag} Here are some more interesting changes from running the ${suiteDescription} suite${openDetails}`;
     const trunctationSuffix = `\n:error: Truncated - see log for full output :error:`;