Extending the supported functionality of the VisualStudio IntegrationTest Utilities and adding the start of 'CSharpAutomaticBraceCompletion' #11223
Conversation
|
|
||
| private void VerifyEditorTextBeforeCursor(string expectedText) => VerifyEditorTextBeforeAndAfterCursor(expectedText, string.Empty); | ||
|
|
||
| private void VerifyEditorTextContains(string expectedText) |
There was a problem hiding this comment.
I would like to see the xUnit primitives being used directly; otherwise it just feels like we're porting our actions in a strange way. Put another way, this adds a layer of indirection which doesn't simplify anything and just increases concept count.
|
Also for review: @dotnet/roslyn-ide. |
| @@ -108,6 +108,7 @@ | |||
| <InternalsVisibleToTest Include="Roslyn.Test.Utilities.Desktop" /> | |||
| <InternalsVisibleToTest Include="Roslyn.VisualStudio.CSharp.UnitTests" /> | |||
| <InternalsVisibleToTest Include="Roslyn.VisualStudio.Services.UnitTests" /> | |||
| <InternalsVisibleToTest Include="Roslyn.VisualStudio.Test.Utilities" /> | |||
There was a problem hiding this comment.
Why? This seems very fishy.
There was a problem hiding this comment.
EditorCompletionOptions.UseSuggestionMode is not available otherwise.
|
Most nitpicks, but I do have significant concerns about some of the waits I see. I would propose that any time we have a "wait for idle" of any kind we clearly state the exact mechanism in VS that we waiting for, ideally with a pointer to the code if we can do so. (I'd even push for mentioning file names if that's OK.) My worry is that much of integration test flakiness comes from poorly understood dependencies, and I'd like to ensure that when reimplementing this we understand it to the best of our ability. |
|
@jasonmalinowski, I believe I have now replied to all feedback so far and updated things appropriately. |
| public InteractiveWindowWrapper(IInteractiveWindow interactiveWindow) | ||
| public static InteractiveWindowWrapper CreateForCSharp() | ||
| { | ||
| var interactiveWindow =RemotingHelper.CSharpInteractiveWindow; |
There was a problem hiding this comment.
inline? (strange spacing in any case)
| User32.SendInput(1, inputs, User32.SizeOf_INPUT); | ||
| } | ||
|
|
||
| private void SendKeyUp(ushort vk) |
There was a problem hiding this comment.
Share code with method above since it's identical except for dwFlags?
|
@tannergooding: did you miss a final push? https://github.com/dotnet/roslyn/pull/11223/files#r62754598 was commented as removed but wasn't. Let's fix the use completion mode command invocation, and then call this good. |
|
@jasonmalinowski, no, the comment just didn't get updated (for some reason). There is a TestHook ( |
|
Also, I will update to use the completion mode command invocation and update (sometime after lunch). |
|
@jasonmalinowski, updated. |
…Test Utilities and adding the start of 'CSharpAutomaticBraceCompletion'
|
@jasonmalinowski, I added a second commit which ensures all DTE calls are wrapped in |
… tend to fail with `RPC_E_CALL_REJECTED`.
| _editorWindow.PlaceCursor("// Marker"); | ||
|
|
||
| await _editorWindow.TypeTextAsync("void Foo("); | ||
| await _editorWindow.TypeTextAsync("\u001B"); // ESC |
There was a problem hiding this comment.
Nitpick: constant please, or support the {ESC} syntax like everybody will expect.
There was a problem hiding this comment.
Fixed. Added the full set of constants previously available.
|
A couple requests for constants, one comment for style, and one bug where your error string isn't what you think it is. Otherwise 👍. |
FYI. @jasonmalinowski, @dotnet/roslyn-infrastructure