Upgrading NUnit and now fully using NUnitAdapter #381

ghost · 2018-02-14T20:32:27Z

This addresses: #243

This was actually mega easy to do.

dotnet test is a first class citizen for Castle Windsor.

Please note there is a TPL test that failed for netcoreapp1.0, I will add a review flagging it so we can discuss.

ghost · 2018-02-14T20:38:47Z

src/Castle.Windsor.Tests/ConventionVerification.cs

@@ -26,7 +26,8 @@ namespace CastleTests

 	using NUnit.Framework;

-	[Explicit]
+	[Ignore("Explicit Test, this fails on dotnet core")]


This is ugly, I might raise an issue on NUnit for the adapter.

Sounds like a defect in the NUnit runner.

I don't know why this test is failing and wasn't being run, but it appears we don't really need it anymore as we are nearly down to no obsolete members on those interfaces and classes, so we can just get rid of the whole test.

ghost · 2018-02-14T20:41:39Z

src/Castle.Windsor.Tests/Lifestyle/ScopedLifestyleExplicitAndMultipleThreadsTestCase.cs

@@ -102,6 +103,7 @@ public void Context_is_passed_onto_the_next_thread_TPL()
 				Assert.AreSame(instance, instanceFromOtherThread);
 			}
 		}
+#endif


This failed for netcoreapp1.0. I am not sure why yet. I will investigate this a little more before we merge this PR. If you have any insights here I am open to suggestions. A cursory and glossy investigation is telling me this should pass. There might be a gremlin here.

Could you remove the conditional compilation so the pull request fails and I can see the error, or just paste the exception here.

Here is the full failure:

Failed Context_is_passed_onto_the_next_thread_TPL Error Message: System.AggregateException : One or more errors occurred. ( Expected: not equal to 6 But was: 6 ) ----> NUnit.Framework.AssertionException : Expected: not equal to 6 But was: 6 Stack Trace: at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions) at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken) at CastleTests.Lifestyle.ScopedLifestyleExplicitAndMultipleThreadsTestCase.Context_is_passed_onto_the_next_thread_TPL() in C:\code\Windsor\src\Castle.Windsor.Tests\Lifestyle\ScopedLifestyleExplicitAndMultipleThreadsTestCase.cs:line 102 --AssertionException at NUnit.Framework.Assert.ReportFailure(String message) at NUnit.Framework.Assert.AreNotEqual(Object expected, Object actual) at CastleTests.Lifestyle.ScopedLifestyleExplicitAndMultipleThreadsTestCase.<>c__DisplayClass1_0.<Context_is_passed_onto_the_next_thread_TPL>b__0() in C:\code\Windsor\src\Castle.Windsor.Tests\Lifestyle\ScopedLifestyleExplicitAndMultipleThreadsTestCase.cs:line 99 at System.Threading.Tasks.Task.Execute()

Looks like we are in a good place, dotnet test is a first class citizen.

jonorossi · 2018-02-15T03:32:38Z

src/Castle.Windsor.Tests/ConventionVerification.cs

@@ -26,7 +26,8 @@ namespace CastleTests

 	using NUnit.Framework;

-	[Explicit]
+	[Ignore("Explicit Test, this fails on dotnet core")]


Sounds like a defect in the NUnit runner.

I don't know why this test is failing and wasn't being run, but it appears we don't really need it anymore as we are nearly down to no obsolete members on those interfaces and classes, so we can just get rid of the whole test.

jonorossi · 2018-02-15T03:35:18Z

src/Castle.Windsor.Tests/Lifestyle/ScopedLifestyleExplicitAndMultipleThreadsTestCase.cs

@@ -102,6 +103,7 @@ public void Context_is_passed_onto_the_next_thread_TPL()
 				Assert.AreSame(instance, instanceFromOtherThread);
 			}
 		}
+#endif


Could you remove the conditional compilation so the pull request fails and I can see the error, or just paste the exception here.

…his does not hold true for net45. Threads that are eagerly shared should not be asserted against.

ghost · 2018-02-15T20:39:49Z

src/Castle.Windsor.Tests/Lifestyle/ScopedLifestyleExplicitAndMultipleThreadsTestCase.cs

@@ -96,14 +95,12 @@ public void Context_is_passed_onto_the_next_thread_TPL()
 				var initialThreadId = Thread.CurrentThread.ManagedThreadId;
 				var task = Task.Factory.StartNew(() =>
 				{
-					Assert.AreNotEqual(Thread.CurrentThread.ManagedThreadId, initialThreadId);
 					instanceFromOtherThread = Container.Resolve<A>();


I found a problem on my machine where if I debugged the test it would pass. Running it normally would make it fail. This test does not have proper task error handling so it swallows the real exception. On the off chance I did notice that the managed thread id was the same for both the task and the running test. Looks like the task ran on the same thread. The assert was kicking in and then squelching the real error. If netcoreapp1.0 is optimising TPL to run on the same thread, should we be asserting against it?

@jonorossi ^^

@Fir3pho3nixx I saw this the other day, I did intend to look at this a bit more before getting this merged. It seems strange that it would run on the same thread, if you put some more code before task.Wait() does it still run on the same thread?

Will mess with it a bit more and report back. Curiously, notice how when I added a9f5f76 which had nothing to do with this test it passed again? I expect this is a flakey test that will only fail randomly every now and then.

@jonorossi - This is further evidence of the flappy test which asserts on random integer.

Please see this random failure when I updated markdown docs: https://ci.appveyor.com/project/castleproject/windsor/build/0.0.350#L321

This has got to be the managed thread id assertion! :)

Boom! It passes! https://ci.appveyor.com/project/castleproject/windsor/build/0.0.351#L305

Rubbish assertion, how MS optimise their threading have clearly changed in netcoreapp1.0.

Jesus christ! Another flappy test.

https://ci.appveyor.com/project/castleproject/windsor/build/0.0.352#L736

Passing again?

https://ci.appveyor.com/project/castleproject/windsor/build/0.0.353#L735

Yep, let's get rid of the assert.

With regards to the WCF test that failed, I've noticed in the past if the build machine is heavily loaded you can sometimes get failing tests like that, yep flakey.

…this explicit test case to be ignored for now

ghost · 2018-02-19T20:19:45Z

src/Castle.Windsor.Tests/TestConventionsValidationTestCase.cs

@@ -20,7 +20,7 @@ namespace CastleTests

 	using NUnit.Framework;

-	[Explicit]
+	[Ignore("Used to be explicit but fails on netcoreapp1.0 when run with VS2017 Resharper")]
 	public class TestConventionsValidationTestCase : AbstractContainerTestCase
 	{
 		[Test]


I ignored this because it was the only other test I found that failed only when running it VS 2017. Was explicit anyway?

Fair enough.

ghost · 2018-02-28T20:59:23Z

@jonorossi - I think we are good to merge here.

ghost mentioned this pull request Feb 14, 2018

Use the latest NUnitTestAdapter for running tests (3.8.0-alpha1) #243

Closed

ghost commented Feb 14, 2018

View reviewed changes

Upgrading NUnit as per: #243

dc120c6

Looks like we are in a good place, dotnet test is a first class citizen.

jonorossi requested changes Feb 15, 2018

View reviewed changes

Gavin van der Merwe added 2 commits February 15, 2018 19:43

Removing test fixture that is no longer needed

a9f5f76

Turns out that managed threads are eagerly shared on netcoreapp1.0, t…

43f6b8a

…his does not hold true for net45. Threads that are eagerly shared should not be asserted against.

ghost commented Feb 15, 2018

View reviewed changes

Because of the problem with nunit explicit attributes, I am changing …

2fa3ed9

…this explicit test case to be ignored for now

ghost commented Feb 19, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into upgrading-nunit

68c9016

jonorossi merged commit aba6f8e into castleproject:master Mar 1, 2018

ghost deleted the upgrading-nunit branch June 22, 2018 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrading NUnit and now fully using NUnitAdapter #381

Upgrading NUnit and now fully using NUnitAdapter #381

ghost commented Feb 14, 2018

ghost Feb 14, 2018

jonorossi Feb 15, 2018

ghost Feb 14, 2018 •

edited by ghost

Loading

jonorossi Feb 15, 2018

ghost Feb 15, 2018

jonorossi Feb 15, 2018

jonorossi Feb 15, 2018

ghost Feb 15, 2018

ghost Feb 19, 2018

jonorossi Feb 20, 2018

ghost Feb 20, 2018

ghost Feb 28, 2018

ghost Feb 28, 2018

ghost Feb 28, 2018

ghost Feb 28, 2018

ghost Feb 28, 2018 •

edited by ghost

Loading

jonorossi Mar 1, 2018

ghost Feb 19, 2018 •

edited by ghost

Loading

jonorossi Feb 20, 2018

ghost commented Feb 28, 2018

Upgrading NUnit and now fully using NUnitAdapter #381

Upgrading NUnit and now fully using NUnitAdapter #381

Conversation

ghost commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 14, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 19, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 28, 2018

ghost Feb 14, 2018 •

edited by ghost

Loading

ghost Feb 28, 2018 •

edited by ghost

Loading

ghost Feb 19, 2018 •

edited by ghost

Loading