Skip to content

Ignore tsserver results when both results have identical request IDs and command names #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 16, 2023
Merged

Ignore tsserver results when both results have identical request IDs and command names #98

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
33197d2
Ignore tsserver results when both results have identical request IDs …
jakebailey Feb 16, 2023
f84ac27
Only ignore old error in PR
jakebailey Feb 16, 2023
2d443b4
Tweak logic so we report when the result changes
jakebailey Feb 16, 2023
9464beb
Update src/main.ts
jakebailey Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 68 additions & 17 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,9 @@ async function getTsServerRepoResult(
downloadDir: string,
replayScriptArtifactPath: string,
rawErrorArtifactPath: string,
diagnosticOutput: boolean): Promise<RepoResult> {
diagnosticOutput: boolean,
isPr: boolean,
): Promise<RepoResult> {

if (!await cloneRepo(repo, userTestsDir, downloadDir, diagnosticOutput)) {
return { status: "Git clone failed" };
Expand Down Expand Up @@ -233,7 +235,6 @@ async function getTsServerRepoResult(
}

console.log("No issues found");
return { status: "Detected no interesting changes" };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this was wrong because fallthrough! Will fix.

case exercise.EXIT_LANGUAGE_SERVICE_DISABLED:
console.log("Skipping since language service was disabled");
return { status: "Language service disabled in new TS" };
Expand All @@ -253,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);
Expand All @@ -268,13 +273,32 @@ 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(
insetLines(
oldSpawnResult?.stdout
? 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);

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)}/` : "";
Expand All @@ -288,7 +312,7 @@ async function getTsServerRepoResult(
: `Timed out after ${executionTimeout} ms`;
summary += `
<details>
<summary>:warning: Note that ${path.basename(path.dirname(path.dirname(oldTsServerPath)))} also had errors :warning:</summary>
<summary>:warning: Note that ${path.basename(path.dirname(path.dirname(oldTsServerPath)))} had errors :warning:</summary>

\`\`\`
${oldServerError}
Expand All @@ -297,6 +321,16 @@ ${oldServerError}
</details>

`;
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 += `
Expand Down Expand Up @@ -621,6 +655,8 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
// 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}`);
Expand All @@ -637,7 +673,7 @@ export async function mainAsync(params: GitParams | UserParams): Promise<void> {
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) {
Expand Down Expand Up @@ -805,24 +841,39 @@ ${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
return error;
}
}

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 = "";
Expand Down