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

Replace xUnit with mstest #1883

Merged
merged 19 commits into from
Mar 30, 2023
Merged

Replace xUnit with mstest #1883

merged 19 commits into from
Mar 30, 2023

Conversation

idg10
Copy link
Collaborator

@idg10 idg10 commented Mar 6, 2023

The TFM upgrades in #1882 were a necessary step in being able to build Rx on current tooling, but not the only one. This change addresses the fact that xUnit is no longer able to run tests on UWP. MSTest supports this in Visual Studio 2022, so we are moving back to MSTest. (Rx used to used MSTest before, so this is a relatively straightforward change.)

xUnit is no longer able to run tests on UWP. MSTest supports this in
Visual Studio 2022, so we are moving back to MSTest. (Rx used to used
MSTest before, so this was a relatively straightforward change.)
@idg10 idg10 added the [area] Rx label Mar 6, 2023
@idg10 idg10 self-assigned this Mar 6, 2023
@idg10
Copy link
Collaborator Author

idg10 commented Mar 6, 2023

Note that a handful of tests stopped working because xUnit (somewhat controversially) set SynchronizationContext.Current to be non-null, and some tests were effectively relying on that. We need to investigate for each of these whether we should have versions of the tests that run with and without a synchronization context, since the behaviour is evidently dependent on that.

These are all currently labelled with a comment with "idg10" in the text in this branch. We will be fixing this as part of this PR.

idg10 added 18 commits March 6, 2023 12:37
Apparently supplying a SynchronizationContext isn't enough to prevent the EventLoop_ScheduleActionNested and EventLoop_ScheduleActionDue tests from hanging on the build agent.

We'll need to fix this properly, so #1885 will track this
Fix race condition in CreateAsyncTest

Make TaskLikeSupportTests run both with and without SynchronizationContext

Revert the -v n option for test - hoping we now won't need it, now that we've configured test timeouts
We had --filter "SkipCI!=true" but according to the docs, what we want is --filter "TestCategory!=SkipCI"
We're still using xUnit's assertions. And we're also still using xUnit in the test project that verifies that the API surface area didn't change.

The newer API generator libraries have fixed some bugs - the 'verified' versions of these files used to have some missing type arguments. Any params argument using a generic type would just have an empty <>. Also, handling of the System.AttributeTargets looks like it used to be broken. The various .verified.cs files representing the expected API surface area have been modified so that they no longer have to work around these issues.

Unfortunately xunit.assert.source appears to have a bug causing it to inject a public type, so we've had to work around that.
Disable the handful of these that can't be expected to work on the current UWP implementation
@idg10 idg10 marked this pull request as ready for review March 30, 2023 09:04
@idg10 idg10 added this to the Rx 6.0 milestone Mar 30, 2023
@idg10
Copy link
Collaborator Author

idg10 commented Mar 30, 2023

We have made a few changes to deal with the SynchronizationContext issues. However, some of the test failures appear to have been 'fixed' as a side effect of enabling default test timeouts. This means there are probably still issues lurking there somewhere. #1885 remains open so we can track this in the longer run, but since the failures can't now be reproduced, there's no need to block this particular PR. (If there really are issues in the underlying Rx code that caused these failures, they were there before this PR too.)

@idg10 idg10 merged commit 44a28a8 into main Mar 30, 2023
@idg10 idg10 deleted the tooling/mstest branch March 30, 2023 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants