-
Notifications
You must be signed in to change notification settings - Fork 156
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
Add CodeActions for Number Constants: Convert between bases, Add digit group separators #1167
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
d8a30da
Log CodeFix exceptions
Booksbaum 2a3e3aa
Add: CodeActions for Number Constants
Booksbaum acb3dbc
Simplify float sign handling
Booksbaum 00be5fa
Add QuickFix: Replace with Name (MinValue, MaxValue, Epsilon)
Booksbaum 08e0a8c
Fix: Adding a sign might lead to invalid code
Booksbaum 280407d
Fix: No Space added before sign when inserting `-infinity` immediatel…
Booksbaum 4bbb2f9
Add signature file
Booksbaum 4628f1c
Handle `SynExpr` in Enum
Booksbaum a15ea1d
Add Quotation Mark in comment
Booksbaum 06a8e0c
Fix: Fantomas doesn't pick up editorconfig
Booksbaum 7b2c9aa
Formatting
Booksbaum e6ce3bc
Fix: Produces unescaped single quotation mark for char
Booksbaum 99e3c94
Apply fantomas formatting
Booksbaum ddef552
Format all files
Booksbaum 6014bc8
Increase CI timeout to 40min
Booksbaum File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,36 @@ | ||
{ | ||
"version": 1, | ||
"isRoot": true, | ||
"tools": { | ||
"fake-cli": { | ||
"version": "5.23.0", | ||
"commands": [ | ||
"fake" | ||
] | ||
}, | ||
"paket": { | ||
"version": "7.2.1", | ||
"commands": [ | ||
"paket" | ||
] | ||
}, | ||
"octonav": { | ||
"version": "0.0.1", | ||
"commands": [ | ||
"octonav" | ||
] | ||
}, | ||
"dotnet-reportgenerator-globaltool": { | ||
"version": "5.0.2", | ||
"commands": [ | ||
"reportgenerator" | ||
] | ||
}, | ||
"fantomas": { | ||
"version": "6.1.0", | ||
"commands": [ | ||
"fantomas" | ||
] | ||
} | ||
} | ||
} | ||
{ | ||
"version": 1, | ||
"isRoot": true, | ||
"tools": { | ||
"fake-cli": { | ||
"version": "5.23.0", | ||
"commands": [ | ||
"fake" | ||
] | ||
}, | ||
"paket": { | ||
"version": "7.2.1", | ||
"commands": [ | ||
"paket" | ||
] | ||
}, | ||
"octonav": { | ||
"version": "0.0.1", | ||
"commands": [ | ||
"octonav" | ||
] | ||
}, | ||
"dotnet-reportgenerator-globaltool": { | ||
"version": "5.0.2", | ||
"commands": [ | ||
"reportgenerator" | ||
] | ||
}, | ||
"fantomas": { | ||
"version": "6.2.0", | ||
"commands": [ | ||
"fantomas" | ||
] | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 concerns me a tiny bit that we keep climbing on this :(
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.
I basically doubled the test suite with
NamedText
/RoslynSourceText
. Might be worth switching toRoslynSourceText
by default then eventually removingNamedText
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.
ok, so this isn't because individual tests are hanging, it's because the suite overall is going long? that's more understandable. I'd agree with both of the things you mention.
I'm guessing there's some Expecto cancellation weirdness that's preventing us from having a truly per-test timeout?
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.
I think this does work (I see some tests fail at 2 mins) but we were hitting the overall timeout we set on the CI. I know I had to make a lot of tests sequential to keep the test suite passing consistently and that's a big part of it being slow too. Probably need to revisit the setup and see why things tend to not be thread safe. Might simply be we're not making an LSP server per test or something else.
I do vaguely remember the "wait for parse/check notifications" is how a lot of tests know how to continue after requesting a typecheck which can fail and not provide the correct feedback or gets lost in the logging.
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.
Yes, individual timeouts do work. It's "just" the overall timeout we run into.
I think that's reasonable. They should act the same, so just using one should be enough (and then maybe some tests to ensure they really act the same). Currently we're practically doing all tests twice -- despite not actually testing the
XXXText
, but the server functionality.Locally the first thing I do before running tests is always uncomment
NamedText
-- which basically cuts the test time in halve.Other "multipliers" are already uncommented ([1] [2]).
I think it's just the initialization and destruction of LSP server that must be sequential. The tests themselves should be able to run in parallel -- but are currently in sequenced. (Though moving tests into a nested
testList
doesn't seem to run them in parallel/affect performance? -> linked code doesn't seem to be limiting factor :/ )Though there might also some issues with getting the correct notifications when tests are run in parallel (I think I remember an issue with that -- though might also be something outdated. I think notifications are now filter by file. So notifications should be assigned to correct tests.)
We're already sharing a server per "logical group" (for example: per CodeFix -- but not for each individual test inside each CodeFix).
Though pushing that further outward might be good for speed. I remember just reusing a document (
docTestList
) has some noticeable (but not huge) impact. And a single doc should be way more light than a full LSP server.We could use just one LSP server for all tests. But we're creating a lot of documents -- which I think get all cached in server? Also notifications from LSP Server to test environment are all cached. So might not actually be faster, but more memory intensive -- and definitely more difficult to debug.
Also: some tests require extra settings -> must get their own LSP server.
I think keeping the existing "One LSP server per logical group" makes most sense: State limited to logical context. And LSP settings explicit for that context.
(Though some behind the scene server caching and clearing a server state instead of throwing away might be reasonable too?)
Yes. Inclusive a required short delay even when correct notification already arrived ... just to ensure it's really the latest notification. Over 100s and 1000s of tests this adds up...
I think getting rid of this notification stream would be the best for tests: Difficult to filter & debug, might be out of order, lots of waiting to ensure/hope correct messages, lots of caching of old messages.
Replacing this with direct calls and direct returns would be great -- but then also not the LSP way :/
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.
Yeah it would be something to test. If an LSP server per test is more stable, is it worth the extra 10% in time it takes to do the init/shutdown dance? Hard to say unless we try it out. Although I don't think it's our biggest source of slowness. I think sequenced combined with my findings below is the painful combination.
Yeah the biggest slowness I think is the
|> Observable.bufferSpan (timeout)
as we have to wait for that full timeout. It would probably be better to have to pass in your desired criteria and only wait as long as we need. Something likeObservable.choose ... |> Observable.timeout
. This would probably give another big boost in speeding things up.We could move to some pull based diagnostics and poll until we get what we need but I fear we might end up not "truly" testing how an LSP is used from a client.
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.
The rest of the discussion is too long, didn't read... but we should do that, and also switch to the Adaptive server, and then remove the old version.