Have CompleteStatement handle EOF statements#83205
Conversation
Agent-Logs-Url: https://github.com/dotnet/roslyn/sessions/33cec84c-4105-42df-9d1a-53427e6e70ab Co-authored-by: mwiemer-microsoft <80539004+mwiemer-microsoft@users.noreply.github.com>
|
They're good tests but they don't pass lol, we need to fix the actual bug first. |
Needs tests
mwiemer-microsoft
changed the title
|
@copilot add tests for the other sections of code I modified, try to test the public functions directly instead of going through the command handler |
…n boundary conditions Agent-Logs-Url: https://github.com/dotnet/roslyn/sessions/7e632c50-f6c2-4baa-be77-58e0b6502157 Co-authored-by: mwiemer-microsoft <80539004+mwiemer-microsoft@users.noreply.github.com>
The bug is fixed in commit
Added 6 tests in
|
|
The four
|
| Assert.NotNull(finalMethodDecl); | ||
| Assert.NotEqual(finalMethodDecl, methodDecl); | ||
| } | ||
|
|
There was a problem hiding this comment.
with the changes reverted in SyntaxNodeExtensions these tests can also be reverted
There was a problem hiding this comment.
I think more tests are good, would you prefer these come in a separate PR though? I specifically kept the non-controversial behavior ones only.
There was a problem hiding this comment.
If we need more tests then it'd be good to have a separate PR explaining that -- otherwise it's odd why we're adding tests for helpers that we're not really changing.
|
Test failure on cdf6cdf was transient Helix failure, should be resolved in this run |
| public void FindTokenOnRightOfPosition_AtStartOfFile_ReturnsTokenAtStartOfFile() | ||
| { | ||
| var root = GetRootForFindTokenTests(); | ||
| var token = root.FindTokenOnRightOfPosition(root.FullSpan.Start); |
There was a problem hiding this comment.
why no FindTokenOnRightOfPosition for root.FullSpan.End?
There was a problem hiding this comment.
we are discussing internally, it will be a separate PR if it's added :)
There was a problem hiding this comment.
It's one more test here? Why a whole different pr for that?
…ommandHandler.cs Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
….com/dotnet/roslyn into copilot/fix-complete-statement-issue
| Assert.NotNull(finalMethodDecl); | ||
| Assert.NotEqual(finalMethodDecl, methodDecl); | ||
| } | ||
|
|
There was a problem hiding this comment.
If we need more tests then it'd be good to have a separate PR explaining that -- otherwise it's odd why we're adding tests for helpers that we're not really changing.
|
Last failure looks like transient Helix issue:
Going to manually trigger re-run of the failing job and work with coworkers to confirm:
|
…ture * upstream/main: (77 commits) Fix ArgumentNullException in VB Edit and Continue (dotnet#83250) Fix property pattern completion filtering out member being edited (dotnet#83230) Add branch merge skill (dotnet#83229) [main] Source code updates from dotnet/dotnet (dotnet#83215) Support MatchPriority comparison in LSP completion (dotnet#83164) Have CompleteStatement handle EOF statements (dotnet#83205) Minor cleanups related to attributes in VB (dotnet#83206) Simplify Address additional PR feedback Port remaining unit test projects to Linux (dotnet#83153) Unsafe evolution: allow unsafe property accessors (dotnet#83115) Address PR review feedback Allow cohost rename in Razor source-generated docs Refine code review skill (dotnet#82666) Review feedback Allow creation of DocumentUri instances even if System.Uri cannot parse it Improve TextLine and line table performance by packing existing data into unused bits (dotnet#83000) Skip TestFindReferencesAsync_UsingAlias on non-Windows platforms (dotnet#83188) [main] Source code updates from dotnet/dotnet (dotnet#83174) fix comment ...
@mwiemer-microsoft authored:
When there was absolutely no whitespace or trivia after the statement to complete, the complete statement handler would fail with an exception. This PR adds a guard against checking for tokens on the right of the last token in the file to fix this. We also had to update some test code to handle unnoticed exceptions.
This PR also adds some tests to the FindTokenOnRightOfPosition and FindTokenOnLeftOfPosition functions. Those functions implicitly throw when given some invalid positions, a behavior not previously documented, but maybe necessary for other parts of the codebase.