Port BasicNavigationBar.CodeSpit to the new test harness #57738

sharwell · 2021-11-12T20:58:27Z

azure-pipelines · 2021-11-15T15:15:50Z

Azure Pipelines successfully started running 4 pipeline(s).

dibarbet · 2021-11-16T18:59:51Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/InputInProcess.cs

+        internal async Task SendAsync(params object[] keys)
+        {
+            // AbstractSendKeys runs synchronously, so switch to a background thread before the call
+            await TaskScheduler.Default;


does SendKeys not need to run on the main thread then? It seems like its calling native code so I would have assumed it did

does SendKeys not need to run on the main thread then?

➡️ No. Eventually we'll have a proper async implementation here, but for now it's reusing some synchronous code from the old harness, and we can't block the main thread while that code runs.

dibarbet · 2021-11-16T19:05:19Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorVerifierInProcess.cs

+
+            var expectedCaretMarkupEndIndex = expectedCaretIndex + "$$".Length;
+
+            var expectedTextBeforeCaret = expectedText.Substring(0, expectedCaretIndex);


nit - maybe have a small helper to extract the text w/out the $$ since it looks like you do it here and on line 129

➡️ It looks like this is cleaned up a bit in a pull request I'm submitting after this one, so I'll leave it here for now.

dibarbet · 2021-11-16T19:12:17Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+                riid = typeof(IVsCodeWindow).GUID;
+                if (ErrorHandler.Failed(frame.QueryViewInterface(ref riid, out ppvObject)) || ppvObject == IntPtr.Zero)
+                {
+                    return null;


would it be useful at all to have logs on failure cases here, or do the dumps typically contain all the information we'd need to verify which of these error cases we're hitting?

➡️ It definitely wouldn't hurt, especially considering the next like after calling TryGetCodeWindow is always Assumes.Present. For now I'm OK keeping this form for two reasons:

This code historically has not failed (copied from older harness)

In the event of failure, we'll get a message saying IVsCodeWindow was expected but not available, and at that point it would be easy to update the implementation to better explain the point of failure

In the event of failure, we'll get a message saying IVsCodeWindow was expected but not available, and at that point it would be easy to update the implementation to better explain the point of failure

taht wfm. it's generally waht i've done when there are issues and the logs aren't sufficient.

CyrusNajmabadi · 2021-11-17T17:47:43Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            await ExpandNavigationBarAsync(index: 2, cancellationToken);
+        }
+
+        public async Task ExpandNavigationBarAsync(int index, CancellationToken cancellationToken)


make private?

➡️ Eliminated the intermediate callers and left this public

CyrusNajmabadi · 2021-11-17T17:48:09Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            return await GetNavigationBarItemsAsync(index: 2, cancellationToken);
+        }
+
+        public async Task<ImmutableArray<string>> GetNavigationBarItemsAsync(int index, CancellationToken cancellationToken)


make private?

➡️ Eliminated the intermediate callers and left this public

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

CyrusNajmabadi · 2021-11-17T17:48:56Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            var itemIndex = await GetNavigationBarItemIndexAsync(index, item, cancellationToken);
+            if (itemIndex < 0)
+            {
+                throw new ArgumentException($"Could not find '{item}' in combobox");


can you have this report the set of items that were there?

➡️ Now using Assert.Contains on the failure path

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

CyrusNajmabadi · 2021-11-17T17:50:02Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            await JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
+
+            var margin = await GetNavigationBarMarginAsync(textView, cancellationToken);
+            return margin.GetFieldValue<List<ComboBox>>("_combos");


well that's terrifying :)

➡️ Copied from old test harness. Happy to use a new approach if we find one.

CyrusNajmabadi · 2021-11-17T17:52:10Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            return margin.GetFieldValue<List<ComboBox>>("_combos");
+        }
+
+        private async Task<UIElement?> GetNavigationBarMarginAsync(IWpfTextView textView, CancellationToken cancellationToken)


who do we send feedback to that VS/editor needs a proper, resilient, testing API so we can do this sort of validation without this stuff?

Maybe @AmadeusW ?

CyrusNajmabadi · 2021-11-17T17:54:49Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorVerifierInProcess.cs

+                    lineText = lineText.Trim();
+                    lineTextBeforeCaret = lineTextBeforeCaret.TrimStart();
+                    lineTextAfterCaret = lineTextAfterCaret.TrimEnd();
+                }


something feels weird here, but i can't put my finger on it. I can't determine why we would have different logic on how to trim the whitespace depending on where the caret is... i do not like this, and it feels to me that some bug is lurking within this.

➡️ trimWhitespace is a bad idea all around in these tests. I'll take this opportunity to remove it.

CyrusNajmabadi · 2021-11-17T17:56:19Z

So what'st he core change(s) here that makes the new test better than teh previous one? thanks!

sharwell · 2021-11-17T22:00:47Z

So what'st he core change(s) here that makes the new test better than teh previous one? thanks!

This is covered by the PR summary in #57626

Fixes a failure to wait for asynchronous operations following a selection in the navigation bars. Fixes dotnet#57719

CyrusNajmabadi · 2021-11-18T23:05:49Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs

+            if (Version.Parse("17.1.31916.450") > await TestServices.Shell.GetVersionAsync(cancellationToken))
+            {
+                // Workaround for extremely unstable async lightbulb prior to:
+                // https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform/pullrequest/361759


can we track removing this somehow?

This code will disable itself at the appropriate time when integration test machines update, so I probably wouldn't bother tracking separately. If we file an issue, it will likely just end up in the backlog and never get seen again, where it may or may not be updated after someone eventually removes this block.

I'd probably take a different approach if this was production code.

CyrusNajmabadi · 2021-11-18T23:07:15Z

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/StateResetInProcess.cs

+        {
+            _ = cancellationToken;
+
+            return Task.CompletedTask;


did i miss where this is implemented somewhere?

No, this is a placeholder for now.

CyrusNajmabadi

small bits of feedback. but nothing that woudl block this.

dotnet-issue-labeler bot added the Area-IDE label Nov 12, 2021

runfoapp bot mentioned this pull request Nov 15, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

sharwell force-pushed the vs-extension-testing branch from ed556ec to ac28bdd Compare November 16, 2021 17:32

sharwell marked this pull request as ready for review November 16, 2021 17:33

sharwell requested a review from a team as a code owner November 16, 2021 17:33

dibarbet approved these changes Nov 16, 2021

View reviewed changes

runfoapp bot mentioned this pull request Nov 16, 2021

Flaky test: Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpFindReferences.FindReferencesToCtor #57552

Closed

CyrusNajmabadi reviewed Nov 17, 2021

View reviewed changes

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 17, 2021

View reviewed changes

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Nov 17, 2021

View reviewed changes

sharwell added 2 commits November 17, 2021 12:50

Port BasicNavigationBar.CodeSpit to the new test harness

c97882e

Stabilize InvokeCodeActionListAsync prior to 17.1 Preview 2

72ee59c

sharwell force-pushed the vs-extension-testing branch from ac28bdd to 72ee59c Compare November 17, 2021 20:51

Simplify API for testing navigation bars

30d2570

sharwell force-pushed the vs-extension-testing branch from f962a07 to 30d2570 Compare November 17, 2021 21:00

sharwell added 2 commits November 17, 2021 14:05

Stabilize integration test BasicNavigationBar.CodeSpit

f2ed9fc

Fixes a failure to wait for asynchronous operations following a selection in the navigation bars. Fixes dotnet#57719

Remove 'trimWhitespace' flag from integration tests

3c57e4b

CyrusNajmabadi reviewed Nov 18, 2021

View reviewed changes

CyrusNajmabadi approved these changes Nov 18, 2021

View reviewed changes

sharwell merged commit 5a99085 into dotnet:main Nov 22, 2021

sharwell deleted the vs-extension-testing branch November 22, 2021 16:25

ghost added this to the Next milestone Nov 22, 2021

allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port BasicNavigationBar.CodeSpit to the new test harness #57738

Port BasicNavigationBar.CodeSpit to the new test harness #57738

sharwell commented Nov 12, 2021 •

edited

Loading

azure-pipelines bot commented Nov 15, 2021

dibarbet Nov 16, 2021

sharwell Nov 17, 2021 •

edited

Loading

dibarbet Nov 16, 2021

sharwell Nov 17, 2021

dibarbet Nov 16, 2021

sharwell Nov 17, 2021

CyrusNajmabadi Nov 18, 2021

CyrusNajmabadi Nov 17, 2021

sharwell Nov 17, 2021

CyrusNajmabadi Nov 17, 2021

sharwell Nov 17, 2021

CyrusNajmabadi Nov 17, 2021

sharwell Nov 17, 2021

CyrusNajmabadi Nov 17, 2021

sharwell Nov 17, 2021

CyrusNajmabadi Nov 17, 2021

CyrusNajmabadi Nov 17, 2021

sharwell Nov 18, 2021

CyrusNajmabadi Nov 17, 2021

sharwell Nov 18, 2021 •

edited

Loading

CyrusNajmabadi commented Nov 17, 2021

sharwell commented Nov 17, 2021

CyrusNajmabadi Nov 18, 2021

sharwell Nov 19, 2021

CyrusNajmabadi Nov 18, 2021

sharwell Nov 19, 2021

CyrusNajmabadi left a comment


		var expectedCaretMarkupEndIndex = expectedCaretIndex + "$$".Length;

		var expectedTextBeforeCaret = expectedText.Substring(0, expectedCaretIndex);

Port BasicNavigationBar.CodeSpit to the new test harness #57738

Port BasicNavigationBar.CodeSpit to the new test harness #57738

Conversation

sharwell commented Nov 12, 2021 • edited Loading

azure-pipelines bot commented Nov 15, 2021

Choose a reason for hiding this comment

sharwell Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 17, 2021

sharwell commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

sharwell commented Nov 12, 2021 •

edited

Loading

sharwell Nov 17, 2021 •

edited

Loading

sharwell Nov 18, 2021 •

edited

Loading