Skip to content

Remove reliance on "questionable" UpdateFile operation#52441

Merged
sharwell merged 1 commit intodotnet:mainfrom
sharwell:update-file
Apr 7, 2021
Merged

Remove reliance on "questionable" UpdateFile operation#52441
sharwell merged 1 commit intodotnet:mainfrom
sharwell:update-file

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Apr 6, 2021 •
edited
Loading

During investigation of #52409, logs were observed where the editor text did not appear to match the text set up by InitializeAsync (e.g. #52441 (comment) below). Since the InitializeAsync method set the text using a somewhat unique method, and other tests aren't showing signs of incorrect values, we modify the problematic test to set the editor text using the same coding pattern that other non-problematic tests are using.

The failure has not been locally reproducible, so it's not clear why (or to some degree if) the UpdateFile call is failing in this scenario.

The new code follows the approach of AbstractEditorTest.SetUpEditor.

Closes #52386

@sharwell sharwell requested a review from a team as a code owner April 6, 2021 19:59
@ghost ghost added the Area-Interactive label Apr 6, 2021
@jasonmalinowski
Copy link
Copy Markdown
Member

jasonmalinowski commented Apr 6, 2021

What is making UpdateFile unreliable? Should we remove it?

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Apr 6, 2021 •
edited
Loading

What is making UpdateFile unreliable

No idea. It's just a guess based on logs like this:

System.AggregateException: One or more errors occurred. ---> System.Exception: Marker '/* 1 */' not found in text: using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace TestProj
{
    class Program
    {
        static void Main(string[] args)
        {
        }
    }
}

   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.TextViewWindow_InProc.<>c__DisplayClass8_0.<PlaceCaret>b__0(IWpfTextView view) in /_/src/VisualStudio/IntegrationTest/TestUtilities/InProcess/TextViewWindow_InProc.cs:line 190
   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.TextViewWindow_InProc.<>c__DisplayClass13_0.<GetExecuteOnActionViewCallback>b__0(CancellationToken cancellationToken) in /_/src/VisualStudio/IntegrationTest/TestUtilities/InProcess/TextViewWindow_InProc.cs:line 257
   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.InProcComponent.<>c__DisplayClass6_0.<<InvokeOnUIThread>b__0>d.MoveNext() in /_/src/VisualStudio/IntegrationTest/TestUtilities/InProcess/InProcComponent.cs:line 59
   --- End of inner exception stack trace ---

Server stack trace: 
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.InProcComponent.InvokeOnUIThread(Action`1 action)
   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.TextViewWindow_InProc.PlaceCaret(String marker, Int32 charsOffset, Int32 occurrence, Boolean extendSelection, Boolean selectBlock) in /_/src/VisualStudio/IntegrationTest/TestUtilities/InProcess/TextViewWindow_InProc.cs:line 160
   at System.Runtime.Remoting.Messaging.StackBuilderSink._PrivateProcessMessage(IntPtr md, Object[] args, Object server, Object[]& outArgs)
   at System.Runtime.Remoting.Messaging.StackBuilderSink.SyncProcessMessage(IMessage msg)

Exception rethrown at [0]: 
   at System.Runtime.Remoting.Proxies.RealProxy.HandleReturnMessage(IMessage reqMsg, IMessage retMsg)
---> (Inner Exception #0) System.Exception: Marker '/* 1 */' not found in text: using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace TestProj
{
    class Program
    {
        static void Main(string[] args)
        {
        }
    }
}

   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.TextViewWindow_InProc.<>c__DisplayClass8_0.<PlaceCaret>b__0(IWpfTextView view) in /_/src/VisualStudio/IntegrationTest/TestUtilities/InProcess/TextViewWindow_InProc.cs:line 190
   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.TextViewWindow_InProc.<>c__DisplayClass13_0.<GetExecuteOnActionViewCallback>b__0(CancellationToken cancellationToken) in /_/src/VisualStudio/IntegrationTest/TestUtilities/InProcess/TextViewWindow_InProc.cs:line 257
   at Microsoft.VisualStudio.IntegrationTest.Utilities.InProcess.InProcComponent.<>c__DisplayClass6_0.<<InvokeOnUIThread>b__0>d.MoveNext() in /_/src/VisualStudio/IntegrationTest/TestUtilities/InProcess/InProcComponent.cs:line 59<---

image

Should we remove it?

I wanted to remove it, but it's still used by another test.

@sharwell sharwell enabled auto-merge April 6, 2021 21:50
jasonmalinowski
jasonmalinowski requested changes Apr 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if we think this is unsafe to use, then we should remove it and update tests. Or we don't have the evidence this is actually problematic then we should clarify the PR title accordingly.

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Apr 7, 2021

@jasonmalinowski The code is used in three locations. This is the only location where tests in the last 30 days failed to achieve the desired outcome following the call. The issue has not been locally reproducible, and we have no evidence suggesting the reason why this call fails at this location. This pull request converts an operation which does not always produce the expected results (unreliable) to another form used elsewhere that does appear to produce the expected results.

@tmat
Copy link
Copy Markdown
Member

tmat commented Apr 7, 2021

@jasonmalinowski I believe this addresses some of the issues reported in #52409

@sharwell sharwell changed the title Remove reliance on unreliable UpdateFile operation Remove reliance on "questionable" UpdateFile operation Apr 7, 2021
@sharwell sharwell mentioned this pull request Apr 7, 2021
Closed
@sharwell sharwell merged commit 29bb4e8 into dotnet:main Apr 7, 2021
@ghost ghost added this to the Next milestone Apr 7, 2021
@sharwell sharwell deleted the update-file branch April 7, 2021 19:05
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat approved these changes

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

Assignees

No one assigned

Labels

Area-Interactive

Projects

None yet

Milestone

16.10.P3

Development

Successfully merging this pull request may close these issues.

CSharpSendToInteractive tests fail when editor text fails to initialize

4 participants

@sharwell @jasonmalinowski @tmat @dibarbet