Removes dependency on RemoteExecutor in tests#14538
Open
ricardobossan wants to merge 3 commits into
Open
Conversation
Supersedes dotnet#12379, which replaced `RemoteExecutor.Invoke(...)` with `Task.Run(...).Wait()`. That doesn't work for this set of tests: they mutate irreversible process-wide state (`Application.EnableVisualStyles`, `Application.VisualStyleState`, `Application.CurrentCulture`, `Control.CheckForIllegalCrossThreadCalls`), so any in-process replacement leaves the mutations behind for whatever runs next. The tests need a host that's isolated by construction. This PR follows RussKie's suggestion on dotnet#4500: move tests that legitimately need an isolated, visual-styles-enabled host into `UIIntegrationTests`, which already runs serially (`eng/xunit.runner.json`: `parallelizeTestCollections: false`, `maxParallelThreads: 1`) and pre-enables visual styles via `ControlTestBase`. The original skip reason on each test (`"Crash with AbandonedMutexException. See: https://github.com/dotnet/arcade/issues/5325"`) blames RemoteExecutor, but arcade#5325 is actually a Coverlet bug (`UnloadModule` mutex); the winforms repo still consumes the affected `coverlet.msbuild` variant. RemoteExecutor isn't the bug; it's the side that was easy to remove. These tests need *some* form of isolation though; `EnableVisualStyles` is a one-way switch, and `VisualStyleState` writes broadcast `WM_THEMECHANGED` to every HWND in the process. UIIntegrationTests already provides that isolation by construction. - Migrate the 17 previously-skipped tests below from `src/test/unit/System.Windows.Forms/...` to `src/test/integration/UIIntegrationTests/...`. Each migrated test no longer needs `RemoteExecutor` and is no longer skipped. - Remove the corresponding `using Microsoft.DotNet.RemoteExecutor;` and orphaned `MemberData` helpers from the unit-test files. `using` is preserved in `ApplicationTests.cs` because two non-skipped tests there still use `RemoteExecutor` (out of scope for this PR). - Net: −616 / +33 lines under `src/test/unit/`, +672 lines under `src/test/integration/UIIntegrationTests/` (six new files). Migrated tests: | New file (UIIntegrationTests) | Migrated tests | |---|---| | `Application.ParkingWindowTests.cs` | `ParkingWindow_DoesNotThrowOnGarbageCollecting` | | `Application.StaticStateTests.cs` | `Application_CurrentCulture_Set_GetReturnsExpected`, `Application_EnableVisualStyles_InvokeBeforeGettingRenderWithVisualStyles_Success`, `Application_VisualStyleState_Set_ReturnsExpected` | | `CommonDialogTests.cs` | 3× `CommonDialog_ShowDialog_*WithVisualStyles_*` | | `DataGridViewHeaderCellTests.cs` | `MouseLeaveUnsharesRow_InvokeWithDataGridViewMouseDown`, `OnMouseDown_InvalidRowIndexVisualStyles`, `OnMouseUp_InvalidRowIndexVisualStyles` | | `ListViewInsertionMarkTests.cs` | 5× `AppearsAfterItem_/Color_/Index_GetInsertMark[WithColor]_Success` (unsafe, via `LVM_GETINSERTMARK` / `LVM_GETINSERTMARKCOLOR`) | | `TabPageTests.cs` | `BackColor_GetVisualStyles_ReturnsExpected`, `BackColor_GetVisualStylesWithParent_ReturnsExpected` | Two intentional deltas from the original tests (both annotated inline): 1. `Application_EnableVisualStyles_InvokeAfterGettingRenderWithVisualStyles_Success` is **dropped**. Its precondition is `UseVisualStyles == false` followed by a call to `EnableVisualStyles()`. `EnableVisualStyles` is a one-way switch, and any test project that has previously enabled visual styles (which UIIntegrationTests does on startup) can never satisfy that precondition again. The behavior under test is documented as "not a recommended scenario"; the original only worked by spawning a fresh process. 2. `Application_CurrentCulture_Set_GetReturnsExpected` keeps every managed-side assertion but **drops the `Assert.Equal(expectedLcid, PInvokeCore.GetThreadLocale())` round-trip**. UIIntegrationTests is decorated with `[UseDefaultXunitCulture]` (via `ControlTestBase`), and xunit's culture fixture restores the thread locale around test execution. `SetThreadLocale` also silently no-ops for some LCIDs (notably invariant `0x7F`) on STA hosts. The Win32-level round-trip was an implementation detail that only held inside a freshly-spawned RemoteExecutor child; `Application.CurrentCulture`'s documented surface is the managed `Thread`/`CultureInfo` pair, which is still verified. Roughly 17 additional tests carry the same `"AbandonedMutexException ... arcade#5325"` skip reason but were not in dotnet#12379's scope: ~6 more in `DataGridViewHeaderCellTests`, 1 in `ListViewTests`, and ~10 in `ListViewGroupTests`. Per the staggered approach we discussed on the issue, leaving those for a follow-up PR keeps this one reviewable. - None. Test-only change. No product code is touched. - No. - Minimal. The migration restores test coverage that was previously skipped; if anything regresses it can only surface as test failures (caught by CI before merge), not as customer-facing behavior. UIIntegrationTests already covers the same project surface. - Unit tests / Integration tests. Built the solution locally; the migrated tests run green inside `UIIntegrationTests`. The unit-tests assembly still builds with the using/helper removals applied. No remaining references to the deleted `MemberData` helpers (`CustomLCIDCultureInfo`, `BackColor_GetVisualStylesWithParent_TestData`, `MouseLeaveUnsharesRow_WithDataGridViewMouseDown_TestData`); grep-verified before deletion. - 11.0.100-preview.4.26210.111
Member
|
@ricardobossan - can you check why the tests fail? |
Member
Author
|
Drafting until feedback is handled. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4500 (partial; see "Out of scope" below).
Supersedes #12379, which replaced
RemoteExecutor.Invoke(...)withTask.Run(...).Wait(). That doesn't work for this set of tests: they mutate irreversible process-wide state (Application.EnableVisualStyles,Application.VisualStyleState,Application.CurrentCulture,Control.CheckForIllegalCrossThreadCalls), so any in-process replacement leaves the mutations behind for whatever runs next. The tests need a host that's isolated by construction.This PR follows RussKie's suggestion on #4500: move tests that legitimately need an isolated, visual-styles-enabled host into
UIIntegrationTests, which already runs serially (eng/xunit.runner.json:parallelizeTestCollections: false,maxParallelThreads: 1) and pre-enables visual styles viaControlTestBase.Root Cause
The original skip reason on each test (
"Crash with AbandonedMutexException. See: https://github.com/dotnet/arcade/issues/5325") blames RemoteExecutor, but arcade#5325 is actually a Coverlet bug (UnloadModulemutex); the winforms repo still consumes the affectedcoverlet.msbuildvariant. RemoteExecutor isn't the bug; it's the side that was easy to remove. These tests need some form of isolation though;EnableVisualStylesis a one-way switch, andVisualStyleStatewrites broadcastWM_THEMECHANGEDto every HWND in the process. UIIntegrationTests already provides that isolation by construction.Proposed changes
src/test/unit/System.Windows.Forms/...tosrc/test/integration/UIIntegrationTests/.... Each migrated test no longer needsRemoteExecutorand is no longer skipped.using Microsoft.DotNet.RemoteExecutor;and orphanedMemberDatahelpers from the unit-test files.usingis preserved inApplicationTests.csbecause two non-skipped tests there still useRemoteExecutor(out of scope for this PR).src/test/unit/, +672 lines undersrc/test/integration/UIIntegrationTests/(six new files).Migrated tests:
Application.ParkingWindowTests.csParkingWindow_DoesNotThrowOnGarbageCollectingApplication.StaticStateTests.csApplication_CurrentCulture_Set_GetReturnsExpected,Application_EnableVisualStyles_InvokeBeforeGettingRenderWithVisualStyles_Success,Application_VisualStyleState_Set_ReturnsExpectedCommonDialogTests.csCommonDialog_ShowDialog_*WithVisualStyles_*DataGridViewHeaderCellTests.csMouseLeaveUnsharesRow_InvokeWithDataGridViewMouseDown,OnMouseDown_InvalidRowIndexVisualStyles,OnMouseUp_InvalidRowIndexVisualStylesListViewInsertionMarkTests.csAppearsAfterItem_/Color_/Index_GetInsertMark[WithColor]_Success(unsafe, viaLVM_GETINSERTMARK/LVM_GETINSERTMARKCOLOR)TabPageTests.csBackColor_GetVisualStyles_ReturnsExpected,BackColor_GetVisualStylesWithParent_ReturnsExpectedTwo intentional deltas from the original tests (both annotated inline):
Application_EnableVisualStyles_InvokeAfterGettingRenderWithVisualStyles_Successis dropped. Its precondition isUseVisualStyles == falsefollowed by a call toEnableVisualStyles().EnableVisualStylesis a one-way switch, and any test project that has previously enabled visual styles (which UIIntegrationTests does on startup) can never satisfy that precondition again. The behavior under test is documented as "not a recommended scenario"; the original only worked by spawning a fresh process.Application_CurrentCulture_Set_GetReturnsExpectedkeeps every managed-side assertion but drops theAssert.Equal(expectedLcid, PInvokeCore.GetThreadLocale())round-trip. UIIntegrationTests is decorated with[UseDefaultXunitCulture](viaControlTestBase), and xunit's culture fixture restores the thread locale around test execution.SetThreadLocalealso silently no-ops for some LCIDs (notably invariant0x7F) on STA hosts. The Win32-level round-trip was an implementation detail that only held inside a freshly-spawned RemoteExecutor child;Application.CurrentCulture's documented surface is the managedThread/CultureInfopair, which is still verified.Out of scope (follow-up)
Roughly 17 additional tests carry the same
"AbandonedMutexException ... arcade#5325"skip reason but were not in #12379's scope: ~6 more inDataGridViewHeaderCellTests, 1 inListViewTests, and ~10 inListViewGroupTests. Per the staggered approach we discussed on the issue, leaving those for a follow-up PR keeps this one reviewable.Customer Impact
Regression?
Risk
Test methodology
UIIntegrationTests. The unit-tests assembly still builds with the using/helper removals applied. No remaining references to the deletedMemberDatahelpers (CustomLCIDCultureInfo,BackColor_GetVisualStylesWithParent_TestData,MouseLeaveUnsharesRow_WithDataGridViewMouseDown_TestData); grep-verified before deletion.Test environment(s)
Microsoft Reviewers: Open in CodeFlow