chore: update to xunit 2 #1380

olevett · 2017-06-03T21:17:34Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fixes #1301

What is the current behavior? (You can also link to an open issue here)
Uses xunit 1.9.x

What is the new behavior (if this is a feature change)?
Use xunit 2.x

Does this PR introduce a breaking change?
Unlikely, this shouldn't be being consumed by end users anyway.

Please check if the PR fulfills these requirements

The commit follows our guidelines: https://github.com/reactiveui/reactiveui#contribute
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:
The biggest change that impacts this is the threading model for xunit 2 has changed a bit, so some tests are marked as STA or WPF which ensures they're running on the right thread.

I'm a little concerned the tests could be a bit flaky because of the changes, so one thing I'm keen for this PR to do is build it on a different machine.

If tests are flaky, we might need to either rework them a bit, possibly doing one of the following

explicitly set a thread/Rx dispatcher
change the way RxApp handles unit test runners
use async/await (:( I know...) because that makes xunit play nice

olevett · 2017-06-03T21:18:37Z

src/ReactiveUI.Tests/MessageBusTest.cs

            int thisThread = Thread.CurrentThread.ManagedThreadId;

-            Task.Run(() => {
+            await Task.Run(() => {


This wasn't that stable locally, but doing async/await rather than Wait() seemed to make it happier

Yeah, calling Wait is a big no-no. See here.

Hmm, hadn't seen that, but does explain things! I think this was the only consistent deadlock, so hopefully we're not doing it elsewhere!

olevett · 2017-06-03T21:19:47Z

src/ReactiveUI.Tests/Platforms/xaml/ActivationForViewFetcherTest.cs


            var obs = activation.GetActivationForView(uc);
-            var activated = obs.CreateCollection();
+            var activated = obs.CreateCollection(scheduler: ImmediateScheduler.Instance);


I get the feeling this shouldn't be necessary, but it looks like the way RxApp works, sometimes MainThreadScheduler might not be set as ImmediateScheduler in unit tests (we set _MainThreadScheduler but _UnitTestScheduler wins in general).

Yep, this is the exact problem I ran into with the SchedulerShouldBeCurrentThreadInTestRunner test under NCrunch. I dream of a day we can completely remove ambient contexts from ReactiveUI.

olevett · 2017-06-03T21:21:06Z

src/ReactiveUI.Tests/TestConfiguration.cs

@@ -0,0 +1,3 @@
+using Xunit;
+
+[assembly: CollectionBehavior(DisableTestParallelization = true, MaxParallelThreads = 1)]


This is to make xunit2 behave more like xunit 1.

coveralls · 2017-06-03T21:23:02Z

Coverage remained the same at 66.835% when pulling 7eac9d9 on olevett:#1301-xunit-update into 9699903 on reactiveui:develop.

kentcb · 2017-06-13T08:52:16Z

This LGTM @olevett!

Using Test Explorer, I can build and successfully run all tests (assuming I unload the Events* and AndroidSupport projects, since they fail to build on my machine).

What's interesting is that NCrunch seems to be working much better, too. I had to update the config (which I can PR separately once this is merged), but now only one test fails. And the test in question is SchedulerShouldBeCurrentThreadInTestRunner, which seems to be failing for the usual reasons of the scheduler ambient context being incredibly confusing and flaky code. It will be great to be able to use NCrunch instead of Test Explorer!

kentcb · 2017-06-13T09:24:09Z

Ugh, I wanted to merge this, but build is failing with OpenCover error now. Am guessing it's an intermittent problem. I don't think there's an easy way for me to just force another build 🤔 Maybe @ghuntley can chime in?

ghuntley · 2017-06-13T13:57:45Z

olevett · 2017-06-13T14:10:46Z

That test seems to be the most flakey of the lot, but also, if it's not working possibly a lot of other tests won't behave quite right.

Will spend some more time digging at it, unless anyone has better ideas!

kentcb · 2017-06-15T10:03:21Z

Wow, the RxApp code frigging sux. I can see what's happening here, but no idea how it ever worked, nor how to solve this problem for good. Here's the sequence of events:

RxApp static ctor is executing, which calls Locator.CurrentMutable.InitializeReactiveUI()
This results in platform registrations being registered, which in turn sets RxApp.MainThreadScheduler to new WaitForDispatcherScheduler(() => DispatcherScheduler.Current)
The RxApp static ctor continues executing and assigns _MainThreadScheduler = CurrentThreadScheduler.Instance
RxApp.MainThreadScheduler then proceeds to ignore that value because __UnitTestMainThreadScheduler is set to a different value

I'm 99% sure the bug is in the static ctor. Instead of assigning to _MainThreadScheduler, I suspect it should be assigning to the property itself. When I do that, the test in question passes, but now a bunch of other tests fail or, worse, stall completely. sigh

Will also dig a bit further...

olevett · 2017-06-16T11:10:56Z

I'm wondering if part of this is due to the ThreadStatic _UnitTestMainThreadScheduler/_UnitTestTaskpoolScheduler. With these, order and threading matters. (e.g. if RxApp is initialized on one thread, but gets used on another, the value of MainThreadScheduler does matter).

I'm not entirely sure if the ThreadStatic stuff actually makes that much sense with schedulers as TestUtils.WithScheduler is gated to prevent multiple overrides of the scheduler (you could conceivably do this some other way which I guess makes any change here a breaking one anyway). The other cases it gets thrown around are quite difficult to trace and understand, but may be more helpful. Just removing the ThreadStatic stuff does however cause a whole load of tests, so maybe it is important...

Also, I think SchedulerShouldBeCurrentThreadInTestRunner relies on being a different thread to the first static constructor call (which does the RxApp->InitializeReactiveUI() call), otherwise as @kentcb points out, the initialise sets _UnitTestMainThreadScheduler to be the WaitForDispatcher scheduler. If it's the only test run it seems to fail, if it's run as part of a block of tests, it passes. I'd argue it's a bad test, though there are clearly other problems around schedulers!

(see reactiveui#1380)

olevett · 2017-06-19T12:23:57Z

As a quick test of building etc, I've disabled the scheduler test that's failing - I don't think this is great (either we should fix it or 🔥 it) but I want to make sure CI can consistently build and run the other tests...

(see reactiveui#1380)

coveralls · 2017-06-23T14:39:52Z

Coverage decreased (-39.6%) to 27.281% when pulling be2021a on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-06-23T14:47:44Z

Coverage decreased (-37.4%) to 29.428% when pulling 636c45f on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-06-23T15:18:47Z

Coverage decreased (-37.4%) to 29.397% when pulling 4d13057 on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-06-23T15:34:04Z

Coverage decreased (-35.07%) to 31.765% when pulling 964400c on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-06-24T12:51:15Z

Coverage decreased (-33.0%) to 33.849% when pulling 7513480 on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-06-24T13:53:58Z

Coverage decreased (-29.3%) to 37.512% when pulling 36c6911 on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

olevett · 2017-06-24T13:57:54Z

As per suggestion from @kentcb I've been skipping most tests and then incrementally adding them back in. I've been using a quick extra build process to run tests 10 times locally which should hopefully catch out any intermittent failures, I've included this as cake task called MinimalBuildAndTest. I'd like to run this a much higher number of times before I'm completely happy, but tests so far don't seem to be crazily broken (I've made a couple of changes like explicit schedulers, but nothing enormous).

coveralls · 2017-06-24T14:00:06Z

Coverage decreased (-14.1%) to 52.731% when pulling 64c8166 on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

kentcb · 2017-06-25T00:02:26Z

@olevett I had a crazy idea that I was gonna try, but haven't had the time...it may not work, but let me run it by you in case it helps.

I was thinking I'd add a console app to the solution, it would reference unit tests. The console app would:

reflectively find all Facts (think we can ignore theories because they complicate matters and not sure there even are any)
randomize the order of the found tests
run all found tests in the randomized order n * d times, where n is some constant (let's say 100) and d is the depth of recursion to be discussed shortly...
if failure is detected, we have found a set of tests that fail when executed together (save this off in a variable for later reference)
split the set of tests in two and recursively repeat the above process, increasing d by 1

Thus we'd whittle down the tests to a very small set that fail when executed together. It's not perfect, I know, but it could be an automated way to whittle the situation down to just a handful of tests quite quickly.

Anyway, just an idea that may amount to nothing, but thought I'd pass it on since it's unlikely I'm going to find the time to do this myself.

olevett · 2017-06-25T07:10:14Z

@kentcb tht sounds like a neat solution, at the moment I've got a cake build that does the iterations part (the easy bit!) My one concern would be the test that causes issues might be a passing test, so only running failing tests might not pick it up correctly. Not sure if that’s a valid concern.

kentcb · 2017-06-25T08:40:29Z

My one concern would be the test that causes issues might be a passing test

Yeah, that's why I'm suggesting to find groups of tests where any test within the group fails (didn't word that very clearly initially). And the randomisation gives ample opportunity to find combinations of tests that might all pass individually, but not when executed together.

Ultimately, if this automation could narrow down to a small set of e.g. 10 or so tests (or even 2!) that cannot be executed together without occasional failures, would save a lot of your time.

coveralls · 2017-06-28T19:50:03Z

Coverage decreased (-10.3%) to 56.584% when pulling a0ca8c0 on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-06-28T20:14:02Z

Coverage decreased (-8.5%) to 58.383% when pulling 348d9a3 on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-06-28T20:47:57Z

Coverage decreased (-5.9%) to 60.909% when pulling 8b1bdd2 on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

olevett · 2017-06-28T21:07:29Z

src/ReactiveUI.Tests/TestUtilsTest.cs

    public class TestUtilsTest
    {
-        [Fact]
-        public async Task WithAsyncScheduler()


This test just seemed to break everything... I'm not even really sure what it's testing, so thought losing it seemed safest...

olevett · 2017-06-28T21:07:30Z

src/ReactiveUI.Tests/TestUtilsTest.cs

    public class TestUtilsTest
    {
-        [Fact]
-        public async Task WithAsyncScheduler()


This test just seemed to break everything... I'm not even really sure what it's testing, so thought losing it seemed safest...

olevett · 2017-06-28T21:15:24Z

I think this is getting pretty near to being reliable, if anyone's interested in the breakdown or a build script that thrashes the tests a few times to pick up threading quirks, there's a branch hhere but I've squashed down to reduce the noise a bit.

coveralls · 2017-06-28T21:15:25Z

Coverage increased (+0.09%) to 66.93% when pulling 463c3cc on olevett:#1301-xunit-update into b6209cd on reactiveui:develop.

coveralls · 2017-07-24T10:27:59Z

Coverage increased (+0.09%) to 66.93% when pulling 6e6dd89 on olevett:#1301-xunit-update into 7221f58 on reactiveui:develop.

coveralls · 2017-07-24T15:21:18Z

Coverage increased (+0.09%) to 66.93% when pulling 2c1acea on olevett:#1301-xunit-update into 7221f58 on reactiveui:develop.

Fixes reactiveui#1301

olevett · 2017-08-05T15:02:38Z

Rebased because file header changes confused git...

coveralls · 2017-08-05T15:06:45Z

Coverage increased (+0.06%) to 66.635% when pulling 663fa00 on olevett:#1301-xunit-update into fea42cd on reactiveui:develop.

Fixes #1301

olevett commented Jun 3, 2017

View reviewed changes

kentcb added the housekeeping label Jun 13, 2017

olevett added a commit to olevett/ReactiveUI that referenced this pull request Jun 19, 2017

Skip scheduler test

7cb4e9b

(see reactiveui#1380)

olevett force-pushed the #1301-xunit-update branch from 4a1bc5f to 9896022 Compare June 21, 2017 15:22

olevett added a commit to olevett/ReactiveUI that referenced this pull request Jun 21, 2017

Skip RxAppTest and ensure initialized correctly

9896022

(see reactiveui#1380)

olevett force-pushed the #1301-xunit-update branch from d2f569c to 0820e2e Compare June 22, 2017 10:43

olevett force-pushed the #1301-xunit-update branch from 11ea79a to 964400c Compare June 23, 2017 15:28

olevett commented Jun 28, 2017

View reviewed changes

olevett force-pushed the #1301-xunit-update branch from d4d9fc2 to 463c3cc Compare June 28, 2017 21:09

olevett force-pushed the #1301-xunit-update branch from 6e6dd89 to 2c1acea Compare July 24, 2017 15:15

olevett added 2 commits August 5, 2017 15:40

chore: update to xunit 2

4a9cd1d

Fixes reactiveui#1301

Set test schedulers and update tests to ensure passing

663fa00

olevett force-pushed the #1301-xunit-update branch from 2c1acea to 663fa00 Compare August 5, 2017 15:01

olevett merged commit e8b6c0b into reactiveui:develop Aug 10, 2017

olevett deleted the #1301-xunit-update branch August 10, 2017 10:15

ghuntley pushed a commit that referenced this pull request Nov 6, 2017

chore: update to xunit 2 (#1380)

1e5132d

Fixes #1301

glennawatson pushed a commit that referenced this pull request Mar 23, 2019

chore: update to xunit 2 (#1380)

c3e5865

Fixes #1301

lock bot locked and limited conversation to collaborators Jun 26, 2019

		@@ -0,0 +1,3 @@
		using Xunit;

		[assembly: CollectionBehavior(DisableTestParallelization = true, MaxParallelThreads = 1)]

Uh oh!

chore: update to xunit 2 #1380

chore: update to xunit 2 #1380

Uh oh!

Conversation

olevett commented Jun 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 3, 2017

Uh oh!

kentcb commented Jun 13, 2017

Uh oh!

kentcb commented Jun 13, 2017

Uh oh!

ghuntley commented Jun 13, 2017

Uh oh!

olevett commented Jun 13, 2017

Uh oh!

kentcb commented Jun 15, 2017

Uh oh!

olevett commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olevett commented Jun 19, 2017

Uh oh!

coveralls commented Jun 23, 2017

Uh oh!

coveralls commented Jun 23, 2017

Uh oh!

coveralls commented Jun 23, 2017

Uh oh!

coveralls commented Jun 23, 2017

Uh oh!

coveralls commented Jun 24, 2017

Uh oh!

coveralls commented Jun 24, 2017

Uh oh!

olevett commented Jun 24, 2017

Uh oh!

coveralls commented Jun 24, 2017

Uh oh!

kentcb commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olevett commented Jun 25, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentcb commented Jun 25, 2017

Uh oh!

coveralls commented Jun 28, 2017

Uh oh!

coveralls commented Jun 28, 2017

Uh oh!

coveralls commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olevett commented Jun 28, 2017

Uh oh!

coveralls commented Jun 28, 2017

Uh oh!

coveralls commented Jul 24, 2017

Uh oh!

coveralls commented Jul 24, 2017

Uh oh!

olevett commented Aug 5, 2017

Uh oh!

olevett commented Jun 16, 2017 •

edited

Loading

kentcb commented Jun 25, 2017 •

edited

Loading

olevett commented Jun 25, 2017 via email •

edited

Loading