10BC0 Upgrade to NUnit 3.12, Add NetCore 3.0 variant by akarnokd · Pull Request #46 · reactive-streams/reactive-streams-dotnet · GitHub
[go: up one dir, main page]
Skip to content

Upgrade to NUnit 3.12, Add NetCore 3.0 variant #46

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Upgrade to NUnit 3.12, Add NetCore 3.0 variant #46

wants to merge 3 commits into from

Conversation

akarnokd
Copy link
Contributor

Looks like there was a breaking change between NUnit 3.6 and 3.12 (latest as of now) so my project used to verify the TCK stopped running unit tests alltogheter. This PR also adds NETCoreApp3.0 as an output framework. I don't know how that will end up in the NuGet release though.

For one, upgrading to NUnit 3.12 dependency explicitly should resolve the problem. Unfortunately, when this is done, the Reactive.Streams.TCK.Tests start to fail with awkward errors on a known to be correct implementation of RangePublisherTest:

Required_spec309_requestNegativeNumberMustSignalIllegalArgumentException
  No source available
   Duration: 584 ms

  Message: 
    Unexpected Subscriber.OnError(System.ArgumentException: §3.9 violated)
  Stack Trace: 
    TestEnvironment.Flop(Exception exception, String message) line 174
    TestSubscriber`1.OnError(Exception cause) line 612
    ManualSubscriberWithSubscriptionSupport`1.OnError(Exception cause) line 560
    RangeSubscription.Request(Int64 n) line 123
    ManualSubscriber`1.Request(Int64 elements) line 370
    PublisherVerification`1.<Required_spec309_requestNegativeNumberMustSignalIllegalArgumentException>b__56_0(IPublisher`1 publisher) line 914
    PublisherVerification`1.ActivePublisherTest(Int64 elements, Boolean completionSignalRequired, Action`1 run) line 1125
    PublisherVerification`1.Required_spec309_requestNegativeNumberMustSignalIllegalArgumentException() line 910

EmptyLazyPublisherTest:

 Optional_spec105_emptyStreamMustTerminateBySignallingOnComplete
  No source available
   Duration: 1 sec

  Message: 
    Multiple failures or warnings in test:
      1) Subscriber.OnComplete() called before Subscriber.OnSubscribe()
      2) Did not receive expected stream completion within 500 ms
    
  Stack Trace: 
    TestEnvironment.Flop(String message) line 150
    ManualSubscriberWithSubscriptionSupport`1.OnComplete() line 544
    SubscriptionImplementation.DoSubscribe() line 212
    <.ctor>b__9_0() line 131
    Task.InnerInvoke()
    <.cctor>b__274_0(Object obj)
    TestEnvironment.Flop(String message) line 150
    Receptacle`1.ExpectCompletion(Int64 timeoutMilliseconds, String errorMessage) line 878
    ManualSubscriber`1.ExpectCompletion(Int64 timeoutMilliseconds, String errorMessage) line 486
    ManualSubscriber`1.ExpectCompletion(Int64 timeoutMilliseconds) line 480
    ManualSubscriber`1.ExpectCompletion() line 477
    PublisherVerification`1.<Optional_spec105_emptyStreamMustTerminateBySignallingOnComplete>b__30_0(IPublisher`1 publisher) line 414
    PublisherVerification`1.PotentiallyPendingTest(IPublisher`1 publisher, Action`1 run, String message) line 1180
    PublisherVerification`1.PotentiallyPendingTest(IPublisher`1 publisher, Action`1 run) line 1175
    PublisherVerification`1.OptionalActivePublisherTest(Int64 elements, Boolean completionSignalRequired, Action`1 run) line 1149
    PublisherVerification`1.Optional_spec105_emptyStreamMustTerminateBySignallingOnComplete() line 410

@akarnokd akarnokd mentioned this pull request Nov 15, 2019
Closed
@akarnokd akarnokd mentioned this pull request Nov 15, 2019
Open
@akarnokd
Copy link
Contributor Author

I've fixed the problems due to now incorrect NUnit Assert usages: these assert failures are tracked beyond them throwing so even if one catches an Assert.Fail and suppresses it, the test runner will still consider the whole test failed.

Note also that there is a slight anomaly regarding .NET Core 3 testing on machines with 4 or less cpu threads. If there are more tasks ready to run and some block the standard threadpool, it takes 500 milliseconds for the pool to grow, causing timeout at random in some TCK tests (which default to 500ms timeout themselves).

@viktorklang
Copy link
Contributor

@marcpiechura @cconstantin Thoughts or feedback here?

@Aaronontheweb
Copy link

@viktorklang @akarnokd this looks good to me, but I'd also be happy migrating to XUnit since that seems to be the lingua franca these days

@marcpiechura
Copy link
Contributor

@viktorklang @akarnokd this looks good to me, but I'd also be happy migrating to XUnit since that seems to be the lingua franca these day

Back in the days XUnit wasn’t able to skip tests based on a condition during the actual run, therefore I’ve chosen nunit. So before you start a big rewrite, I would check if xunit supports those cases now, or you’ll have to implement a different strategy ;-)

Some examples are

reactive-streams-dotnet/src/tck/Reactive.Streams.TCK/SubscriberWhiteboxVerification.cs

Line 458 in f1a95c6

public void NotVerified(string message) => Assert.Ignore(message);

And

reactive-streams-dotnet/src/tck/Reactive.Streams.TCK/SubscriberWhiteboxVerification.cs

Line 334 in f1a95c6

throw new IgnoreException("Can't verify behavior for value types");

@akarnokd
Copy link
Contributor Author

NUnit works now so I'd leave any major framework change to a later version.

@Aaronontheweb
Copy link

@marcpiechura did not know that!

@Aaronontheweb Aaronontheweb mentioned this pull request Aug 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akarnokd @viktorklang @Aaronontheweb @marcpiechura
0