On-the-fly-docs -- Add better testing and provide more context for more cases #78148
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
src/EditorFeatures/CSharpTest/OnTheFlyDocs/OnTheFlyDocsUtilitiesTests.cs
Outdated
Show resolved
Hide resolved
src/Features/CSharp/Portable/QuickInfo/OnTheFlyDocsUtilities.cs
Outdated
Show resolved
Hide resolved
| var solution = workspace.CurrentSolution; | ||
| var document = solution.Projects.First().Documents.First(); | ||
|
|
||
| var syntaxTree = await document.GetSyntaxTreeAsync(); | ||
| var semanticModel = await document.GetSemanticModelAsync(); | ||
|
|
||
| var methodInvocation = syntaxTree!.GetRoot() | ||
| return (solution, document, semanticModel!, syntaxTree!); |
There was a problem hiding this comment.
- this needs to return the workspace. THe caller needs to dispose it at the end.
- no need to return document and solution, the former gives you the latter.
- no need to return semantic model and syntax tree, the former gives you the latter.
- tuples names are camelCased :)
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.CSharp.QuickInfo; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using Microsoft.CodeAnalysis.QuickInfo; |
| Func<SyntaxTree, SemanticModel, ISymbol> getSymbol, | ||
| string? expectedText = null, | ||
| bool expectAllNull = false, | ||
| int resultIndex = 0) |
There was a problem hiding this comment.
instead of resultIndex, just pass the N results you expect through here. the test can assert the right count of result contexts vs expected reuslts, then check all N of them.
| var (solution, _, semanticModel, syntaxTree) = await SetupWorkspaceAsync(testCode); | ||
|
|
||
| var symbol = getSymbol(syntaxTree, semanticModel); | ||
| var result = OnTheFlyDocsUtilities.GetAdditionalOnTheFlyDocsContext(solution, symbol); |
There was a problem hiding this comment.
probably easier to just extract this out to a helper. Then have TestSuccessAsync and TestFailureAsync helper that uses this. instead of trying to jam all values into 'expecteText/expectAllNull/resultIndex' parameterization approach.
|
|
||
| await TestInvocationContextAsync(testCode, customTypeText, resultIndex: 0); | ||
| await TestInvocationContextAsync(testCode, customStructText, resultIndex: 1); | ||
| } |
There was a problem hiding this comment.
the cleanup is very appreciated. consider calling this like so:
| } | |
| await TestInvocationContextAsync(testCode, customTypeText, customStructText); |
| } | ||
|
|
||
| private static async Task TestSymbolContextAsync( | ||
| string testCode, |
There was a problem hiding this comment.
add [StringSyntax] attribute here.
| { | ||
| return TestSymbolContextAsync( | ||
| testCode, | ||
| (syntaxTree, semanticModel) => GetMethodSymbol(syntaxTree, semanticModel), |
There was a problem hiding this comment.
| (syntaxTree, semanticModel) => GetMethodSymbol(syntaxTree, semanticModel), | |
| GetMethodSymbol, |
| expectedText, | ||
| expectAllNull, | ||
| resultIndex); | ||
| } |
There was a problem hiding this comment.
can create single helper fort hsi and the above method that generically is passed a <TSyntaxNode> type arg.
| { | ||
| return elementInfo; | ||
| } | ||
| } |
There was a problem hiding this comment.
whats' the general logic/thinking here? I don't relaly get the idea of why we're just getting the first thing and returning, and ending with that. for example, if i have (string s, Customer c) why do i only get relevant file info for string and nto Customer?
| { | ||
| return constraintInfo; | ||
| } | ||
| } |
| if (typeSymbol is IArrayTypeSymbol arrayTypeSymbol) | ||
| { | ||
| typeSymbol = arrayTypeSymbol.ElementType; | ||
| } |
There was a problem hiding this comment.
IPointerTypeSymbol as well.
| { | ||
| typeSymbol = namedTypeSymbol.TypeArguments[0]; | ||
| } | ||
|
|
||
| var typeSyntaxReference = typeSymbol.DeclaringSyntaxReferences.FirstOrDefault(); |
There was a problem hiding this comment.
same here. what does this mean for partial types?
|
Glad to see tests :) Consider adding tests taht show what happens when the call is in varying contexts: property initializers, accessors, constructors, operators, indexers, methods, local functions, top level functions, lambdas. |
Fixes #78102