Untangle test utils refs

[RussKie]

Pre-req or addendum to #3010

Proposed changes

• Break up and remove InternalUtilitiesForTests project
• Move all low-level general test functionality into System.Windows.Forms.Primitives.TestUtilities project. System.Windows.Forms.Primitives.Tests is made dependent on this project.
• Move all other test-related functionality into System.Windows.Forms.TestUtilities project, dependent on System.Windows.Forms.Primitives.TestUtilities project. Update all main test projects to depend on this project.

NB: some tests in System.Windows.Forms.Primitives.Tests have been commented out until necessary low-level abstractions (such as introduced in #3010) are added.

Microsoft Reviewers: Open in CodeFlow

[codecov]

Codecov Report

Merging #3013 into master will decrease coverage by 30.95412%.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##              master       #3013          +/-   ##
====================================================
- Coverage   65.14233%   34.18821%   -30.95413%     
====================================================
  Files           1324         890         -434     
  Lines         489393      253798      -235595     
  Branches       40028       36786        -3242     
====================================================
- Hits          318802       86769      -232033     
+ Misses        165176      162265        -2911     
+ Partials        5415        4764         -651     

Flag	Coverage Δ
#Debug	34.18821% <ø> (-30.95413%)	⬇️
#production	34.18821% <ø> (+0.00038%)	⬆️
#test	?

[RussKie]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[RussKie]

@JeremyKuhne what do you think of the structure?

[JeremyKuhne]

In general this looks good to me. I'm sure we'll iterate over time, but this is a good step in the right direction.

[RussKie]

@hughbe @weltkante @vladimir-krestov FYI - I'm planning to merge this in the next 24hrs, unless there are objections.

[weltkante]

I don't like that we are turning WinFormsFact back to StaFact, I don't have definitive proof it causes issues but during the xunit.stafact upgrade it felt like I had to turn many StaFact to WinFormsFact to get tests succeed reliably. I couldn't exactly pin down all the root causes so it may have been unrelated, but if not this change may introduce flakyness (also see #3271 where my current theory is that OLE initialization may have been related to the previously observed CI flakyness)

If you want to merge as-is to get the restructuring going we'll want to track if it causes issues, or perhaps create a follow-up issue to clean up annotations. Personally I'd prefer as little usage of StaFact as possible because it requires a lot of thought to reason about when you can use them safely. Using WinFormsFact is always the safer way because as soon as you are using WinForms API you want a test infrastructure compatible with it. Directly testing pure STA based COM interop (without WinForms or OLE) is kinda rare in this repo.

[weltkante]

I don't think thats good, I still suspect STA is not enough for OLE, but don't have definitive proof yet. WinFormsFact will call Application.OleRequired but StaFact will just set the thread to STA.

StaFact often may work because WinForms implicitly initializes OLE in a lot of call chains, but if you directly test OLE interop like here that may not happen. Also the StaFact thread becomes "poisoned" by having OLE initialized through tests, depending on order tests may run with or without OLE initialized and become flaky if they actually require OLE

IMHO WinForms almost never should want StaFact unless you are testing some STA-but-not-OLE COM interop, which is rare in the scope of WinForms, most things are OLE.

[RussKie]

We don't have access to System.Windows.Forms so we can't use WinFormsFact or any of SWF types

[weltkante]

this is using OLE APIs due to the usage of IPicture for testing IDispatch, I would keep them on WinFormsFact

[weltkante]

also see #3271 where we wanted to track if this being WinFormsFact fixes flaky tests after the xunit.stafact update, so turning it back to stafact may interfere

[RussKie]

Speculating here, but flakiness might come from a fact that we're creating multiple of AxHost controls concurrently, that spin up the whole SWF infra, whereas for these tests we want to ensure our COM interop definition is correct.

[RussKie]

This is one of the issues this restructure is aiming to address - to provide clarity on what we're testing - COM/pinvoke definitions or actual control implementations.

[weltkante]

Its not introduced by the PR, but this seems wrong, GetIPictureFromCursor calls OleCreatePictureIndirect so the test should be WinFormsFact (or at the very least StaFact). Even if this API is robust enough to be called from MTA it doesn't feel right to rely on it, normally all OLE API is required to have OLE initialized.

Also adding [Collection("Sequential")] is probably not required here, its not testing some globally shared resource like clipboard or drag'n'drop, so I don't see why it should coordinate with other tests.

[weltkante]

Note that there were a few tests which were intentionally testing for failure when called on the wrong COM apartment, these may need to remain Fact and not turned to WinFormsFact. I don't see any of these tests in this file though.

[weltkante]

This being Fact is also tracked in #3271 so if you want you can defer it and not update it in this PR

[RussKie]

I don't believe any of IPictureTests, IDispatchTests or ITypeInfoTests test x-thread access, or failures for that matter.

I copied [Collection("Sequential")] from the other two types, I think it ensures that these tests aren't run in parallel.

[weltkante]

I don't believe any of IPictureTests, IDispatchTests or ITypeInfoTests test x-thread access, or failures for that matter.

There are unexplained E_FAIL returns from OleCreatePictureIndirect on the CI machine (#3271). The only theory I currently have is that OLE isn't initialized and for some reason the CI machine is less robust than running locally, otherwise I have no idea why the initial call already fails with E_FAIL.

I copied [Collection("Sequential")] from the other two types, I think it ensures that these tests aren't run in parallel.

It ensures that the tests are not run in parallel with other tests having that attribute, so it can protect process wide shared resources. xunit docs say that tests on the same class are never run in parallel, only multiple test classes are run in parallel (can look up the doc link if you need it)

[RussKie]

Correct, this is what I meant (sorry for not being clearer).
Given that all of the tests from these test classes test more of less the same area we probably don't want them to run in parallel.

[RussKie]

@weltkante the idea here a clear separation of layers. System.Windows.Forms.Primitives project contains "bare bone" implementations and imports. This project underpins System.Windows.Forms project which defines and implements higher level abstractions such as Application, Control, Form, etc.
Previously we had a shared test infra, that led to cross contamination of tests, e.g. passing higher level abstractions into tests for lower level abstractions.

WinFormsFact is dependent on System.Windows.Forms, so we can't use it at System.Windows.Forms.Primitives.Tests level, but we can use StaFact that provides STA thread affinity.

[weltkante]

We need to watch if these tests are still flaky then (#3271) and if they are we need explicit OleInitialize calls to test if unexpected E_FAIL return values go away when OleInitialize was called. Automatically initializing OLE is something that was only added to WinFormsFact because its not something STA threads usually need, unless they are also running WinForms or WPF.

[RussKie]

The tests seem to hold after two runs.
I'm happy to re-run them few more times, if that'll give you a higher degree of confidence.

[weltkante]

Ok so some concerns are adressed by the fact that you don't have a reference to WinForms, so you can't accidently call its API and "poison" the STA thread with WinForms infrastructure that doesn't get cleaned up.

That leaves the OLE initialization which we don't know if its an issue or not. Never was an issue locally for me and I don't know how rare the failures in #3271 were, but feel free to merge, it probably will be noticable over time if OleCreatePictureIndirect still returns E_FAIL occasionaly on the CI machines.