Improve performance on SymbolCompletionProviderTests#75496
Improve performance on SymbolCompletionProviderTests#75496CyrusNajmabadi merged 8 commits intodotnet:mainfrom
SymbolCompletionProviderTests#75496Conversation
ghost
added
Area-IDE
untriaged
dotnet-policy-service
bot
added
the
Community
src/EditorFeatures/CSharpTest/Completion/CompletionProviders/SymbolCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
| await VerifyItemIsAbsentAsync(AddUsingDirectives("using System;", AddInsideMethod("string s = \"$$")), @"String"); | ||
| await VerifyItemIsAbsentAsync(AddUsingDirectives("using System;", AddInsideMethod("string s = \"$$")), @"System"); | ||
| var code = AddUsingDirectives("using System;", AddInsideMethod("string s = \"$$")); | ||
| await VerifyExpectedItemsAsync(code, [ |
There was a problem hiding this comment.
In C# opening brace of blocks usually occupies its own line. I would expect something similar for multiline collection expressions, so that [ is on its own line
There was a problem hiding this comment.
InvocationExpressionSignatureHelpProviderTests.cs contains many tests using this bracket style, I think it's okay to leave as is
src/EditorFeatures/CSharpTest/Completion/CompletionProviders/SymbolCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
| CompletionTestExpectedResult.Exists("Equals"), | ||
| CompletionTestExpectedResult.Absent("DoSomething") with | ||
| { | ||
| DisplayTextSuffix = "<>" |
There was a problem hiding this comment.
Why not an optional parameter of CompletionTestExpectedResult.Absent? This would be more consistent with how single assertions work, which specify all properties to verify as parameters
src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
| SourceCodeKind? SourceCodeKind = null) | ||
| { | ||
| public static CompletionTestExpectedResult[] None = CreateGeneralMatchingArray(true); | ||
| public static CompletionTestExpectedResult[] Any = CreateGeneralMatchingArray(false); |
There was a problem hiding this comment.
If these are not intended to be changed (and as far as I can see it is so), please use ImmutableArray
There was a problem hiding this comment.
VerifyAnyItemExistsAsync and VerifyNoItemsExistAsync use those to pass down to VerifyExpectedItemsAsync which takes an array, not ImmutableArray. This was designed in the original PR that way, but without any reason against ImmutableArray for VerifyExpectedItemsAsync.
src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
|
@CyrusNajmabadi ptal |
| return base.VerifyWorkerAsync( | ||
| code, position, usePreviousCharAsTrigger, deletedCharTrigger, hasSuggestionItem, sourceCodeKind, | ||
| expectedResults, matchingFilters, flags, options, skipSpeculation); | ||
| } |
There was a problem hiding this comment.
i don't understand this. it seems to be an overrride, which just calls the base, but passes all the values along. why have this?
There was a problem hiding this comment.
This is the implementation of BaseVerifyWorkerAsync calling the base VerifyWorkerAsync, not the same method. Since we do not override VerifyWorkerAsync here, we just call its base.
There was a problem hiding this comment.
@CyrusNajmabadi if this is okay can we move the PR forward?
4 participants
From #70307, redone in a fresh branch to avoid complex merge conflicts
With approval from @CyrusNajmabadi, many tests in
SymbolCompletionProviderTestswere adjusted with new methods that allow checking for multiple items in the suggestions with a single run, hence improving performance.This PR contains those adjustments for many tests that were the biggest hits, with either many verifications in the same method on the same source, or in test theories that have many distinct combinations. The greatest hit was from pattern matching tests that were added in #70262, which test against a large parameter matrix and assert a lot of recommendations.
Additionally, the new API makes it very convenient to test the recommended items more extensively.
Tests for Visual Basic were not changed in this PR. However, the new APIs were overridden in the abstract recommender, to be used once those are also updated to use the new assertion APIs.
Benchmarks
Running all the tests in
SymbolCompletionProviderTests.cs(about 1110) in my machine, takes:The performance aligns with the theory of each individual
await Verify...doing unnecessary work to parse, etc. up until the completion is triggered, in order to test the presence/absence of the symbol.Notes
The new version removes seemingly unnecessary arguments and batches each individual argument about a specific item in a record. These may be freely updated depending on the requirements for each test.
Test methods that only verify exactly one item do not need to be updated. It will be required though if the same test is extended to test more than one recommendation items.