Enable .NET Standard 2.0 tests on non-Windows platforms #2609

jnm2 · 2017-12-11T15:10:09Z

Commit 2 fixes #2542
Commit 4 fixes #2608
Commit 5 fixes #2555

ChrisMaddock

#2542 is not nice for .NET Standard. 😞

The first four commits look great. 😄

The warnings I'm not totally happy with, as I feel like we won't ever see them. How often do we actually look through the whole CI logs to check for any messages like that? We have the same problem with Ignore("Fix this later") - they just get ignored for ever more.

I'd personally prefer to limit to be pushed up instead - I think all we want to avoid with these tests is any crazy 2 minute runtimes like we used to have. If we want to actually test performance (which I'd love to, one day... :-p), then this isn't the right way to do that.

The constraint looks nice though. 😄

ChrisMaddock · 2017-12-11T21:50:20Z

One more - is there a reason you only did this for .NET Standard 2.0 and not 1.6 as well? Is there more work required for 1.6? We should definitely do that at some point.

CharliePoole · 2017-12-11T21:52:13Z

General comment on Ignores and Warnings...

IMO, it's a social rather than a technical problem. As a visiting coach, I had to solve the problem many times and it generally involved giving visibility to Ignored tests (we didn't have Warnings till recently, of course). Once the entire team had the warnings displayed in front of them all the time social pressure led to their being fixed. No special technology was involved. Management wasn't involved.

The thing is, I only know how to do this in collocated teams I'm coaching. As an older guy, the whole "internet presence" thing sounds like an oxymoron to me. 😄 We did it by hanging reports with the failures and ignored tests featured on the wall. Is there anything you feel would be paid attention to by this group, separated as we are in time (zone) and space?

jnm2 · 2017-12-11T22:00:00Z

@ChrisMaddock

There is more work required to compile our .NET Core 1.1 tests in order to test .NET Standard 1.6 and I'm aiming for smaller PRs 😎 #2611

The other thing to consider with warnings is that once build.cake uses a newer console runner, we will start seeing warnings and ignores every single time we run build.cake even when there are no errors. (We should make sure NUnitLite does that too.)

jnm2 · 2017-12-11T22:02:26Z

@CharliePoole we could have build status indicators on our GitHub landing page that make us look bad when there are warnings... 😁

CharliePoole · 2017-12-11T22:08:48Z

@jnm2 I for one never see the status indicators. I have to scroll down two pages to see them. But maybe others check them.

jnm2 · 2017-12-11T22:48:21Z

The other thing with a performance test is that it's incredibly sensitive to hardware and to parallelism, so it's not like there's anything sacred about the duration 100ms. We'd need a dedicated server and no parallelism for the actual number to be meaningful.

I'm not sure if that builds the case for warnings or padding the limit. I have no real opinion but I do want to get this through, so let me know what's agreeable to you in the short term.

rprouse

Looks good except for some minor nits. As for the MaxTime vs Warnings, I am on the fence. I agree with @ChrisMaddock that warnings will go unnoticed, but I also agree with @jnm2 that using timeouts for performance tests aren't really valid tests because they are so hardware specific. Down to warnings, if we don't believe that they are useful because they will never be seen, then why did we add them in the first place 😄

I'm not a huge fan of warnings, but if we believe they are a useful feature, we should probably use them occasionally ourselves, so I am okay with these two occurrences, but I wouldn't want to see them used too widely in the future.

My nits are very minor, so I am going to approve. If you decide to address the nits, I will re-approve 😄 It would be nice if GitHub had an option to only dismiss negative approvals for situations like this.

rprouse · 2017-12-12T00:43:35Z

BUILDING.md

@@ -74,6 +74,7 @@ Feature constants are defined in [Common.props](src/NUnitFramework/Common.props)
 - `PARALLEL` enables running tests in parallel
 - `PLATFORM_DETECTION` enables platform detection
 - `THREAD_ABORT` enables timeouts and forcible cancellation
+ - `COM_APARTMENT` enables control of the thread apartment state


Minor nit, but would something like APARTMENT_STATE or THREAD_APARTMENT be clearer? Many new developers probably don't know what COM is 😋

That's fine. I changed it from APARTMENT_STATE just to be didactic about the naming and the shorter constant grew on me but I'll switch back for clarity.

rprouse · 2017-12-12T00:46:33Z

src/NUnitFramework/framework/Internal/Execution/WorkItemQueue.cs

@@ -101,17 +101,28 @@ public SavedState(WorkItemQueue queue)
        private int _addId = int.MinValue;
        private int _removeId = int.MinValue;

+#if COM_APARTMENT


This could be moved to line 109/110 to prevent the duplicate documentation.

The weird thing is that you can't put preprocessor inside XML documentation. I thought it was simple but this comment blew my mind.

rprouse · 2017-12-12T00:48:37Z

src/NUnitFramework/tests/Assertions/CollectionAssertTest.cs


-        #region IsSubsetOf
+#region IsSubsetOf


I really wish Visual Studio would fix this formatting issue. Makes me want to do a mass unindent checkin.

I would love it if you did. That, and an endianness fix, and a trailing whitespace fix. Or just say it's okay for me to. 😀

jnm2 · 2017-12-12T01:10:42Z

What does everyone think of excluding [Category("Performance")] for Travis and AppVeyor?

jnm2 · 2017-12-12T01:28:35Z

@rprouse Need new review because of the APARTMENT_STATE commit. 😀

rprouse · 2017-12-12T01:32:41Z

LGTM, waiting on CI. As for the category, would you propose not running that category of tests? I think I prefer the warning.

jnm2 · 2017-12-12T01:36:50Z

@rprouse I would propose not running performance tests on CI, yes. Since I'm thinking the data they generate is not very meaningful and they slow everything down a bit. But if you prefer warnings I'm happy with that so long as this PR is unblocked. Anyone can follow up later.

rprouse · 2017-12-12T02:19:31Z

Another .NET 3.5 failure

1) Failed : NUnit.Framework.Internal.Execution.EventQueueTests+SetWaitHandle_Enqueue_AsynchronousTest.SetWaitHandle_Enqueue_Asynchronous
  Expected: null
  But was:  <NUnit.Framework.AssertionException:   Expected: True
  But was:  False
   at NUnit.Framework.Assert.ReportFailure(String message)
   at NUnit.Framework.Assert.That[TActual](TActual actual, IResolveConstraint expression, String message, Object[] args)
   at NUnit.Framework.Internal.Execution.EventQueueTests.SetWaitHandle_Enqueue_AsynchronousTest.Consumer() in C:\projects\nunit\src\NUnitFramework\tests\Internal\EventQueueTests.cs:line 196
   at NUnit.Framework.Internal.Execution.EventQueueTests.ProducerConsumerTest.ConsumerThreadWrapper() in C:\projects\nunit\src\NUnitFramework\tests\Internal\EventQueueTests.cs:line 316>
   at NUnit.Framework.Internal.Execution.EventQueueTests.ProducerConsumerTest.RunProducerConsumer() in C:\projects\nunit\src\NUnitFramework\tests\Internal\EventQueueTests.cs:line 304
   at NUnit.Framework.Internal.Execution.EventQueueTests.SetWaitHandle_Enqueue_AsynchronousTest.SetWaitHandle_Enqueue_Asynchronous() in C:\projects\nunit\src\NUnitFramework\tests\Internal\EventQueueTests.cs:line 181

jnm2 · 2017-12-12T14:34:34Z

A new perf-sensitive failure to keep an eye on, Travis/netstandard2.0:

3) Failed : NUnit.Framework.Internal.Execution.TestWorkerTests.BusyExecuteIdleEventsCalledInSequence
  Expected: "BusyExecIdle" after 200 milliseconds delay
  But was:  "BusyExec"

CharliePoole · 2017-12-12T18:07:40Z

<off-topic>
Warnings can be very useful to a team, if the team knows how to use them. It's not a technical problem but a social one. If you have warnings, then you have to have something in place to make sure they get attention.

I can assure you that my teams have always made effective use of warnings, because we set up non-technical team structures to deal with them. I'm sure that plenty of teams are using our NUnit warnings in that way, even if we are not able to because we are distributed and haven't yet invented a social mechanism to make those who create warnings want to get rid of them.

I don't think that says anything real bad about us as a team. I do not believe the necessary social connections for proper functioning of a distributed team have been invented yet. Every few years I review what's out there and it's all still very primitive as compared to the standard organizational patterns like "Expert in Earshot" and "Big Visible Displays".
</off-topic>

mitchellmaler · 2017-12-12T18:58:59Z

Quick Question: With the new dotnet test command using the adapter do you know if it is still possible to pass arguments to nunit? The argument I am looking to pass is the --workers parameter so I can limit the amount of parallel tests that run at a given time.

rprouse · 2017-12-13T01:11:53Z

@mitchellmaler, dotnet test only passes arguments on to VSTest, not through to our adapter, so we are out of luck there. xUnit solved the problem by writing their own dotnet xunit command, giving them full control over the test process. We are considering doing the same with a dotnet nunit command, but we haven't started work on it.

mitchellmaler · 2017-12-13T22:46:46Z

Okay that makes sense. I will just use the NumberOfTestWorkers .runsettings property for now and use cli to change that if needed.

jnm2 added 5 commits December 9, 2017 23:40

Enable netstandard2.0 test on non-Windows

1e62cee

Put apartment state logic behind COM_APARTMENT

703fe20

Added COM_APARTMENT to BUILDING.md

e41e64a

Respect platform-specific culture and comparison behavior

192fe0a

Convert performance tests to warnings

5c45bef

This was referenced Dec 11, 2017

NUnit does not support parallelism on .NET Core 2.0 #2542

Closed

Enable .NET Standard 1.6 tests on non-Windows #2611

Closed

CI timeout: NUnit.Framework.Assertions.CollectionAssertTest.PerformanceTests #2555

Closed

jnm2 added the pri:critical Use this to indicate a hotfix may be necessary label Dec 11, 2017

ChrisMaddock reviewed Dec 11, 2017

View reviewed changes

rprouse previously approved these changes Dec 12, 2017

View reviewed changes

Rename COM_APARTMENT to APARTMENT_STATE

8a96dd2

jnm2 dismissed rprouse’s stale review via 8a96dd2 December 12, 2017 01:28

rprouse approved these changes Dec 12, 2017

View reviewed changes

jnm2 merged commit 38e61b6 into master Dec 12, 2017

jnm2 deleted the jnm2/enable_tests_netstandard2.0 branch December 12, 2017 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable .NET Standard 2.0 tests on non-Windows platforms #2609

Enable .NET Standard 2.0 tests on non-Windows platforms #2609

jnm2 commented Dec 11, 2017 •

edited

ChrisMaddock left a comment •

edited

ChrisMaddock commented Dec 11, 2017

CharliePoole commented Dec 11, 2017

jnm2 commented Dec 11, 2017

jnm2 commented Dec 11, 2017

CharliePoole commented Dec 11, 2017

jnm2 commented Dec 11, 2017

rprouse left a comment

rprouse Dec 12, 2017

jnm2 Dec 12, 2017

rprouse Dec 12, 2017

jnm2 Dec 12, 2017 •

edited

rprouse Dec 12, 2017

jnm2 Dec 12, 2017

jnm2 commented Dec 12, 2017

jnm2 commented Dec 12, 2017

rprouse commented Dec 12, 2017

jnm2 commented Dec 12, 2017

rprouse commented Dec 12, 2017

jnm2 commented Dec 12, 2017

CharliePoole commented Dec 12, 2017

mitchellmaler commented Dec 12, 2017

rprouse commented Dec 13, 2017

mitchellmaler commented Dec 13, 2017

Enable .NET Standard 2.0 tests on non-Windows platforms #2609

Enable .NET Standard 2.0 tests on non-Windows platforms #2609

Conversation

jnm2 commented Dec 11, 2017 • edited

ChrisMaddock left a comment • edited

Choose a reason for hiding this comment

ChrisMaddock commented Dec 11, 2017

CharliePoole commented Dec 11, 2017

jnm2 commented Dec 11, 2017

jnm2 commented Dec 11, 2017

CharliePoole commented Dec 11, 2017

jnm2 commented Dec 11, 2017

rprouse left a comment

Choose a reason for hiding this comment

rprouse Dec 12, 2017

Choose a reason for hiding this comment

jnm2 Dec 12, 2017

Choose a reason for hiding this comment

rprouse Dec 12, 2017

Choose a reason for hiding this comment

jnm2 Dec 12, 2017 • edited

Choose a reason for hiding this comment

rprouse Dec 12, 2017

Choose a reason for hiding this comment

jnm2 Dec 12, 2017

Choose a reason for hiding this comment

jnm2 commented Dec 12, 2017

jnm2 commented Dec 12, 2017

rprouse commented Dec 12, 2017

jnm2 commented Dec 12, 2017

rprouse commented Dec 12, 2017

jnm2 commented Dec 12, 2017

CharliePoole commented Dec 12, 2017

mitchellmaler commented Dec 12, 2017

rprouse commented Dec 13, 2017

mitchellmaler commented Dec 13, 2017

jnm2 commented Dec 11, 2017 •

edited

ChrisMaddock left a comment •

edited

jnm2 Dec 12, 2017 •

edited