-
Notifications
You must be signed in to change notification settings - Fork 737
Port non-baseline diagnostics tests #2079
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ports fourslash diagnostic tests from TypeScript to the native Go implementation. It adds three new verification functions (VerifyDiagnostics, VerifyNonSuggestionDiagnostics, and VerifySuggestionDiagnostics) to enable testing of diagnostics in the language server protocol context, replacing the previous distinction between syntactic, semantic, and suggestion diagnostics with an LSP-based approach.
Key changes:
- Added diagnostic verification methods that filter diagnostics based on whether they are suggestions (using tags and severity)
- Updated the fourslash test converter script to parse diagnostic expectations from TypeScript tests
- Fixed a bug in
selectLineto handle the last line of a file correctly - Added approximately 30 new diagnostic tests
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/fourslash/fourslash.go | Implements the three diagnostic verification functions, adds helper functions for filtering diagnostics and getting ranges in a file, fixes selectLine edge case, and adds diagnostic ignore options for deep equality checks |
| internal/fourslash/_scripts/convertFourslash.mts | Adds parsing logic for diagnostic verification commands (getSemanticDiagnostics, getSuggestionDiagnostics, getSyntacticDiagnostics) and converts them to Go test code |
| internal/fourslash/_scripts/manualTests.txt | Adds parserCorruptionAfterMapInClass to manual tests list |
| internal/fourslash/_scripts/failingTests.txt | Adds TestAutoImportModuleNone1 and TestJsDocAugmentsAndExtends to failing tests list |
| internal/fourslash/tests/manual/parserCorruptionAfterMapInClass_test.go | Manual test for parser corruption after map usage in class |
| internal/fourslash/tests/gen/*.go | 29 generated diagnostic test files covering various scenarios including syntax errors, suggestion diagnostics, and semantic diagnostics |
| } | ||
|
|
||
| func isSuggestionDiagnostic(diag *lsproto.Diagnostic) bool { | ||
| return diag.Tags != nil && len(*diag.Tags) > 0 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions don't necessarily have to have tags; e.g. if we were to do the "convert to ESM" fix, that would just be a hint, no tags. The tags just apply a visual change like strikethrough or greying out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an heuristic, as I think we don't have a consistent way of signaling a diagnostic is a suggestion, right? e.g. the unreachable code diagnostic has a tag, but its severity is Error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is so long as allowUnreachableCode is true; if it's undefined then it's Hint. But if that's the difference you're checking then yeah we can leave it.
This PR ports diagnostics fourslash tests. I think about 60% of tests that rely on
verify.getXDiagnosticsfunctions also rely onverify.codeFixthough, which isn't ported, so the tests in this PR are the remaining 40%.Note that in Strada, we had
verify.getSyntacticDiagnostics,verify.getSemanticDiagnostics, andverify.getSuggestionDiagnostics, but in LSP there is no such distinction. I considered adding some information to the diagnostics we produce so that we could have those three kinds of testing functions, but that seemed not worth the effort, as I think long term we should think of diagnostics in the terms set by LSP, and there aren't that many diagnostics tests to start with.So instead I've added
VerifyDiagnostics,VerifySuggestionDiagnostics, andVerifyNonSuggestionDiagnostics.VerifyDiagnosticsis for verifying all diagnostics for a file, and what I think we should use for new tests in Corsa, whileVerifyNonSuggestionDiagnosticsis likeverify.getSyntacticDiagnostics+verify.getSemanticDiagnostics, and is implemented by filtering out the diagnostics sent by server that look like suggestion diagnostics, based on their severity and tags. This distinction was enough to get all but one ported test passing (modulo ones that fail for non-diagnostics reasons), and I think it's useful because it's common for a test to have unused identifier errors that don't really matter for the test, and those end up as suggestions.I'll add baseline diagnostics in my next PR.