-
Notifications
You must be signed in to change notification settings - Fork 984
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
Comments
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. |
In the past we tried to ensure those aren't run in parallel by adding annotation attributes to the test classes preventing that. |
@weltkante 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 |
@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
|
@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. |
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) |
@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? |
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 |
@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. |
@Tanya-Solyanik @LeafShi1 @JeremyKuhne I found that ControlTests,cs which uses the Clipboard does not have
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. |
The fact that https://xunit.net/docs/running-tests-in-parallel
@LeafShi1 - is it possible to find specific tests in the ControlTests class that use the Clipboard and move them to the sequential collection?? |
@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. |
@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
|
@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 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. |
Perhaps at some point, if we see that tests are running too slowly. |
@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. |
@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:
Under System.Windows.Forms.Tests:
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. For cases that still fail after adding collection "Sequential", it may be because of other clipboard tests that are not in the collection. |
@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. |
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 |
Disable failing tests and open tracking bugs
disabled-test
label to the bug.APIs to invoke methods
SendInput
, then make sure it is moved to the UI Integration tests assemblyThe following unit tests exist flaky issues:
The text was updated successfully, but these errors were encountered: