Unstable unit tests that need to be disabled and fixed

[LeafShi1]

Disable failing tests and open tracking bugs

1. When test fails in the CI or PR build, open a bug for each test fail to investigate the test further and add a disabled-test label to the bug.
2. Disable test by adding a [skip] attribute, If only a specific theory data causes the failure, comment that data out. (In order to prevent some problems from being caused by changes in the PR itself or machine problems, if it fails only once in a month, we will not disable it. If it fails a second time, we will disable it.)
3. Add a link to the tracking bug to the SkipAttribute of the comment line.
4. Collect all data relevant to the failure into the bug.
5. Try to root-cause the flakiness, and fix these tests:
   • Replacing SendInput or cursor-positioning steps with direct method invocations, or by sending WM_COMMAND
   • If UI element is easily identifiable, for example by and accessible name, or we have access to the AccessibleObject, we can use UIA
     APIs to invoke methods
   • Add logging if possible.
   • If the test must use SendInput, then make sure it is moved to the UI Integration tests assembly

The following unit tests exist flaky issues:

• System.Windows.Forms.Tests.ImageCollectionTests.ImageCollection_Item_Get32bppColorDepth_Success(pixelFormat: Format1bppIndexed, pixel00Color: Color [Empty], givenPixel10Color: Color [Empty], expectedPixel10Color: Color [A=255, R=0, G=0, B=0])
• System.Windows.Forms.Tests.ToolTipTests.ToolTip_SetToolTip_TabControl_DoesNotAddToolForTabControlItself
• System.Windows.Forms.Tests.ToolTipTests.ToolTip_WmShow_Invokes_AnnounceText_WithExpectedText_ForTabControlTabs
• System.Windows.Forms.Tests.DataGridViewTests.DataGridView_OnColumnHeadersHeightChanged_InvokeWithHandle_CallsColumnHeadersHeightChanged(columnHeadersWidthSizeMode: EnableResizing, columnHeadersVisible: True, eventArgs: null)
• System.Windows.Forms.UITests.ButtonTests.Button_CancelButton_EscapeClicksCancelButtonAsync
• System.Windows.Forms.UITests.ButtonTests.Button_DialogResult_SpaceToClickFocusedButtonAsync
• System.Windows.Forms.UITests.ButtonTests.Button_Hotkey_Fires_OnClickAsync
• System.Windows.Forms.UITests.ButtonTests.Button_Press_Enter_Fires_OnClickAsync
• System.Windows.Forms.UITests.ListViewTests.ListView_Group_NavigateKeyboard_SucceedsAsync
• System.Windows.Forms.UITests.NumericUpDownTests.NumericUpDownAccessibleObject_Focused_ReturnsCorrectValueAsync
• System.Windows.Forms.Tests.CheckBoxRendererTests.CheckBoxRenderer_DrawCheckBox_OverloadWithSizeAndText
• System.Windows.Forms.Tests.TextBoxBaseTests.TextBoxBase_ClearUndo_CanUndo_Success
• System.Windows.Forms.Tests.ToolStripTests.ToolStrip_WndProc_InvokeMouseActivate_Success
• System.Windows.Forms.Tests.ToolStripTests.ToolStrip_WndProc_InvokeMouseActivateWithHandle_Success
• System.Windows.Forms.Tests.DataGridViewTests.DataGridView_ColumnHeadersHeight_SetWithHandle_GetReturnsExpected(columnHeadersWidthSizeMode: AutoSize, columnHeadersVisible: True, autoSize: True, value: 4, expectedValue: 18, expectedInvalidatedCallCount: 0)
• System.Windows.Forms.Tests.DataGridViewTests.DataGridView_ColumnHeadersHeight_SetWithParentWithHandle_GetReturnsExpected(columnHeadersWidthSizeMode: AutoSize, columnHeadersVisible: True, autoSize: True, value: 4, expectedValue: 18, expectedInvalidatedCallC
• System.Windows.Forms.Tests.MenuStripTests.MenuStrip_WndProc_InvokeMouseActivate_Success
• System.Windows.Forms.Tests.MenuStripTests.MenuStrip_WndProc_InvokeMouseActivateWithHandle_Success
• System.Windows.Forms.Tests.TextBoxBaseTests.TextBoxBase_Copy_PasteNotEmptyWithHandle_Success
• System.Windows.Forms.Tests.TextBoxBaseTests.TextBoxBase_Paste_InvokeNotEmpty_Success
• System.Windows.Forms.Tests.ListViewItem_IKeyboardToolTipTests.ListViewItemKeyboardToolTip_InvokeIsHoveredWithMouse_ReturnsExpected(insideListView: True, virtualMode: True, isHovered: True, expected: True)
• System.Windows.Forms.Tests.TextBoxBaseTests.TextBoxBase_Undo_CanUndo_Success
• System.Windows.Forms.Tests.TextBoxBaseTests.TextBoxBase_Copy_PasteNotEmpty_Success
• System.Windows.Forms.Tests.BindingContextTests.BindingContext_Item_GetIListSourceDataSourceWithDataMemberReturningIListNull_ThrowsArgumentNullException
• System.Windows.Forms.Tests.ClipboardTests.Clipboard_SetText_InvokeString_GetReturnsExpected
• System.Windows.Forms.Tests.TreeNode_TreeNodeIKeyboardToolTipTests.TreeNodeIKeyboardToolTip_InvokeIsHoveredWithMouse_ReturnsExpected(insideTreeView: True, isHovered: True, expected: True)
• System.Windows.Forms.Tests.TextBoxBaseTests+ClipboardTests.TextBoxBase_Copy_PasteNotEmptyWithHandle_Success
• System.Windows.Forms.Tests.TreeViewAccessibilityObjectTests.TreeViewAccessibilityObject_Role_IsOutline_ByDefault
• System.Windows.Forms.Tests.TextBoxBaseTests+ClipboardTests.TextBoxBase_Undo_CanUndo_Success
• Microsoft.VisualBasic.Devices.Tests.NetworkTests.PingUri_ShortTimeout_Success
• System.Windows.Forms.Tests.AccessibleObjects.TextBoxAccessibleObjectTests.TextBoxAccessibilityObject_Role_IsText_ByDefault
• System.Windows.Forms.Tests.CheckBoxRendererTests.CheckBoxRenderer_DrawCheckBox_VisualStyleOn_OverloadWithTextFormat

[paul1956]

Tests involving shared resources like Clipboard are also flaky when run in parallel.

[LeafShi1]

Tests involving shared resources like Clipboard are also flaky when run in parallel.

The current list is Flaky problems that have appeared in PR builds in the past two weeks. Regarding the Clipboard-related errors you mentioned, if we encounter them in subsequent pipelines, I will update them in the issue list.

[weltkante]

Tests involving shared resources like Clipboard are also flaky when run in parallel.

In the past we tried to ensure those aren't run in parallel by adding annotation attributes to the test classes preventing that.

[paul1956]

@weltkante
Do you have example of required annotations?

One issue with Clipboard is there are tests in different test classes using it is that possible or do they all need to be in same class?

[weltkante]

@weltkante Do you have example of required annotations?

One issue with Clipboard is there are tests in different test classes using it is that possible or do they all need to be in same class?

Couldn't quickly find it regarding clipboard, but WebBrowser tests had the same issue (WebBrowser control didn't like being run from multiple UI threads and caused memory corruption) - see #3429

You basically put a [Collection("...")] attribute on the class with a string token and tests from all classes using the same collection string are run sequentially (tests within a class a run sequentially anyways).

[paul1956]

@weltkante thanks I found this when looking at Collection and it seems to address the whole issue even with tests writing in C# AND Visual Basic

<Collection("Clipboard Tests")>
<CollectionDefinition("Clipboard Tests", DisableParallelization:=True)>

[paul1956]

@LeafShi1 Clipboard tests are still Flakey, there 142 tests in 4 different projects. I think they need to be moved into 1 project/class or there needs to be a guaranteed way to make sure they don't run in parallel.
They all pass if run 1 at a time and there is no single test that fails every time.

[weltkante]

If they are properly annotated with the collection attribute (see above in the issues discussion) then I don't think the clipboard tests themselves are the issue, more likely they are the victim of an unannotated test thats accessing shared resources anyways.

(in other words, you won't fix it by moving them into a single project/class if that other tests are still accessing things in parallel)

[paul1956]

@weltkante very possible. The 142 tests I found are all annotated. But looking at the list of other flakey tests it's clear some also use clipboard. Do all tests that are annotated use the same name? Anything that relies on the clipboard should use the same name but it might not be obvious that a test is using it. Is there a list of names and what shared resources the name guards?
Is there a way to centralize that list and maybe make the name include the resource? Like "ClipboardGuard" just as an example.

[weltkante]

doesn't look like https://github.com/dotnet/winforms/blob/main/docs/testing.md has anything about it and I'm not aware of other documentation; might want to ping someone from the team for further discussion, it doesn't look like this issue is watched as far as github tells me

[paul1956]

@LeafShi1 @JeremyKuhne Can one of you get this on someone's radar. Where Clipboard use is obvious, the appropriate Attributes are added but in some cases the use of the Clipboard or other shared resource is not so clear and it's a side effect of the test. The current method of just disabling the failing tests is a Band-Aid because they are generally in an appropriately Attributed class.

[LeafShi1]

@LeafShi1 @JeremyKuhne Can one of you get this on someone's radar. Where Clipboard use is obvious, the appropriate Attributes are added but in some cases the use of the Clipboard or other shared resource is not so clear and it's a side effect of the test. The current method of just disabling the failing tests is a Band-Aid because they are generally in an appropriately Attributed class.

Tanya is working on resolving the clipboard issue, but it will take some time.

[paul1956]

@Tanya-Solyanik @LeafShi1 @JeremyKuhne I found that ControlTests,cs which uses the Clipboard does not have

[Collection("Sequential")]
[CollectionDefinition("Sequential", DisableParallelization = true)]

When I add that to the C# and VB files all my tests consistently pass. In my testing leaving the second line improves the stability but I have not done enough testing to prove its necessary. I have not seen any failures with the second line, without it on the VB side I see random failures.

[Tanya-Solyanik]

The fact that [CollectionDefinition("Sequential", DisableParallelization = true)] makes a difference indicates that we have still more Clipboard tests among other test classes, because this attribute ensures that all tests within this collection run sequentially after all other tests completed.

https://xunit.net/docs/running-tests-in-parallel

Turn off parallelism for specific Test Collection
  [CollectionDefinition(DisableParallelization = true)] (placed on the collection definition class)
Default: false
Parallel-capable test collections will be run first (in parallel), followed by parallel-disabled test collections (run sequentially).

@LeafShi1 - is it possible to find specific tests in the ControlTests class that use the Clipboard and move them to the sequential collection??

[paul1956]

@Tanya-Solyanik there are tests being added daily it seems, I think there should be some guidance to test writers about what resource can't be shared, Clipboard is just the most obvious. Also originally I used a name other that "Sequential" to separate Clipboard from other things like WebView2 control (Clipboard and WebView2 control run in parallel fine). I did find that tests in VB and C# in multiple different classes/files/languages using Clipboard can avoid collisions by using "Sequential" so there really is not a reason to collect all of them into one giant file, the Annotation is the important thing.

[Tanya-Solyanik]

@paul1956 - we are moving clipboard tests into new classes, not adding them to ClipboardTests.cs file -

winforms/src/System.Windows.Forms/tests/UnitTests/TextBoxBaseTests.ClipboardTests.cs

Line 8 in 34298ef

[Collection("Sequential")]

[LeafShi1]

@LeafShi1 - is it possible to find specific tests in the ControlTests class that use the Clipboard and move them to the sequential collection??

Currently only RichTextBoxTests also contains clipboard tests, for which I submitted a new PR #11936.

[paul1956]

@LeafShi1 I found tests that use Clipboard, in some cases it's just a couple of test and in other file its many tests spread throughout the file.

ToolStripTextBoxTests.cs Multiple Tests
ClipboardComTests.cs This file has 2 test
DataGridViewCellTests.cs Multiple Tests
DataObjectTests Multiple Tests

I like the idea of moving the tests that use Clipboard to OriginalFileName.Clipboard.cs and Annotating it but it is a one-person job to avoid merge conflicts.

I stopped looking but there are more.

@Tanya-Solyanik I think the name "Sequential" should be changed to "ClipboardSequential", "WebBrowserSequential" and any other specific shared resource.

[Tanya-Solyanik]

@Tanya-Solyanik I think the name "Sequential" should be changed to "ClipboardSequential", "WebBrowserSequential" and any other specific shared resource.

Perhaps at some point, if we see that tests are running too slowly.

[paul1956]

@Tanya-Solyanik it would also make it clear what resource is being protected, but I understand not wanting to do it unless it's absolutely necessary.

Still failing Clipboard tests

[LeafShi1]

@Tanya-Solyanik @paul1956 I searched the entire project using keywords (clipboard, copy, paste) and found that the following test case used the clipboard

Under Microsoft.VisualBasic.Devices.Tests:

• ComputerTests.cs
• ClipboardProxyTests.cs

Under System.Windows.Forms.Tests:

• ClipboardComTests.cs (It has been added in collection "Sequential")
• ClipboardTests.cs (It has been added in collection "Sequential")
• TextBoxBaseTests.ClipboardTests.cs (It has been added in collection "Sequential")
• RichTextBoxTests.ClipboardTests.cs (Created in PR Moving Clipboard related tests to a new class and combine the ClipboardTests class #11936)
• DataFormatsTests.cs `
• DataGridViewCellTests.cs
• DataObjectTests.cs (DataFormats)
• DragDropFormatTests.cs (PInvoke.RegisterClipboardFormat)

The bottom four files call DataFormats/PInvoke.RegisterClipboardFormat without using the clipboard's copy/paste/clipboard.SetText... functions, so I did not move them to the collection Sequential.

For these cases, do we really need to move them to collection Sequential? If so, I will move them in next PR.
In addition, I created issue #11948 to track the clipboard tests.

For cases that still fail after adding collection "Sequential", it may be because of other clipboard tests that are not in the collection.

[paul1956]

@LeafShi1 if the Clipboard Data Format is changed or tested other tests will fail if they depend on the value of data format.

To be safe I think everything that uses the Clipboard should be guarded by the Annotation. I don't see how it hurts, except to lengthen testing by a tiny amount of time. Overlapping Annotated tests with different names (ie different shared resources) would easily mask that.

[Tanya-Solyanik]

DataObjectTests mostly work with our custom DataObject.DataStore implementation. Good point about registering the same formates across different tests. Perhaps we should enforce that each test registers format named as nameof(test name)