Remove dependency on RemoteExecutor in tests

[willibrandon]

Fixes #4500

Proposed changes

• Remove dependency on RemoteExecutor in tests as it has been proven to be unstable.
• Migrate tests that change global state and that rely on RemoteExecutor to the UIIntegration test project.

Customer Impact

• Test methods should not be skipped.

Regression?

• No

Risk

• Minimal

Test methodology

• Ran unit and integration tests and observed for any signs of destabilisation.

Test environment(s)

• 7.0.100-preview.6.22352.1

Microsoft Reviewers: Open in CodeFlow

[RussKie]

Visual styles are enabled at the base level

winforms/src/System.Windows.Forms/tests/IntegrationTests/UIIntegrationTests/Infra/ControlTestBase.cs

Lines 23 to 27 in c290164

	protected ControlTestBase(ITestOutputHelper testOutputHelper)
	{
	TestOutputHelper = testOutputHelper;
	Application.EnableVisualStyles();

For intents and purposes of the tests we're porting in this PR, we'd likely want to be able to toggle visual styles. Perhaps you could do something like this:

    [UseDefaultXunitCulture]
    public abstract class ControlTestBase : IAsyncLifetime, IDisposable
    {
        protected ControlTestBase(ITestOutputHelper testOutputHelper, bool enableVisualStyles)
        {
            if (enableVisualStyles)
            {
                Application.EnableVisualStyles();
            }
        }
        protected ControlTestBase(ITestOutputHelper testOutputHelper)
            : this(testOutputHelper, enableVisualStyles: true)
        {
        }

    public class ListViewInsertionMarkVisualStylesOnTests : ControlTestBase
    {
        public ListViewInsertionMarkTests(ITestOutputHelper testOutputHelper)
            : base(testOutputHelper, enableVisualStyles: true)
        {
        }

    public class ListViewInsertionMarkVisualStylesOffTests : ControlTestBase
    {
        public ListViewInsertionMarkTests(ITestOutputHelper testOutputHelper)
            : base(testOutputHelper: enableVisualStyles: false)
        {
        }

[willibrandon]

I've provided a way for the tests to toggle visual styles like you suggested but for this particular test, does it make sense to test with visual styles off?

[willibrandon]

No. I've been adjusting to a new schedule, pardon me.

I'm close to having the changes ready and will make it a priority now.

[RussKie]

Progressing nicely

[RussKie]

💭 May interesting to see if this is still an issue with this move

[willibrandon]

If I un-skip the test and change [WinformsTheory] to [Theory], the test executes and passes successfully. If I leave it as [WinformsTheory], the test hangs on SystemEventsHelper.SendMessageOnUserPreferenceChanged(category).

[RussKie]

Thank you for testing. At a glance it doesn't look like there's anything that requires an affinitisation to the UI thread, so I think it'd be safe to go with [Theory].

[RussKie]

We need to run with visual styles on and off, e.g. similar to what you've defined in ListViewInsertionMarkVisualStylesOnTests and ListViewInsertionMarkVisualStylesOffTests.

[RussKie]

As noted above

[RussKie]

@willibrandon I got a notification of a comment but I can't seem to locate it, sorry.... It looks like we have a lot of ongoing discussions, and I find it hard to keep track of all of them. Do you think you could rebase and address any remaining pending comments?

[willibrandon]

@RussKie - Someone should take the delete button away from me. Sometimes I'm second guessing myself so much that it is too tempting not to click it. With that said, I have been made acutely aware of how confusing this can be and that I should treat this like email. I will do my best to never click the delete button ever again.

[RussKie]

Haha, all good. I do post and then delete sometimes, though generally I try to post another message instead.

[willibrandon]

I have some feedback left to address on this, and working towards that now.

[willibrandon]

I could use some help understanding why Application.UseVisualStyles is true here even though we pass a value of false for enableVisualStyles to the ControlTestBase base constructor. It seems the toggling of visual styles might be problematic since this test seems to be affected by other UITests enabling visual styles.

Did I miss something?

[RussKie]

Gah... In short, it looks like I underappreciated how the tests from this project are executed. All tests from this project are run in a single process, and, because some tests do enable the visual styles, depending on the order of execution tests (that expect the state of visual styles on or off) will fail. If you run this test in isolation it will succeed.

There are multiple ways to resolve this, and after spending several hours exploring some of them, none appears particlualry painless.
E.g.,

1. we can condition the tests using ConditionalFact, but it won't work for STA-bound tests, and we'll have to exend WinFormsFact (and WinFormsTheory) to be conditional. This likely require an update the Xunit.StaFact library, as some infra is private and doesn't allow an easy overriding.
2. We can pepper the code with something like the following:

     <PropertyGroup Condition="'$(TestVisualStylesOn)' == ''">
       <TestVisualStylesOn>true</TestVisualStylesOn>
     </PropertyGroup>
   
     <PropertyGroup Condition="'$(TestVisualStylesOn)' == 'true'">
       <DefineConstants>$(DefineConstants);VISUAL_STYLES_ON</DefineConstants>
     </PropertyGroup>

   #if VISUAL_STYLES_ON
      [WinFormsFact(Skip = "Not supported with Visual Styles enabled")]
   #else
      [WinFormsFact]
   #endif
      [WinFormsFact]
      [VisualStylesEnabled(false)]
      public void Application_VisualStylesOn_must_not_be_enabled()
      {
          Assert.False(Application.UseVisualStyles, "Tests in this configuration must not have VisualStyles enabled.");
          Assert.False(Application.RenderWithVisualStyles);
      }

   We'd also need to wrap all callsites with:

   #if VISUAL_STYLES_ON
           Application.EnableVisualStyles();
   #endif

   Running from a command line we'd need to add /p:TestVisualStylesOn=false to run without the visual styles.
3. We can add traits, and decorate test classes and/or specific tests with traits. This would require finding a way how to pass -trait VisualStylesEnabled=false (example) to the test runner (which is part of microsoft.dotnet.arcade.sdk).
4. We can try to update to Xuni v3, which has Skip functionality - Need ability to skip test programmatically xunit/xunit#2073 (comment)

I also tried this, but it didn't appear to work - instead of skipping the test it would still fail it:

    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public class SkipIfVisualStylesAttribute : BeforeAfterTestAttribute
    {
        public bool Enable { get; set; }

        public override unsafe void Before(MethodInfo methodUnderTest)
        {
#if VISUAL_STYLES_ON
            if (Enable)
                throw new SkipException("Not supported with Visual Styles disabled");
#else
            if (!Enable)
                throw new SkipException("Not supported with Visual Styles enabled");
#endif
        }
    }

[RussKie]

So as a quick get out of jail card - we can wrap all tests that expect visual styles to be off with

#if VISUAL_STYLES_OFF
           // tests
#endif

...and then we can finish this work at a later date.

[merriemcgaw]

@willibrandon sorry this fell through the cracks! I'm closing this now but if you're willing to pick it back up just holler and we can re-open it for you.

[willibrandon]

My apologies. Sometimes I bite off more than I can chew 😩. These tests can be very finicky, and time-consuming.

With that said, I don't want to give up on this, especially if #4500 is still an issue. I just might not be the best person to tackle this one yet. I'll see if I can get back to it, and if so, I will holler.