Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unstable unit tests that need to be disabled and fixed #11227

Open
22 of 33 tasks
LeafShi1 opened this issue Apr 17, 2024 · 23 comments
Open
22 of 33 tasks

Unstable unit tests that need to be disabled and fixed #11227

LeafShi1 opened this issue Apr 17, 2024 · 23 comments
Assignees

Comments

@LeafShi1
Copy link
Member

LeafShi1 commented Apr 17, 2024

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
  • System.Windows.Forms.UITests.MonthCalendarTests.MonthCalendar_Click_Date_InvokeEventsAsync(delta: -1)
@paul1956
Copy link
Contributor

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

@LeafShi1
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

weltkante commented Aug 13, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link
Contributor

paul1956 commented Aug 18, 2024

@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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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

@LeafShi1
Copy link
Member Author

@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
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

paul1956 commented Aug 20, 2024

@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
image

@LeafShi1
Copy link
Member Author

LeafShi1 commented Aug 21, 2024

@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
Copy link
Contributor

@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
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants