Isolate tests by sharwell · Pull Request #25681 · dotnet/roslyn

sharwell · 2018-03-23T17:30:26Z

This change implements MEF container isolation during tests.

It is further expected to close a number of open flaky test issues where xunit was crashing in a FailFast.

This is almost strictly a test change. The only change that could impact a user is if code with IVTs to Roslyn uses new AsynchronousOperationListenerProvider(), as opposed to going to the MEF container.

…roviderCache

…tProvider

…vider in a test constructor

…ch test

… teardown

Closes dotnet#25560

This addresses a code review feedback point from dotnet#25558.

CyrusNajmabadi · 2018-03-26T07:27:16Z

is this ready for review?
is there any writup that explains the new system and how new test fixtures should be created?
Does this affect how people write features?

sharwell · 2018-03-26T14:55:05Z

is this ready for review?

Yes, ready and working aside from #25691

is there any writup that explains the new system and how new test fixtures should be created?

It's not much different from the last one, and if you break one of the new rules it will typically tell you what the specific problem was. Most of the changes to how tests are created are listed in #25560.

Does this affect how people write features?

It shouldn't.

heejaechang · 2018-03-26T22:52:36Z

src/EditorFeatures/CSharpTest/Classification/SemanticClassifierTests.cs

                using (var disposableView = workspace.ExportProvider.GetExportedValue<ITextEditorFactoryService>().CreateDisposableTextView(extraBuffer))
                {
-                    var listenerProvider = new AsynchronousOperationListenerProvider();
+                    var listenerProvider = workspace.ExportProvider.GetExportedValue<IAsynchronousOperationListenerProvider>();


tagging @dotnet/roslyn-ide @jinujoseph @DustinCampbell @Pilchie @jasonmalinowski

I think if we go this route, we should change our code to use [Import] property/fields rather than doing [ImportingConstructor]

we choose ImportingConstructor at the time with completely opposite mindset. we wanted to give in Mock to dependencies. and make test more API level unit tests.

we start to move to more behavior test kind of unit tests. start to use MEF to provide dependencies (since our dependency was too complex, almost impossible to provide all those mocks at right layer).

now with this change, where we use MEF provider itself as isolation point, I think ImportingConstructor actually work against what we want. since user can provide one at the constructor, and deep down other service getting same dependency from MEF export and messing up everything.

we should ban using ImportingConstructor from our code, and use [Import] instead. to force, all dependencies are coming from MEF export.

rather than doing [ImportingConstructor]

We need to keep [ImportingConstructor] so we can block direct construction of the object (otherwise the default constructor would be public and accessible). The placement of dependencies as parameters vs. properties at that point makes little difference.

Also note that this strategy does not prevent the use of mocks. To the contrary, it provides a way for a mock to be uniformly introduced for the duration of a single test, and have it disposed at the end of the test.

hmm.. I dont understand what you mean by blocking direct construction? you can give in null to all dependencies. or mock to it. isn't one of point of what you are doing here is to make sure all dependencies get same services rather than different one?

why use code review to make sure it gets same dependency when you can make sure in does by the system?

having [Import] is same as I create object giving null to all dependencies?

The goal of the change is for the following to be a compilation error:

new AsynchronousOperationListenerProvider(...)

Alternatives to what I have implemented here are fine as long as that characteristic is preserved.

that's interesting. so planning to mark all importing constructor as obsolete? that should work as well.

so, when someone wants to test service itself like
http://source.roslyn.io/#Roslyn.Services.Editor2.UnitTests/GoToDefinition/GoToDefinitionCommandHandlerTests.vb,103

they should get CSharpGotoDefinitionService from MEF from the beginning? ya. that should work as well rather than changing all code to [Import] from [ImportingConstructor]

so I guess that work is not done yet? I see only Async Listener Provider is marked obsolete. that can fix the problem where 2 different instance of IAsynchornounsListenerProvider is used. one directly given and one from MEF. but what about all other services?

Correct, I have only made the change for two services to date (one in #25558, and one here).

heejaechang · 2018-03-27T18:24:22Z

src/EditorFeatures/Test2/GoToDefinition/GoToDefinitionCommandHandlerTests.vb

+            Using workspace = TestWorkspace.Create(definition, exportProvider:=GoToTestHelpers.ExportProviderFactory.CreateExportProvider())
                Dim cursorDocument = workspace.Documents.First(Function(d) d.CursorPosition.HasValue)
                Dim cursorPosition = cursorDocument.CursorPosition.Value



this will be the example. here it uses mock IDocumentNavigationService and IStreamFindUsagespresenter mock.

but one of them is given as dependency and the other uses MEF. one is probably given as MEF since it is not exposed through constructor of CSharpGotoDefinitionService but want to easily inject that dependency under several layers.

and the other is happen to be exposed through CSharpGoToDefinitionService. unless I know exactly how these dependencies are used, I can't be sure whether IStreamingFindUsagesPresernter I gave in here is the one used internally.

removing all possibility to provide dependency except through MEF removes your concern that some tests uses 2 different instances of same service (one from constructor and one from MEF) and making test flaky.

especially, when one can add dependency through MEF to new service without realizing one had to change test like this as well since one just introduced possiblity of tests having 2 different instance of same service.

by the way, I just searched Mock in roslyn solution. there are quite more mocks we used in our tests. I didn't even search for Testxxx. (I believe we used Mockxxx or Testxxx naming for our mock dependencies for unit tests)

Correct, fixing the mocks is left as a follow-up item. 😄

jasonmalinowski

Round one of reviewing complete. To give context, I was reviewing commit-by-commit, and tried my best to revise previous comments if a later commit cleared up an earlier question. My apologies if I missed something.

I would like more comments in ExportProviderCache and UseExportProviderAttribute. Perhaps more comments than code. Half way through I wasn't able to figure out what was going on there anymore (perhaps a side effect of commit-by-commit, I admit) and so further commits only got me more confused. There's some complicated threading bits where both the "what" and the "why" aren't clear from the code, which I suspect can be simplified or at least explained well. If we can at least get comments there, I'm happy to re-review and (presuming I actually understand it and think it's right 😄) can sign off

I did very much appreciate the change to using IExportProviderFactory more. Wish we had done that sooner.

Many ProjectReferences to EditorFeatures* stuff got added, and I believe those are layering violations we'd like to avoid. I recognize that if we're already consuming those via MEF the violation is there, but let's not perpetuate it. 😄 By the end of this UseExportProviderAttribute seems to have become MakeSureThingsGetCleanedUpAttribute, and I wonder if that hints to a bit better abstraction. Spitballing here, if we just had a UseDeferredTestCleanupAttribute that just held a List that various pieces of code could register into (which would of course fail if somebody didn't have the attribute, preserving your guard), it means the ExportProviderCache could just create an ExportProvider and then register a cleanup action. Ditto for all the other things happening in the finally {} block there, complete with all the various waiting logic. I could imagine the ExportProvider cleanup also just doing a ExportProvider.GetExportValues() and where that's just an interface we define if you want other ways to inject. It's adding a layer of indirection, sure, but I think this could become a very useful tool long term. We have a lot of test stuff where we often stick logic in finalizers to ensure that test stuff was Disposed -- this would mean we could easily add "assert this was done with this helper by the time we're done" which would be great.

jasonmalinowski · 2018-03-28T23:04:48Z

src/EditorFeatures/TestUtilities/Workspaces/TestWorkspaceFixture.cs

        }

-        public void CloseTextView()
+        public void DisposeAfterTest()


What's the point of CSharpTestWorkspaceFixture at all then? And isn't this still going to fail to flag any of the tests that are sharing it for the xunit-constructed fixture?

➡️ No purpose. Filed #25804 to address this in a follow-up PR.

jasonmalinowski · 2018-03-28T23:20:09Z

src/VisualStudio/CSharp/Test/CodeModel/AbstractFileCodeElementTests.cs

+        {
+            get
+            {
+                return _workspaceAndCodeModel ?? (_workspaceAndCodeModel = CreateWorkspaceAndFileCodeModelAsync(_contents));


Just to confirm understanding, is this an isolation fix, or just avoiding eagerly creating things when we're not using them?

➡️ Constructors are called before UseExportProviderAttribute, so eager initialization of these properties will throw an exception on the following line (the entire gap from the end of a test to the initialization of the export provider for the next test is attributed to the former test in the exception message):

roslyn/src/Workspaces/CoreTestUtilities/UseExportProviderAttribute.cs

Line 74 in 7eb0b2d

MefHostServices.HookServiceCreation((_, __) => throw new InvalidOperationException("Cannot create host services after test tear down."));

Constructors are called before UseExportProviderAttribute

Interesting. That feels like an xunit bug/gotcha -- is that something we can fix or at least make more diagnosable? Otherwise the obvious sequence of events of:

Write a new test from scratch, get error that you don't have attribute.

Apply attribute.

Run test.

Still broken.

seems quite undebuggable.

➡️ Extracted a method which throws the InvalidOperationException, and added a comment to help users who hit it.

jasonmalinowski · 2018-03-28T23:21:14Z

src/Workspaces/CSharpTest/CodeGeneration/AddImportsTests.cs

+                    "test.dll",
+                    LanguageNames.CSharp,
+                    metadataReferences: new[] { TestReferences.NetFx.v4_0_30319.mscorlib }));
+


Extra blank line.

jasonmalinowski · 2018-03-28T23:27:20Z

src/Workspaces/CoreTestUtilities/TestWorkspace.cs

-        private readonly NonReentrantLock _serializationLock = new NonReentrantLock();
-
-        public TestWorkspace(HostServices hostServices = null)
-            : base(hostServices ?? new AdhocWorkspace().Services.HostServices, "Test")


By switching the WorkspaceKind we're potentially changing the services under test here. We validated this is maintaining the existing test intentions?

➡️ It was only used in three places, all in FormattingTests.cs. These tests do not appear to be impacted by the change.

jasonmalinowski · 2018-03-28T23:27:32Z

src/Workspaces/CoreTestUtilities/TestWorkspace.cs

-
-namespace Microsoft.CodeAnalysis.UnitTests
-{
-    internal class TestWorkspace : Workspace


I always hated having two TestWorkspaces, thank you.

jasonmalinowski · 2018-03-29T00:17:22Z

src/EditorFeatures/TestUtilities/TestExportProvider.cs

@@ -111,9 +118,9 @@ private static Type[] GetNeutralAndCSharpAndVisualBasicTypes()
        /// Create fresh ExportProvider that doesn't share anything with others. Tests can use this


Comment now stale.

jasonmalinowski · 2018-03-29T00:22:29Z

src/Workspaces/CoreTestUtilities/ExportProviderCache.cs

+                {
+                    return s_defaultHostCatalog;
+                }
+                else if (s_desktopHostCatalog != null && assembliesArray == DesktopMefHostServices.DefaultAssemblies)


Oof this is magic: so the static initializer calls the method that then uses the field inside of it? That seems like it's ripe for breaking.

➡️ Refactoring to avoid magic

jasonmalinowski · 2018-03-29T00:26:26Z

src/Workspaces/CoreTestUtilities/ExportProviderCache.cs

+                var expectedCatalog = Interlocked.CompareExchange(ref _expectedCatalog, _catalog, null) ?? _catalog;
+                if (expectedCatalog != _catalog)
+                {
+                    throw new InvalidOperationException($"Only one {nameof(ExportProvider)} can be created for a single test.");


I admit I'm still not getting the one-export-provider restriction. If it's for cleanup, why can't we just clean all of the ones that were made up? It seems like it'd simplify the code.

I can imagine you had cases through this work where we were creating one that was previously a static and by re-creating multiple instances you caused a bug because state that was a singleton in a test now isn't. But it seems that was a point-in-time offense, where now just allowing multiple to be created and cleaning up all of them would be easier code to maintain going forward.

Put another way: this rule might have been great for your work to do this, but seems like it needs justification (and commenting) if it's going to stay.

➡️ Documenting condition code at the site where the exception is thrown

jasonmalinowski · 2018-03-29T00:29:38Z

src/Workspaces/CoreTestUtilities/ExportProviderCache.cs

+                if (expected == null)
+                {
+                    expected = _exportProviderFactory.CreateExportProvider();
+                    expected = Interlocked.CompareExchange(ref _expectedProviderForCatalog, expected, null) ?? expected;


Why the Interlocked* here? Is this trying to support a single test consuming multiple threads? It's clearly not multiple tests running in parallel (since this mechanism of using statics wouldn't work)...

jasonmalinowski · 2018-03-29T00:30:38Z

src/Workspaces/CSharpTest/CSharpServicesTest.csproj

    <ProjectReference Include="..\..\Compilers\Core\Portable\CodeAnalysis.csproj" />
    <ProjectReference Include="..\..\Compilers\CSharp\Portable\CSharpCodeAnalysis.csproj" />
    <ProjectReference Include="..\..\Compilers\Test\Resources\Core\CompilerTestResources.csproj" />
+    <ProjectReference Include="..\..\EditorFeatures\Core\EditorFeatures.csproj" />


This is a layering violation I believe.

➡️ This should be covered by #25805

jasonmalinowski · 2018-03-29T01:08:46Z

And I should clarify: the ProjectReference concerns I wouldn't consider blocking for this PR, but if they get cleaned up while trying to simplify the attribute and ExportProviderCache, I won't complain...

sharwell · 2018-03-29T04:24:43Z

@jasonmalinowski I filed follow-up issues (with links to/from comments) and pushed a new commit with documentation and minor refactoring.

jasonmalinowski

With #25806 I'm at least convinced we kinda know what this code is doing and how to make it better, which is better than prior to it being filed.

jasonmalinowski · 2018-03-30T20:16:28Z

src/Workspaces/CoreTestUtilities/ExportProviderCache.cs

-            CreateExportProviderFactory(s_remoteHostCatalog);
+            CreateExportProviderFactoryNoCache(s_remoteHostCatalog);

        private static bool _enabled;


Can't stick a comment on line 47 so doing it here: can we rename the setting property to something more like "Enabled_OnlyCallFromUseExportProvider"? I read the property as "enabled via the attribute" which implies there are other ways you can enable that are also valid. I didn't read it as "only use from there".

➡️ Name is improved now

jasonmalinowski · 2018-03-30T20:18:12Z

src/Workspaces/CoreTestUtilities/ExportProviderCache.cs

                }
            }

+            return CreateAssemblyCatalogNoCache(assemblies, resolver);


You can remove the null checks now in CreateAssemblyCatalog, since they're always non-null since this method is always running after the static field initialization ran.

(I do like the cleanup though, thanks.)

Also random thought/naming nit: should CreateAssemblyCatalogNoCache just be called "CreateAssemblyCatalog" and the caching one called GetOrCreate? I think that pattern is seen elsewhere and clarifies that "Create" doesn't always create something new.

➡️ Modified to use Create and GetOrCreate consistently aside from the remaining issue in #25863

jasonmalinowski · 2018-03-30T20:20:39Z

src/Workspaces/CoreTestUtilities/ExportProviderCache.cs

+                    // * A test attempted to perform multiple test sequences in the context of a single test method,
+                    //   rather than break up the test into distict tests for each case.
+                    // * A test referenced different predefined ExportProvider instances within the context of a test.
+                    //   Each test is expected to use the same ExportProvider throughout the test.


This comment is great, and motivates the reasoning well. Thanks.

sharwell · 2018-03-30T22:30:56Z

@Pilchie Can I get ask mode approval for the production code changes in this PR? They are:

A few IVTs from production to local test assemblies (should not need special approval?)
A change to IAsynchronousOperationListenerProvider.cs here, which prevents outside code from using the constructor of AsynchronousOperationListenerProvider. We already verified it is not used in this manner downstream.

sharwell@33a1a3b#diff-ce3a65ac7e5f22f0b2333d00b6c89556

The addition of one test hook method:

roslyn/src/Workspaces/Core/Desktop/Workspace/Host/Mef/DesktopMefHostServices.cs

Lines 25 to 28 in 7e8391f

    
           internal static void ResetHostServicesTestOnly() 
        
           { 
        
               s_defaultServices = null; 
        
           }

Pilchie · 2018-03-30T23:38:37Z

Yes, I approve those changes.

The getter is 'Enabled' and the setter is 'SetEnabled_OnlyUseExportProviderAttributeCanCall'.

…een tests

sharwell · 2018-03-31T18:14:06Z

Fun, so error code COR_E_EXECUTIONENGINE is a new one for me. 😆

Closes dotnet#25805

This change ensures that all work added to the work queue after disconnection is immediately cancelled.

sharwell · 2018-03-31T20:32:48Z

@Pilchie There is one more change to approve in commit a4eb409. I kept hitting cases where a test didn't cancel it's work, resulting in a failed test. After quite a bit of debugging locally, I found that in some cases (average once per execution of the full test suite), the CancellationTokenSource was getting reset to non-cancelled after TagSource.Dispose was called, and then a work item queued with that token.

Edit: 8ede7e4 further improves test cleanup reliability by avoiding scheduling new work items for which cancellation is requested in advance.

While it doesn't make sense to still be requesting foreground actions after cancellation is requested, tracking down all callers of IForegroundNotificationService to ensure the proper checks are in place can be tedious. This change treats IForegroundNotificationService as the gatekeeper for scheduling asynchronous operations, and proactively cancels operations if requested before adding them to the work queue.

sharwell added 18 commits March 23, 2018 12:23

Remove TestWorkspace from WorkspacesTestUtilities

ac8ec16

Move several helper methods from MinimalTestExportProvider to ExportP…

c2d44d0

…roviderCache

Implement UseExportProviderAttribute for single-test scoping of Expor…

2409375

…tProvider

Move UseExportProviderAttribute to the workspaces layer

aad82d2

Implement RoslynServices hook

d38b799

Use correct ExportProvider in TestWorkspace

19aef6d

Split test to avoid creating multiple export providers in a single test

58c1a55

Encapsulate field SyntaxGeneratorTests._g to avoid creating ExportPro…

3a64f13

…vider in a test constructor

Update WorkspaceFixture to support disposing the workspace between ea…

12a7896

…ch test

Remove shared ExportProvider from TestHostServices

6894e9e

Avoid attempts to create ExportProvider in test constructors

7eb0b2d

Apply UseExportProviderAttribute to tests where required

c5465fc

Always obtain IAsynchronousOperationListenerProvider from MEF

33a1a3b

Clear cancelled items from IForegroundNotificationService during test…

860428e

… teardown

Remove sharing of ExportProvider instances between tests

7e8391f

Closes dotnet#25560

Improve the caching mechanism used for export providers during testing

0f7eeec

Use ApplicationIdle instead of SystemIdle in DispatcherExtensions

8401a66

Update code comment to reflect actual behavior

c53626a

This addresses a code review feedback point from dotnet#25558.

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

sharwell added 2 commits March 23, 2018 12:50

Add missing project references

b0b89a2

Fix failure to dispose of workspaces during tests

426c63f

heejaechang reviewed Mar 26, 2018

View reviewed changes

heejaechang reviewed Mar 27, 2018

View reviewed changes

sharwell mentioned this pull request Mar 27, 2018

InteractivePasteCommandhandlerTests.PasteCommandWithInteractiveFormat failure #25315

Closed

jasonmalinowski reviewed Mar 29, 2018

View reviewed changes

This was referenced Mar 29, 2018

Remove TestWorkspaceFixture and derived types #25804

Closed

Implement extensible state cleanup in UseExportProviderAttribute #25805

Closed

sharwell mentioned this pull request Mar 29, 2018

Simplify DoEvents implementation and use for test state cleanup #25806

Closed

Document code and slightly refactor to improve clarity/maintainability

a7dba96

sharwell mentioned this pull request Mar 30, 2018

Pass SemanticModel as an argument where it is required #25839

Merged

jasonmalinowski approved these changes Mar 30, 2018

View reviewed changes

Merge remote-tracking branch 'dotnet/dev15.7.x' into isolate-tests

e7427b5

Pilchie added the Approved to merge label Mar 30, 2018

sharwell added 5 commits March 31, 2018 09:41

Rename EnabledViaUseExportProviderAttributeOnly for clarity

3c17eae

The getter is 'Enabled' and the setter is 'SetEnabled_OnlyUseExportProviderAttributeCanCall'.

Use Create and GetOrCreate method prefixes as appropriate for clarity

0666f37

Remove unnecessary null checks

e5ff94f

Add guidance for users who hit ExportProvider-related exceptions betw…

9c1df9f

…een tests

Use Lazy<T> to simplify value initialization

2e7f840

sharwell added 2 commits March 31, 2018 13:18

Simplify the Join operation used for waiters during test cleanup

10548cd

Closes dotnet#25805

Fix race condition in AsynchronousSerialWorkQueue work cancellation

a4eb409

This change ensures that all work added to the work queue after disconnection is immediately cancelled.

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

sharwell deleted the isolate-tests branch March 31, 2018 23:33

sharwell mentioned this pull request Sep 22, 2020

Track completion of ProjectCacheService operations #47902

Draft

BillWagner mentioned this pull request Feb 2, 2022

Add generic attributes dotnet/docs#26895

Merged

		@@ -111,9 +118,9 @@ private static Type[] GetNeutralAndCSharpAndVisualBasicTypes()
		/// Create fresh ExportProvider that doesn't share anything with others. Tests can use this

Conversation

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

Uh oh!

Uh oh!

CyrusNajmabadi commented Mar 26, 2018

Uh oh!

sharwell commented Mar 26, 2018

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

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!

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

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!

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!

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

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!

jasonmalinowski commented Mar 29, 2018

Uh oh!

sharwell commented Mar 29, 2018

sharwell commented Mar 23, 2018 •

edited

Loading

heejaechang Mar 26, 2018 •

edited

Loading

sharwell Mar 26, 2018 •

edited

Loading

heejaechang Mar 27, 2018 •

edited

Loading

heejaechang Mar 27, 2018 •

edited

Loading

sharwell Mar 29, 2018 •

edited

Loading

sharwell Mar 29, 2018 •

edited

Loading

sharwell commented Mar 31, 2018 •

edited

Loading

sharwell commented Mar 31, 2018 •

edited

Loading