From 33197d254546aae0b1e65fc16cec6dfd90e6b9b1 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 16 Feb 2023 14:31:51 -0800 Subject: [PATCH 1/4] Ignore tsserver results when both results have identical request IDs and command names --- src/main.ts | 48 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/src/main.ts b/src/main.ts index ca2f2fd..7d5b85a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -275,6 +275,19 @@ async function getTsServerRepoResult( oldSpawnResult?.stdout ? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ false) : `Timed out after ${executionTimeout} ms`)); + + if (oldSpawnResult) { + const oldOut = parseServerHarnessOutput(oldSpawnResult.stdout); + const newOut = parseServerHarnessOutput(newSpawnResult.stdout); + + if ( + typeof oldOut !== "string" && typeof newOut !== "string" + && oldOut.request_seq === newOut.request_seq + && oldOut.command === newOut.command + ) { + return { status: "Detected no interesting changes" }; + } + } } const owner = repo.owner ? `${mdEscape(repo.owner)}/` : ""; @@ -805,17 +818,16 @@ ${spawnResult.stderr} `); } -function prettyPrintServerHarnessOutput(error: string, filter: boolean): string { - try { - const errorObj = JSON.parse(error); - if (errorObj.message) { - return `Req #${errorObj.request_seq} - ${errorObj.command} -${filter ? filterToTsserverLines(errorObj.message) : errorObj.message}`; - } - // It's not really clear how this could happen, but reporting the whole repsonse should be fine - // if there's no message property - return JSON.stringify(errorObj, undefined, 2); +export interface ServerHarnessOutput { + request_seq: number; + command: string; + message: string +} + +function parseServerHarnessOutput(error: string): ServerHarnessOutput | string { + try { + return JSON.parse(error) } catch { // Sometimes, the response isn't JSON and that's fine @@ -823,6 +835,22 @@ ${filter ? filterToTsserverLines(errorObj.message) : errorObj.message}`; } } +function prettyPrintServerHarnessOutput(error: string, filter: boolean): string { + const errorObj = parseServerHarnessOutput(error); + if (typeof errorObj === "string") { + return errorObj; + } + + if (errorObj.message) { + return `Req #${errorObj.request_seq} - ${errorObj.command} +${filter ? filterToTsserverLines(errorObj.message) : errorObj.message}`; + } + + // It's not really clear how this could happen, but reporting the whole repsonse should be fine + // if there's no message property + return JSON.stringify(errorObj, undefined, 2); +} + function filterToTsserverLines(stackLines: string): string { const tsserverRegex = /^.*tsserver\.js.*$/mg; let tsserverLines = ""; From f84ac2762ef934a6459c8b70964a85c53ddd4359 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 16 Feb 2023 14:54:32 -0800 Subject: [PATCH 2/4] Only ignore old error in PR --- src/main.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/main.ts b/src/main.ts index 7d5b85a..5cb7bd9 100644 --- a/src/main.ts +++ b/src/main.ts @@ -187,7 +187,9 @@ async function getTsServerRepoResult( downloadDir: string, replayScriptArtifactPath: string, rawErrorArtifactPath: string, - diagnosticOutput: boolean): Promise { + diagnosticOutput: boolean, + isPr: boolean, +): Promise { if (!await cloneRepo(repo, userTestsDir, downloadDir, diagnosticOutput)) { return { status: "Git clone failed" }; @@ -276,7 +278,7 @@ async function getTsServerRepoResult( ? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ false) : `Timed out after ${executionTimeout} ms`)); - if (oldSpawnResult) { + if (isPr && oldSpawnResult) { const oldOut = parseServerHarnessOutput(oldSpawnResult.stdout); const newOut = parseServerHarnessOutput(newSpawnResult.stdout); @@ -634,6 +636,8 @@ export async function mainAsync(params: GitParams | UserParams): Promise { // An object is easier to de/serialize than a real map const statusCounts: { [P in RepoStatus]?: number } = {}; + const isPr = params.testType === "user" && !!params.prNumber + let i = 1; for (const repo of repos) { console.log(`Starting #${i++} / ${repos.length}: ${repo.url ?? repo.name}`); @@ -650,7 +654,7 @@ export async function mainAsync(params: GitParams | UserParams): Promise { const rawErrorFileName = `${repoPrefix}.${rawErrorFileNameSuffix}`; const { status, summary, 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); + : await getTsServerRepoResult(repo, userTestsDir, oldTsEntrypointPath, newTsEntrypointPath, downloadDir, path.join(params.resultDirName, replayScriptFileName), path.join(params.resultDirName, rawErrorFileName), diagnosticOutput, isPr); console.log(`Repo ${repo.url ?? repo.name} had status "${status}"`); statusCounts[status] = (statusCounts[status] ?? 0) + 1; if (summary) { From 2d443b4ee1f8cf34fef1b2f3bdcead68a104a3da Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 16 Feb 2023 15:11:46 -0800 Subject: [PATCH 3/4] Tweak logic so we report when the result changes --- src/main.ts | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/main.ts b/src/main.ts index 5cb7bd9..ec7d88c 100644 --- a/src/main.ts +++ b/src/main.ts @@ -235,7 +235,6 @@ async function getTsServerRepoResult( } console.log("No issues found"); - return { status: "Detected no interesting changes" }; case exercise.EXIT_LANGUAGE_SERVICE_DISABLED: console.log("Skipping since language service was disabled"); return { status: "Language service disabled in new TS" }; @@ -255,9 +254,13 @@ async function getTsServerRepoResult( return { status: "Unknown failure" }; } - console.log(`Issue found in ${newTsServerPath} (new):`); - console.log(insetLines(prettyPrintServerHarnessOutput(newSpawnResult.stdout, /*filter*/ false))); - await fs.promises.writeFile(rawErrorPath, prettyPrintServerHarnessOutput(newSpawnResult.stdout, /*filter*/ false)); + const newServerFailed = !newSpawnResult.code; + + if (newServerFailed) { + console.log(`Issue found in ${newTsServerPath} (new):`); + console.log(insetLines(prettyPrintServerHarnessOutput(newSpawnResult.stdout, /*filter*/ false))); + await fs.promises.writeFile(rawErrorPath, prettyPrintServerHarnessOutput(newSpawnResult.stdout, /*filter*/ false)); + } console.log(`Testing with ${oldTsServerPath} (old)`); const oldSpawnResult = await spawnWithTimeoutAsync(repoDir, process.argv[0], [path.join(__dirname, "..", "node_modules", "@typescript", "server-replay", "replay.js"), repoDir, replayScriptPath, oldTsServerPath, "-u"], executionTimeout); @@ -270,6 +273,10 @@ async function getTsServerRepoResult( // NB: Unlike newServerFailed, this includes timeouts because "it used to timeout" is useful context for an error in the new server const oldServerFailed = !oldSpawnResult || !!oldSpawnResult.code || !!oldSpawnResult.signal; + if (!newServerFailed && !oldServerFailed) { + return { status: "Detected no interesting changes" }; + } + if (oldServerFailed) { console.log(`Issue found in ${oldTsServerPath} (old):`); console.log( @@ -278,7 +285,7 @@ async function getTsServerRepoResult( ? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ false) : `Timed out after ${executionTimeout} ms`)); - if (isPr && oldSpawnResult) { + if (isPr && newServerFailed && oldSpawnResult) { const oldOut = parseServerHarnessOutput(oldSpawnResult.stdout); const newOut = parseServerHarnessOutput(newSpawnResult.stdout); @@ -303,7 +310,7 @@ async function getTsServerRepoResult( : `Timed out after ${executionTimeout} ms`; summary += `
-:warning: Note that ${path.basename(path.dirname(path.dirname(oldTsServerPath)))} also had errors :warning: +:warning: Note that ${path.basename(path.dirname(path.dirname(oldTsServerPath)))} had errors :warning: \`\`\` ${oldServerError} @@ -312,6 +319,16 @@ ${oldServerError}
`; + if (!newServerFailed) { + summary += ` +:tada: New server no longer has errors :tada: +`; + return { status: "Detected interesting changes", summary } + } + } + + if (!newServerFailed) { + return { status: "Detected no interesting changes" }; } summary += ` From 9464beb9ddd3e339721cb3648517662267c27eb2 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 16 Feb 2023 15:37:00 -0800 Subject: [PATCH 4/4] Update src/main.ts Co-authored-by: Daniel Rosenwasser --- src/main.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main.ts b/src/main.ts index ec7d88c..43bb41c 100644 --- a/src/main.ts +++ b/src/main.ts @@ -285,6 +285,8 @@ async function getTsServerRepoResult( ? prettyPrintServerHarnessOutput(oldSpawnResult.stdout, /*filter*/ false) : `Timed out after ${executionTimeout} ms`)); + // We don't want to drown PRs with comments. + // Override the results to say nothing interesting changed. if (isPr && newServerFailed && oldSpawnResult) { const oldOut = parseServerHarnessOutput(oldSpawnResult.stdout); const newOut = parseServerHarnessOutput(newSpawnResult.stdout);