Default expected sortText to LocationPriority when verifying completions
#1264
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the fourslash tests so that they now verify that the expected completion item's sortText defaults to LocationPriority when not provided. In addition, the t.Skip() calls have been removed from several test files to allow these tests to run.
- Removed t.Skip() calls in multiple fourslash test files.
- Added an assertion in fourslash.go to verify that SortText falls back to LocationPriority.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/fourslash/tests/gen/completionsWithOptionalPropertiesGenericPartial3_test.go | Removed t.Skip() to run the test |
| internal/fourslash/tests/gen/completionsPropertiesWithPromiseUnionType_test.go | Removed t.Skip() to run the test |
| internal/fourslash/tests/gen/completionsObjectLiteralUnionTemplateLiteralType_test.go | Removed t.Skip() to run the test |
| internal/fourslash/tests/gen/completionsObjectLiteralUnionStringMappingType_test.go | Removed t.Skip() to run the test |
| internal/fourslash/tests/gen/completionsGenericIndexedAccess6_test.go | Removed t.Skip() to run the test |
| internal/fourslash/tests/gen/completionsGenericIndexedAccess2_test.go | Removed t.Skip() to run the test |
| internal/fourslash/tests/gen/completionsGenericIndexedAccess1_test.go | Removed t.Skip() to run the test |
| internal/fourslash/tests/gen/completionsConditionalMember_test.go | Removed t.Skip() to run the test |
| internal/fourslash/fourslash.go | Added assertion for SortText default check |
| }, | ||
| cmp.Ignore(), | ||
| ) | ||
| assertDeepEqual(t, actual, expected, prefix, ignoreKind) | ||
| if expected.Kind != nil { | ||
| assertDeepEqual(t, actual.Kind, expected.Kind, prefix+" Kind mismatch") | ||
| } | ||
| assertDeepEqual(t, actual.SortText, core.OrElse(expected.SortText, ptrTo(string(ls.SortTextLocationPriority))), prefix+" SortText mismatch") |
There was a problem hiding this comment.
|
I wonder if instead of doing this, it wouldn't be better to just have the conversion script add the default sort text explicitly when it's missing in the original Strada tests. |
|
@gabritto I considered this but figured out it will be annoying to include this explicitly in all tests once u start adding new tests manually to Corsa. If you prefer that though - let me know, I can certainly switch this PR to do that instead |
|
Yeah, I think defaulting to something is more convenient for writing the tests, but might be more confusing for reading them. I've been trying to make the fourslash behavior more consistent in terms of which properties we ignore, which we default etc, but I think that's impossible, sadly. |
No description provided.