-
Notifications
You must be signed in to change notification settings - Fork 990
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
update Xunit.StaFact to include bugfixes #3276
Conversation
Both tests timed out, any screenshots or other hint what was hanging? (I don't think I have access to these build artifacts) |
Thanks, I'll have a look |
The three The CPU usage is a polling message loop, can be easily fixed on stafact side, but it doesn't affect the hang. More notes in the issue for this PR, I also created stafact issues for further investigation. |
Updating the package fails due to #3297, I've commented out the offending tests (can revert that if #3297 is fixed before this is merged), that did let the tests pass locally, we'll see how CI does. PS: Skipping them doesn't work for weird reasons I don't understand, xunit complains about missing theory parameters on the skipped test, at least on my local run. |
CI build seems to be still hanging even with the bugfixes, not sure how to diagnose, cannot reproduce locally. I opened #3298 as one suggestion how to diagnose these hangs better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to update, please skip tests instead of commenting out
...indows.Forms/tests/UnitTests/System/Windows/Forms/WindowsFormsSynchronizationContextTests.cs
Outdated
Show resolved
Hide resolved
I commented out
|
These tests also started failing:
|
Once I have skipped I have also replaced
I can't see what is so special about this test, all other tests have exactly the same arrangement to the same line... |
SystemEvents had reliability issues because it creates a global HWND (outside WinForms codebase) on the first STA thread using it, ignoring the possibility that thread may shut down before the process exits. I didn't follow it closely but has something been turned to skipping in response to #3254 ? If not then this is probably a hang unrelated to upgrading xunit.stafact - the change in stafact implementation may just change the rate at which this triggers. Odd that it didn't trigger locally for me, did several test runs, none were hanging. I'll try to identify responsible tests and mark them skipped, seeing if that makes CI stop hanging.
I'll have to look into that and see if that fails locally. Not having clean test runs due to reliance on english locale makes it hard to notice new failures. |
The offending test seems to have been skipped, not sure about whether the hangs are related to SystemEvents. The thread seems to be in WndProc so that indicates it has a message loop, might be a false alarm. Still can't reproduce any hang locally.
To my knowledge this only happens when you make inappropriate use of However when looking at the call chains this doesn't make any sense at all, nobody is calling either of those methods from what I can tell.
|
Not helping, still failing on STA threads with property disabled OLE, disabling those tests and creating an issue, this needs technical investigation, its beyond my level of expertise.
|
Cannot figure out whats up with that failing clipboard test. It seems consistent and not flaky. The test is a bit fishy by testing the clipboard before putting the text there, but I'd still have assumed VB and WinForms return consistent results (and they do locally, cannot reproduce CI failures) |
The last puzzle piece that seems to be missing is that the |
Rebased for second ITypeInfo PR and also removed some changes from WIP commit to start figuring out which of those changes are still significant. |
DoDragDrop tests are definitely still hanging. Not sure if its good to have them either, they should be integration tests instead as proper drag'n'drop tests needs mouse interaction, otherwise OLE probably exits out very early. These tests basically just cover a failure case which in practice nobody is interested in (and can easily covered by the integration test as well). Sure its useful to ensure that a noop failure case stays a noop failure case but it may give a false sense of test coverage. Not creating an issue yet, but if this still hangs after fixing the sequential execution in xunit.stafact I'd suggest removing them from unit tests and building an integration test instead. |
This is weird, the last two CI runs have been passing. Not sure what changed since then. I'm going to start removing skips and/or creating issues for them if we have to keep them. |
Codecov Report
@@ Coverage Diff @@
## master #3276 +/- ##
====================================================
+ Coverage 64.68177% 98.63044% +33.94866%
====================================================
Files 1318 430 -888
Lines 484206 230439 -253767
Branches 39910 3142 -36768
====================================================
- Hits 313193 227283 -85910
+ Misses 165638 2523 -163115
+ Partials 5375 633 -4742
|
@RussKie I have no idea what happened but the last three CI runs were successful and I don't have any local failures anymore either (did quite a few runs). I've cleaned up the PR and think its ready for review now. If the ITypeInfo tests should become flaky again ping me, I still have an idea what to look for since I had it several times happen in my debugger, but now it doesn't. The [edit] was able to repro the "having one thread per method in debugger" - all but one thread were ready to terminate so probably misinterpreted what was happening. Still don't know why the test failures suddenly disappeared, maybe #3321 was really all that was missing. |
Thank you for taking the initiative on this.
👍 |
Fixes #3122
Proposed changes
Update Xunit.StaFact package to consume bugfixes made upstream
Customer Impact
None, test infrastructure only
Regression?
No
Risk
Unknown issues may be introdcued by updating 3rd party packages, but getting fixes for already known issues should outweight that risk.
Before
Unable to run WinFormsTheory with complex MemberData inside VS
After
Able to run WinFormsTheory with complex MemberData inside VS
(+ various other fixes I don't know exactly how to test for, like calling OleRequired to initialize OLE on the STA thread)
Test methodology
Making sure one of the known issues is actually fixed. I didn't explicitly test for other fixes that have been merged.
Test environment
local
Microsoft Reviewers: Open in CodeFlow