Skip to content

Commit dd74c5f

Browse files
DanielRosenwasserDanielRosenwasser
authored
Revert "Group concurrent errors" (#106)
1 parent fc6a148 commit dd74c5f

File tree

3 files changed

+71
-216
lines changed

3 files changed

+71
-216
lines changed

‎src/main.ts

Lines changed: 70 additions & 193 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import fs = require("fs");
99
import path = require("path");
1010
import mdEscape = require("markdown-escape");
1111
import randomSeed = require("random-seed");
12-
import { getErrorMessageFromStack, getHash, getHashForStack } from "./utils/hashStackTrace";
1312

1413
interface Params {
1514
/**
@@ -85,31 +84,9 @@ export type RepoStatus =
8584
| "Detected no interesting changes"
8685
;
8786

88-
interface TSServerResult {
89-
oldServerFailed: boolean;
90-
oldSpawnResult?: SpawnResult;
91-
newServerFailed: boolean;
92-
newSpawnResult: SpawnResult;
93-
replayScriptPath: string;
94-
installCommands: ip.InstallCommand[];
95-
}
96-
97-
interface Summary {
98-
tsServerResult: TSServerResult;
99-
repo: git.Repo;
100-
oldTsEntrypointPath: string;
101-
rawErrorArtifactPath: string;
102-
replayScriptPath: string;
103-
repoDir: string;
104-
downloadDir: string;
105-
replayScriptArtifactPath: string;
106-
replayScriptName: string;
107-
}
108-
10987
interface RepoResult {
11088
readonly status: RepoStatus;
11189
readonly summary?: string;
112-
readonly tsServerResult?: TSServerResult;
11390
readonly replayScriptPath?: string;
11491
readonly rawErrorPath?: string;
11592
}
@@ -226,6 +203,8 @@ async function getTsServerRepoResult(
226203
? (await installPackagesAndGetCommands(repo, downloadDir, repoDir, monorepoPackages, /*cleanOnFailure*/ true, diagnosticOutput))!
227204
: [];
228205

206+
const isUserTestRepo = !repo.url;
207+
229208
const replayScriptName = path.basename(replayScriptArtifactPath);
230209
const replayScriptPath = path.join(downloadDir, replayScriptName);
231210

@@ -323,173 +302,110 @@ async function getTsServerRepoResult(
323302
}
324303
}
325304

326-
const tsServerResult = {
327-
oldServerFailed,
328-
oldSpawnResult,
329-
newServerFailed,
330-
newSpawnResult,
331-
replayScriptPath,
332-
installCommands,
333-
};
334-
335-
if (oldServerFailed && !newServerFailed) {
336-
return { status: "Detected interesting changes", tsServerResult }
337-
}
338-
if (!newServerFailed) {
339-
return { status: "Detected no interesting changes" };
340-
}
305+
const owner = repo.owner ? `${mdEscape(repo.owner)}/` : "";
306+
const url = repo.url ? `(${repo.url})` : "";
341307

342-
return { status: "Detected interesting changes", tsServerResult, replayScriptPath, rawErrorPath };
343-
}
344-
catch (err) {
345-
reportError(err, `Error running tsserver on ${repo.url ?? repo.name}`);
346-
return { status: "Unknown failure" };
347-
}
348-
finally {
349-
console.log(`Done ${repo.url ?? repo.name}`);
350-
logStepTime(diagnosticOutput, repo, "language service", lsStart);
351-
}
352-
}
308+
let summary = `## [${owner}${mdEscape(repo.name)}]${url}\n`;
353309

354-
function groupErrors(summaries: Summary[]) {
355-
const groupedOldErrors = new Map<string, Summary[]>();
356-
const groupedNewErrors = new Map<string, Summary[]>();
357-
let group: Map<string, Summary[]>;
358-
let error: ServerHarnessOutput | string;
359-
for (const summary of summaries) {
360-
if (summary.tsServerResult.newServerFailed) {
361-
// Group new errors
362-
error = parseServerHarnessOutput(summary.tsServerResult.newSpawnResult!.stdout);
363-
group = groupedNewErrors;
364-
}
365-
else if (summary.tsServerResult.oldServerFailed) {
366-
// Group old errors
367-
const { oldSpawnResult } = summary.tsServerResult;
368-
error = oldSpawnResult?.stdout
369-
? parseServerHarnessOutput(oldSpawnResult.stdout)
310+
if (oldServerFailed) {
311+
const oldServerError = oldSpawnResult?.stdout
312+
? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ true)
370313
: `Timed out after ${executionTimeout} ms`;
371-
372-
group = groupedOldErrors;
373-
}
374-
else {
375-
continue;
376-
}
377-
378-
const key = typeof error === "string" ? getHash([error]) : getHashForStack(error.message);
379-
const value = group.get(key) ?? [];
380-
value.push(summary);
381-
group.set(key, value);
382-
}
383-
384-
return { groupedOldErrors, groupedNewErrors }
385-
}
386-
387-
function getErrorMessage(output: string): string {
388-
const error = parseServerHarnessOutput(output);
389-
390-
return typeof error === "string" ? error : getErrorMessageFromStack(error.message);
391-
}
392-
393-
function createOldErrorSummary(summaries: Summary[]): string {
394-
const { oldSpawnResult } = summaries[0].tsServerResult;
395-
396-
const oldServerError = oldSpawnResult?.stdout
397-
? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ true)
398-
: `Timed out after ${executionTimeout} ms`;
399-
400-
const errorMessage = oldSpawnResult?.stdout ? getErrorMessage(oldSpawnResult.stdout) : oldServerError;
401-
402-
let text = `
314+
summary += `
403315
<details>
404-
<summary>${errorMessage}</summary>
316+
<summary>:warning: Note that ${path.basename(path.dirname(path.dirname(oldTsServerPath)))} had errors :warning:</summary>
405317
406318
\`\`\`
407319
${oldServerError}
408320
\`\`\`
409321
410-
<h4>Repos no longer reporting the error</h4>
411-
<ul>
412-
`;
413-
414-
for (const summary of summaries) {
415-
const owner = summary.repo.owner ? `${mdEscape(summary.repo.owner)}/` : "";
416-
const url = summary.repo.url ?? "";
417-
418-
text += `<li><a href="${url}">${owner + mdEscape(summary.repo.name)}</a></li>\n`
419-
}
420-
421-
text += `
422-
</ul>
423322
</details>
424-
`;
425323
426-
return text;
427-
}
324+
`;
325+
if (!newServerFailed) {
326+
summary += `
327+
:tada: New server no longer has errors :tada:
328+
`;
329+
return { status: "Detected interesting changes", summary }
330+
}
331+
}
332+
333+
if (!newServerFailed) {
334+
return { status: "Detected no interesting changes" };
335+
}
428336

429-
async function createNewErrorSummaryAsync(summaries: Summary[]): Promise<string> {
430-
let text = `<h2>${getErrorMessage(summaries[0].tsServerResult.newSpawnResult.stdout)}</h2>
337+
summary += `
431338
432339
\`\`\`
433-
${prettyPrintServerHarnessOutput(summaries[0].tsServerResult.newSpawnResult.stdout, /*filter*/ true)}
340+
${prettyPrintServerHarnessOutput(newSpawnResult.stdout, /*filter*/ true)}
434341
\`\`\`
342+
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
343+
`;
435344

436-
<h4>Affected repos</h4>`;
437-
438-
for (const summary of summaries) {
439-
const owner = summary.repo.owner ? `${mdEscape(summary.repo.owner)}/` : "";
440-
const url = summary.repo.url ?? "";
441-
442-
text += `
345+
summary += `
443346
<details>
444-
<summary><a href="${url}">${owner + mdEscape(summary.repo.name)}</a></summary>
445-
Raw error text: <code>${summary.rawErrorArtifactPath}</code> in the <a href="${artifactFolderUrlPlaceholder}">artifact folder</a>
446-
<h4>Last few requests</h4>
347+
<summary><h3>Last few requests</h3></summary>
447348
448349
\`\`\`json
449-
${fs.readFileSync(summary.replayScriptPath, { encoding: "utf-8" }).split(/\r?\n/).slice(-5).join("\n")}
350+
${fs.readFileSync(replayScriptPath, { encoding: "utf-8" }).split(/\r?\n/).slice(-5).join("\n")}
450351
\`\`\`
451352
452-
<h4>Repro steps</h4>
353+
</details>
354+
355+
`;
356+
357+
// Markdown doesn't seem to support a <details> list item, so this chunk is in HTML
358+
359+
summary += `<details>
360+
<summary><h3>Repro Steps</h3></summary>
453361
<ol>
454362
`;
455-
// No url means is user test repo
456-
if (!summary.repo.url) {
457-
text += `<li>Download user test <code>${summary.repo.name}</code></li>\n`;
363+
if (isUserTestRepo) {
364+
summary += `<li>Download user test <code>${repo.name}</code></li>\n`;
458365
}
459366
else {
460-
text += `<li><code>git clone ${summary.repo.url} --recurse-submodules</code></li>\n`;
367+
summary += `<li><code>git clone ${repo.url!} --recurse-submodules</code></li>\n`;
461368

462369
try {
463370
console.log("Extracting commit SHA for repro steps");
464-
const commit = (await execAsync(summary.repoDir, `git rev-parse @`)).trim();
465-
text += `<li>In dir <code>${summary.repo.name}</code>, run <code>git reset --hard ${commit}</code></li>\n`;
371+
const commit = (await execAsync(repoDir, `git rev-parse @`)).trim();
372+
summary += `<li>In dir <code>${repo.name}</code>, run <code>git reset --hard ${commit}</code></li>\n`;
466373
}
467374
catch {
468375
}
469376
}
470377

471-
if (summary.tsServerResult.installCommands.length > 1) {
472-
text += "<li><details><summary>Install packages (exact steps are below, but it might be easier to follow the repo readme)</summary><ol>\n";
378+
if (installCommands.length > 1) {
379+
summary += "<li><details><summary>Install packages (exact steps are below, but it might be easier to follow the repo readme)</summary><ol>\n";
473380
}
474-
for (const command of summary.tsServerResult.installCommands) {
475-
text += ` <li>In dir <code>${path.relative(summary.downloadDir, command.directory)}</code>, run <code>${command.tool} ${command.arguments.join(" ")}</code></li>\n`;
381+
for (const command of installCommands) {
382+
summary += ` <li>In dir <code>${path.relative(downloadDir, command.directory)}</code>, run <code>${command.tool} ${command.arguments.join(" ")}</code></li>\n`;
476383
}
477-
if (summary.tsServerResult.installCommands.length > 1) {
478-
text += "</ol></details>\n";
384+
if (installCommands.length > 1) {
385+
summary += "</ol></details>\n";
479386
}
480387

481388
// The URL of the artifact can be determined via AzDO REST APIs, but not until after the artifact is published
482-
text += `<li>Back in the initial folder, download <code>${summary.replayScriptArtifactPath}</code> from the <a href="${artifactFolderUrlPlaceholder}">artifact folder</a></li>\n`;
483-
text += `<li><code>npm install --no-save @typescript/server-replay</code></li>\n`;
484-
text += `<li><code>npx tsreplay ./${summary.repo.name} ./${summary.replayScriptName} path/to/tsserver.js</code></li>\n`;
485-
text += `<li><code>npx tsreplay --help</code> to learn about helpful switches for debugging, logging, etc</li>\n`;
389+
summary += `<li>Back in the initial folder, download <code>${replayScriptArtifactPath}</code> from the <a href="${artifactFolderUrlPlaceholder}">artifact folder</a></li>\n`;
390+
summary += `<li><code>npm install --no-save @typescript/server-replay</code></li>\n`;
391+
summary += `<li><code>npx tsreplay ./${repo.name} ./${replayScriptName} path/to/tsserver.js</code></li>\n`;
392+
summary += `<li><code>npx tsreplay --help</code> to learn about helpful switches for debugging, logging, etc</li>\n`;
486393

487-
text += `</ol>
394+
summary += `</ol>
488395
</details>
396+
489397
`;
490-
}
491398

492-
return text;
399+
return { status: "Detected interesting changes", summary, replayScriptPath, rawErrorPath };
400+
}
401+
catch (err) {
402+
reportError(err, `Error running tsserver on ${repo.url ?? repo.name}`);
403+
return { status: "Unknown failure" };
404+
}
405+
finally {
406+
console.log(`Done ${repo.url ?? repo.name}`);
407+
logStepTime(diagnosticOutput, repo, "language service", lsStart);
408+
}
493409
}
494410

495411
// Exported for testing
@@ -742,8 +658,6 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
742658

743659
const isPr = params.testType === "user" && !!params.prNumber
744660

745-
var summaries: Summary[] = [];
746-
747661
let i = 1;
748662
for (const repo of repos) {
749663
console.log(`Starting #${i++} / ${repos.length}: ${repo.url ?? repo.name}`);
@@ -758,36 +672,16 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
758672
: repo.name;
759673
const replayScriptFileName = `${repoPrefix}.${replayScriptFileNameSuffix}`;
760674
const rawErrorFileName = `${repoPrefix}.${rawErrorFileNameSuffix}`;
761-
762-
const rawErrorArtifactPath = path.join(params.resultDirName, rawErrorFileName);
763-
const replayScriptArtifactPath = path.join(params.resultDirName, replayScriptFileName);
764-
765-
const { status, summary, tsServerResult, replayScriptPath, rawErrorPath } = params.entrypoint === "tsc"
675+
const { status, summary, replayScriptPath, rawErrorPath } = params.entrypoint === "tsc"
766676
? await getTscRepoResult(repo, userTestsDir, oldTsEntrypointPath, newTsEntrypointPath, params.buildWithNewWhenOldFails, downloadDir, diagnosticOutput)
767-
: await getTsServerRepoResult(repo, userTestsDir, oldTsEntrypointPath, newTsEntrypointPath, downloadDir, replayScriptArtifactPath, rawErrorArtifactPath, diagnosticOutput, isPr);
677+
: await getTsServerRepoResult(repo, userTestsDir, oldTsEntrypointPath, newTsEntrypointPath, downloadDir, path.join(params.resultDirName, replayScriptFileName), path.join(params.resultDirName, rawErrorFileName), diagnosticOutput, isPr);
768678
console.log(`Repo ${repo.url ?? repo.name} had status "${status}"`);
769679
statusCounts[status] = (statusCounts[status] ?? 0) + 1;
770-
771680
if (summary) {
681+
772682
const resultFileName = `${repoPrefix}.${resultFileNameSuffix}`;
773683
await fs.promises.writeFile(path.join(resultDirPath, resultFileName), summary, { encoding: "utf-8" });
774-
}
775-
776-
if (tsServerResult) {
777-
summaries.push({
778-
tsServerResult,
779-
repo,
780-
oldTsEntrypointPath,
781-
rawErrorArtifactPath,
782-
replayScriptPath: path.join(downloadDir, path.basename(replayScriptArtifactPath)),
783-
repoDir: path.join(downloadDir, repo.name),
784-
downloadDir,
785-
replayScriptArtifactPath,
786-
replayScriptName: path.basename(replayScriptArtifactPath),
787-
});
788-
}
789684

790-
if (summary || tsServerResult) {
791685
// In practice, there will only be a replay script when the entrypoint is tsserver
792686
// There can be replay steps without a summary, but then they're not interesting
793687
if (replayScriptPath) {
@@ -796,7 +690,9 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
796690
if (rawErrorPath) {
797691
await fs.promises.copyFile(rawErrorPath, path.join(resultDirPath, rawErrorFileName));
798692
}
693+
799694
}
695+
800696
}
801697
finally {
802698
// Throw away the repo so we don't run out of space
@@ -831,25 +727,6 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
831727
}
832728
}
833729

834-
// Group errors and create summary files.
835-
if (summaries.length > 0) {
836-
const { groupedOldErrors, groupedNewErrors } = groupErrors(summaries);
837-
838-
for (let [key, value] of groupedOldErrors) {
839-
const summary = createOldErrorSummary(value);
840-
const resultFileName = `!${key}.${resultFileNameSuffix}`; // Exclamation point makes the file to be put first when ordering.
841-
842-
await fs.promises.writeFile(path.join(resultDirPath, resultFileName), summary, { encoding: "utf-8" });
843-
}
844-
845-
for (let [key, value] of groupedNewErrors) {
846-
const summary = await createNewErrorSummaryAsync(value);
847-
const resultFileName = `${key}.${resultFileNameSuffix}`;
848-
849-
await fs.promises.writeFile(path.join(resultDirPath, resultFileName), summary, { encoding: "utf-8" });
850-
}
851-
}
852-
853730
if (params.tmpfs) {
854731
await execAsync(processCwd, "sudo rm -rf " + downloadDir);
855732
await execAsync(processCwd, "sudo rm -rf " + oldTscDirPath);
@@ -1116,4 +993,4 @@ async function downloadTsNpmAsync(cwd: string, version: string, entrypoint: TsEn
1116993
}
1117994

1118995
return { tsEntrypointPath, resolvedVersion };
1119-
}
996+
}

‎src/postGithubComments.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,6 @@ else {
5555
summary = `Everything looks good!`;
5656
}
5757

58-
// Files starting with an exclamation point are old server errors.
59-
const hasOldErrors = pu.glob(resultDirPath, `**/!*.${resultFileNameSuffix}`).length !== 0;
60-
6158
const resultPaths = pu.glob(resultDirPath, `**/*.${resultFileNameSuffix}`).sort((a, b) => path.basename(a).localeCompare(path.basename(b)));
6259
const outputs = resultPaths.map(p => fs.readFileSync(p, { encoding: "utf-8" }).replace(new RegExp(artifactFolderUrlPlaceholder, "g"), artifactsUri));
6360

@@ -70,10 +67,9 @@ if (!outputs.length) {
7067
git.createComment(+prNumber, +commentNumber, postResult, [header]);
7168
}
7269
else {
73-
const oldErrorHeader = `<h2>:warning: Old server errors :warning:</h2>`;
7470
const openDetails = `\n\n<details>\n<summary>Details</summary>\n\n`;
7571
const closeDetails = `\n</details>`;
76-
const initialHeader = header + openDetails + (hasOldErrors ? oldErrorHeader : '');
72+
const initialHeader = header + openDetails;
7773
const continuationHeader = `@${userToTag} Here are some more interesting changes from running the ${suiteDescription} suite${openDetails}`;
7874
const trunctationSuffix = `\n:error: Truncated - see log for full output :error:`;
7975

0 commit comments

Comments
 (0)