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

[jakebailey]

While the tsc reporter will compare errors and hide when there's a diff, the tsserver reporter instead always reports if the current PR fails, including both traces.

It seems like all of our current spurious outputs (microsoft/TypeScript#52809 (comment)) are cases where the errors have identical IDs and command names.

This PR makes it so that if they are identical, we ignore the problem.

[DanielRosenwasser]

I think we're trying to drive the "known" number down to 0 - so until then, we should continue reporting.

[andrewbranch]

This seems like the right behavior for PRs, though.

Implementation-wise, I don’t know this codebase at all, but it’s not clear to me why command name and request id would be enough to say that two outputs are the same?

[andrewbranch]

Oh, this object only exists if it’s a failure, so the assumption is that if each failed, they’re equal-ish.

[jakebailey]

I think we're trying to drive the "known" number down to 0 - so until then, we should continue reporting.

Sure, but right now, it's spamming PRs with huge amounts of comments and text that have nothing to do with the PRs, and we don't do this at all for tsc, regardless of if it's a PR. I think we need to do something.

Implementation-wise, I don’t know this codebase at all, but it’s not clear to me why command name and request id would be enough to say that two outputs are the same?

It's not, really; I'm struggling to come up with a good mechanism because I can't just stringly-compare the messages. I would need to sanitize them for file/line, at least, but we'd still be sensitive to function name changes too.

I'll see if there's a specific option that's set on PRs, though.

[DanielRosenwasser]

This seems like the right behavior for PRs, though.

I agree with that - the only exception being cases where your PR is supposed to fix these issues.

[jakebailey]

I agree with that - the only exception being cases where your PR is supposed to fix these issues.

If we get rid of the errors, then the existing code here doesn't report anything either! I noticed that too. I can try and fix that here.

        switch (newSpawnResult.code) {
            case 0:
            case null:
                if (newSpawnResult.signal !== null) {
                    console.log(`Exited with signal ${newSpawnResult.signal}`);
                    return { status: "Unknown failure" };
                }

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

[jakebailey]

Okay, I've changed it such that now:

• If both succeed, we report nothing.
• If the new fails, but the old succeeds, we report the new failure.
• If the old fails, but the new succeeds, we report the old failure (collapsed) but say that the new server no longer fails.
• If both fail, then:
  • If in a PR and the sequence number and command match, we report nothing.
  • Otherwise, we report both errors.

[jakebailey]

Ugh, this was wrong because fallthrough! Will fix.