Further work to stabilize the integration test interop layer by sharwell · Pull Request #25455 · dotnet/roslyn

sharwell · 2018-03-13T23:30:48Z

Use IMessageFilter for consistent COM retry semantics
~~Make DebugAssertFailureException serializable~~
Avoid manual message pumping in HostWaitHelper
Initial work to retry UI automation calls if/when they fail with UIA_E_ELEMENTNOTAVAILABLE
Make ThrowingTraceListener use InvalidOperationException instead of DebugAssertFailureException

…t attempt fails

…ABLE

sharwell · 2018-03-30T03:30:06Z

Looks like 54330a6 only fixed half the problem with figuring out what exception is thrown.

Before my change we got this exception:

System.Runtime.Serialization.SerializationException: Type 'Microsoft.CodeAnalysis.ThrowingTraceListener+DebugAssertFailureException' in Assembly 'Roslyn.Test.Utilities, Version=42.42.42.42, Culture=neutral, PublicKeyToken=fc793a00266884fb' is not marked as serializable.
   at System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo.InitSerialize(Object obj, ISurrogateSelector surrogateSelector, StreamingContext context, SerObjectInfoInit serObjectInfoInit, IFormatterConverter converter, ObjectWriter objectWriter, SerializationBinder binder)
   at System.Runtime.Serialization.Formatters.Binary.WriteObjectInfo.Serialize(Object obj, ISurrogateSelector surrogateSelector, StreamingContext context, SerObjectInfoInit serObjectInfoInit, IFormatterConverter converter, ObjectWriter objectWriter, SerializationBinder binder)
   at System.Runtime.Serialization.Formatters.Binary.ObjectWriter.Serialize(Object graph, Header[] inHeaders, __BinaryWriter serWriter, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize(Stream serializationStream, Object graph, Header[] headers, Boolean fCheck)
   at System.Runtime.Remoting.Channels.CrossAppDomainSerializer.SerializeObject(Object obj, MemoryStream stm)
   at System.AppDomain.Serialize(Object o)
   at System.AppDomain.MarshalObject(Object o)

After my change in 54330a6 we got this exception:

System.Runtime.Serialization.SerializationException: Unable to find assembly 'Roslyn.Test.Utilities, Version=42.42.42.42, Culture=neutral, PublicKeyToken=fc793a00266884fb'.
   at System.Runtime.Serialization.Formatters.Binary.BinaryAssemblyInfo.GetAssembly()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.GetType(BinaryAssemblyInfo assemblyInfo, String name)
   at System.Runtime.Serialization.Formatters.Binary.ObjectMap..ctor(String objectName, String[] memberNames, BinaryTypeEnum[] binaryTypeEnumA, Object[] typeInformationA, Int32[] memberAssemIds, ObjectReader objectReader, Int32 objectId, BinaryAssemblyInfo assemblyInfo, SizedArray assemIdToAssemblyTable)
   at System.Runtime.Serialization.Formatters.Binary.__BinaryParser.ReadObjectWithMapTyped(BinaryObjectWithMapTyped record)
   at System.Runtime.Serialization.Formatters.Binary.__BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(HeaderHandler handler, __BinaryParser serParser, Boolean fCheck, Boolean isCrossAppDomain, IMethodCallMessage methodCallMessage)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, HeaderHandler handler, Boolean fCheck, Boolean isCrossAppDomain, IMethodCallMessage methodCallMessage)
   at System.Runtime.Remoting.Channels.CrossAppDomainSerializer.DeserializeObject(MemoryStream stm)
   at System.AppDomain.Deserialize(Byte[] blob)
   at System.AppDomain.UnmarshalObject(Byte[] blob)

Now I've switch to just using InvalidOperationException.

… failures

333fred · 2018-03-30T22:39:43Z

+
+        private static void RetryIfNotAvailable<T>(Action<T> action, T state)
+        {
+            for (var i = 0; true; i++)


true [](start = 28, length = 4)

I'd rather you make this explicitly i < AutomationRetryCount, and throw an unreachable exception after the try/catch. It took me a minute of verifying exit conditions to actually be sure this wasn't an infinite loop, and that pattern will much easier to reason about your expectations.

💭 The issue I encountered here is it doesn't change the way the exception filter is used, so technically such a change would increase the cyclomatic complexity of the method but not actually add any reachable paths.

It might increase some complexity of the method, but imo would result in code that is more easily understandable and verifiable.

In reply to: 178400103 [](ancestors = 178400103)

➡️ I went with a documentation approach instead. Hopefully it's enough to make it reasonably clear.

333fred · 2018-03-30T22:44:07Z

+                var view = GetActiveTextView();
                var broker = GetComponentModel().GetService<ILightBulbBroker>();
-                return GetLightBulbActions(broker, view).Select(a => a.DisplayText).ToArray();
+                return (await GetLightBulbActionsAsync(broker, view)).Select(a => a.DisplayText).ToArray();


Missing a few explicit ConfigureAwaits in this file.

333fred · 2018-03-30T22:46:03Z

+            if (blockUntilComplete)
            {
-                BeginInvokeExecuteOnActiveView(lightBulbAction);
+                task.Join();


Is there a way to add an assert here that we're not currently on the main thread?

➡️ This is JoinableTask.Join; there is no caller affinity restriction.

333fred

Done review (Iteration 6).

The implementations of ISuggestedAction use threading assertions instead of automatic transition to the correct thread, placing the burden of correct thread selection on the caller, TextViewWindow_InProc. This change switches to the vs-threading approach for maintaining thread affinity, which has two parts: 1. When entering thread-affinitized code, switch to the correct thread 2. Avoid using ConfigureAwait(false) to lose the required affinity The entire file relies on the default behavior of ConfigureAwait, which is the normal coding practice in asynchronous code following vs-threading patterns.

sharwell · 2018-03-31T13:04:59Z

💭 Failure in CSharpCodeActions.GenerateMethodInClosedFile looks a code action is incorrectly reporting that it has no changes to apply (empty preview).
https://ci.dot.net/job/dotnet_roslyn/job/dev15.7.x/job/windows_debug_vs-integration_prtest/266/

    Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile
      Assert.Equal() Failure
                    ↓ (pos 2)
      Expected: \r\nusing System;\r\n\r\npublic class Foo\r\n{\r\n   Â·Â·Â·
      Actual:   \r\npublic class Foo\r\n{\r\n}\r\n
                    ↑ (pos 2)
      Stack Trace:
           at Microsoft.VisualStudio.IntegrationTest.Utilities.OutOfProcess.SolutionExplorer_OutOfProc.Verifier.FileContents(Project project, String fileName, String expectedContents)
           at Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile()

sharwell · 2018-03-31T14:27:11Z

Merging this test-only change to unblock ongoing test stabilization work. I alerted @333fred to the change in ba524a7 (which reverts something he requested in review) and we can review alternatives in the upcoming week if a change needs to be made.

See dotnet/roslyn#25455

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 13, 2018

sharwell requested a review from a team as a code owner March 13, 2018 23:30

sharwell force-pushed the com-message-filter branch 2 times, most recently from 6ef54ea to a4e53e7 Compare March 14, 2018 02:43

sharwell mentioned this pull request Mar 14, 2018

Run integration tests on an STA thread #25467

Merged

Install a COM message filter to handle retry operations when the firs…

08fee02

…t attempt fails

sharwell force-pushed the com-message-filter branch from a4e53e7 to 08fee02 Compare March 29, 2018 16:37

sharwell added 4 commits March 29, 2018 12:35

Make DebugAssertFailureException serializable in all available builds

54330a6

Replace HostWaitHelper (integration tests) with asynchronous code

b9180c3

Retry calls to GetCurrentPattern if they return UIA_E_ELEMENTNOTAVAIL…

3688699

…ABLE

Retry pattern operations if they return UIA_E_ELEMENTNOTAVAILABLE

b13e974

sharwell changed the title ~~[WIP] Further work to stabilize the integration test interop layer~~ Further work to stabilize the integration test interop layer Mar 30, 2018

Use System.InvalidOperationException to avoid exception serialization…

1d5c75e

… failures

sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 30, 2018

sharwell changed the base branch from dev15.7.x to dev15.7.x-vs-deps March 30, 2018 22:18

sharwell changed the base branch from dev15.7.x-vs-deps to dev15.7.x March 30, 2018 22:18

333fred reviewed Mar 30, 2018

View reviewed changes

333fred requested changes Mar 30, 2018

View reviewed changes

Updates from code review feedback

aefa753

333fred approved these changes Mar 30, 2018

View reviewed changes

sharwell merged commit b45fa2d into dotnet:dev15.7.x Mar 31, 2018

sharwell deleted the com-message-filter branch March 31, 2018 14:27

sharwell added a commit to sharwell/MouseFastScroll that referenced this pull request May 1, 2018

Use a COM message filter for retry semantics

ffaf45e

See dotnet/roslyn#25455

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further work to stabilize the integration test interop layer#25455

Further work to stabilize the integration test interop layer#25455
sharwell merged 8 commits intodotnet:dev15.7.xfrom
sharwell:com-message-filter

sharwell commented Mar 13, 2018 •

edited

Loading

Uh oh!

sharwell commented Mar 30, 2018 •

edited

Loading

Uh oh!

333fred Mar 30, 2018

Uh oh!

sharwell Mar 30, 2018

Uh oh!

333fred Mar 30, 2018

Uh oh!

sharwell Mar 30, 2018

Uh oh!

333fred Mar 30, 2018

Uh oh!

sharwell Mar 30, 2018

Uh oh!

333fred Mar 30, 2018

Uh oh!

sharwell Mar 30, 2018

Uh oh!

333fred left a comment

Uh oh!

sharwell commented Mar 31, 2018 •

edited

Loading

Uh oh!

sharwell commented Mar 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sharwell commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Mar 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sharwell commented Mar 13, 2018 •

edited

Loading

sharwell commented Mar 30, 2018 •

edited

Loading

sharwell commented Mar 31, 2018 •

edited

Loading