Move to xunit.v3

[Youssef1313]

Fixes #

Proposed changes

• Migrates from xunit to xunit.v3

Customer Impact

• None. This is test-only changes

Regression?

• No

Risk

• Very low.

Test methodology

Microsoft Reviewers: Open in CodeFlow

[Youssef1313]

Test Form_DpiChanged_MinMaxSizeNotChanged_Default is failing consistently at least in CI. This needs some investigation.

ConvertManagedToNative_NullObject is also still failing even with the last change. I need to look again.

[Youssef1313]

For Form_DpiChanged_MinMaxSizeNotChanged_Default, it was written in #7467

The PR description says that changing DPI should affect min size and max size, but the test asserts that they actually don't change. The test as written in the PR contained a factor variable that was unused. So I think the intent is that min/max size change by that factor, at least that's what the PR description suggests. What is curious there is how this test ever passed before.

cc @RussKie

[Youssef1313]

Regarding Form_DpiChanged_MinMaxSizeNotChanged_Default, I investigated further. Before the migration to MTP, we are run under VSTest's testhost which was .NET Core 2.1:

So,

winforms/src/System.Windows.Forms.Primitives/src/System/LocalAppContextSwitches/LocalAppContextSwitches.cs

Line 117 in 86f7d0f

if (framework.Version.Major >= 8)

was false :)

[codecov]

Codecov Report

Attention: Patch coverage is 91.13924% with 14 lines in your changes missing coverage. Please review.

Project coverage is 76.61378%. Comparing base (44e2900) to head (559a770).
Report is 3 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #13540         +/-   ##
===================================================
+ Coverage   76.59914%   76.61378%   +0.01463%     
===================================================
  Files           3236        3235          -1     
  Lines         639344      639351          +7     
  Branches       47314       47313          -1     
===================================================
+ Hits          489732      489831         +99     
+ Misses        146085      145994         -91     
+ Partials        3527        3526          -1     

Flag	Coverage Δ
Debug	76.61378% <91.13924%> (+0.01463%)	⬆️
integration	18.54353% <0.00000%> (+0.00280%)	⬆️
production	51.02011% <0.00000%> (+0.01744%)	⬆️
test	97.41264% <91.71975%> (+0.01196%)	⬆️
unit	48.40983% <0.00000%> (+0.01220%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Youssef1313]

Thanks for review @JeremyKuhne. I addressed your comment about COM2PictureConverter. This is ready for another review, and if you can investigate the few remaining test failures please.

[merriemcgaw]

@LeafShi1 can you help investigate any remaining test failures related to this?

[LeafShi1]

@Youssef1313 The actual problem cannot be seen from the current failure log, and the failure case cannot be reproduced in the local environment. You can try to optimize these two failure cases so that the failure reason can be seen more clearly when it fails.

Perhaps you can try to give ToolStripItem an initial name and assert whether the ToolStripItem name is equal

1. Replace Assert.True(toolStrip1.Items[0].Selected); in the test Control_SelectNextControl_ToolStrips_CycleBackwardExpected with Assert.Equal(toolStrip1.GetSelectedItem().Name, toolStrip1.Items[0].Name);

2. Replace Assert.Equal(toolStripButton3, actual); in test ToolStrip_GetNextItem_ReturnsBackwardItem with Assert.Equal(toolStripButton3.Name, actual.Name);

[Youssef1313]

One thing to explore here is that the built assemblies in the x86 job are actually AnyCPU. Previously, they would run under an x86 dotnet.exe. But after the change in this PR, they are run as x64.

For xunit 2, it happens via https://github.com/dotnet/arcade/blob/b305863166c975997aafee78cf69942e7d2f862a/src/Microsoft.DotNet.Arcade.Sdk/tools/XUnit/XUnit.Runner.targets#L31

[AArnott]

Do you have a custom test method runner? Is it possible that it has somehow caused an OOM?

This repo uses xunit.stafact. It's hard to tell if that's the cause or not. And the stack trace isn't pointing to anything useful. cc @AArnott Do you see anything here that could somehow be caused by xunit.stafact?

I am not aware of any problem like that.

[Youssef1313]

@AArnott Thanks. It was caused by xunit's source info calculation which is disabled by default now in 2.0.3. PR is green now 🎉

@merriemcgaw @LeafShi1 @JeremyKuhne Can I please get reviews so we can hopefully get this merged soon?

[bradwilson]

Does the above snippet mean that both RunTestCases and OnTestMethodFinished use the same aggregator? How can OnTestMethodFinished know if the exception happened during RunTestCases vs OnTestMethodFinished.

It's the same aggregator, but what OnTestMethodFinished sees would be from things before OnTestMethodFinished is called. If OnTestMethodFinished threw, then it would be on the next observer to see the exception in the aggregator. So in that sense, OnTestMethodFinished can only see things that came from before it, which is going to be RunTestCases (which enumerates the test cases, calling RunTestCase on each, by default).

Because my current understanding from what you said is that OOM happens during OnTestMethodFinished so the test has already been executed correctly? Or am I missing something here?

It is OnTestMethodFinished which observes the exception(s) from RunTestCases to send the test method cleanup failure message. It's also important to note that exceptions during RunTestCases are unusual; exceptions from failing tests are normally caught to convert into a failing test result. It's surprising to see anything throwing from RunTestCases, generally speaking. (There are other cleanup errors which are more common, like for example class cleanup failures that are generated by virtue of a class fixture throwing in Dispose/DisposeAsync. There are no method fixtures or any other extensibility point where you might normally expect the possibility of exceptions.)

It was caused by xunit's source info calculation which is disabled by default now in 2.0.3.

It's also worth noting that 3.0.0 moves away from Cecil and uses [CallerFilePath] and [CallerLineNumber] for source information, which should hopefully take the memory pressure away even in scenarios where it's requested.

[JeremyKuhne]

Thanks for the work here!