Ported TCK and examples

[marcpiechura]

All specs are passing, but I would like to update this PR with the changes introduced in #7 after it was merged.

Some notes about the implementation.

• In .Net we don't have a equivalent of the ExecutorService afaik, therefore I have removed the parameter and simply call Task.Run directly.
• We can't create sub classes "on the fly" with new Subscriber() {...}, so if needed I created a private class and for the most common ones I created helper classes (Subscriber, Subscription, Publisher)
• Due to the way NUnit is working ~500 specs are marked as Skiped, the reason is that I needed to create sub classes of the verification in some verification tests and although the class is private NUnit tries to run these specs as well. For example IdentityProcessorVerificationTest.cs

@ktoso would appreciate if you also could take a look.

[ktoso]

Looks awesome,I'll nervier tomorrow :)

[ktoso]

PS. Could I run this using mono in theory?

[cconstantin]

@ktoso in theory yes.

[viktorklang]

In .Net we don't have a equivalent of the ExecutorService afaik, therefore I have removed the parameter and simply call Task.Run directly.

As long as Task.Run executes the task concurrently that should be OK.
OTOH, don't you have things like this: https://msdn.microsoft.com/en-us/library/system.threading.threadpool(v=vs.110).aspx ?

We can't create sub classes "on the fly" with new Subscriber() {...}, so if needed I created a private class and for the most common ones I created helper classes (Subscriber, Subscription, Publisher)

Ah, ok. Good call.

Due to the way NUnit is working ~500 specs are marked as Skiped, the reason is that I needed to create sub classes of the verification in some verification tests and although the class is private NUnit tries to run these specs as well. For example IdentityProcessorVerificationTest.cs

Ouch, won't that create quite a bit of maintenance burden if/when the -jvm TCK gets updates?

[marcpiechura]

As long as Task.Run executes the task concurrently that should be OK.
OTOH, don't you have things like this: https://msdn.microsoft.com/en-us/library/system.threading.threadpool(v=vs.110).aspx ?

Yes it executes the task concurrently, it's the equivalent of scalas Future. The ThreadPool class is static and only gives you access to the managed thread pool from .Net but it's not usable as parameter if you want to provide the ability for custom implementation. Aaron is currently working on some changes in the internal Akka.Net mailboxes and want's to introduce a IRunnable interface and new dispatcher which would be close to java's Executor, so maybe we could adapt that in the future if necessary. My concern here is more about the extensibility from a users perspective, because as I understood it the parameter gives the user the ability to provide custom executors.

Ouch, won't that create quite a bit of maintenance burden if/when the -jvm TCK gets updates?

Do you mean when new specs are added?
Luckily we can skip the complete class:

[TestFixture(Ignore = "Helper verification for single test")]
private sealed class Spec104WaitingVerification : IdentityProcessorVerification<int>

So if in the feature another spec is added to the IdentityProcessorVerification it's automatically skipped in that specific implementation.

[marcpiechura]

I updated the PR with the changes from #7, I also modified the build script and added a placeholder to the RELEASE_NOTES.md because otherwise the script wouldn't work.

[cconstantin]

@Silv3rcircl3 Viktor's preference it to only add an entry to release notes with a PR for the release. I think we can do that and not worry about the build just yet.

[cconstantin]

@Silv3rcircl3 the tck needs to be added to the build, so we can build and publish the package. In that context, a placeholder in RELEASE_NOTES.md is needed so we can test the build.
Also, there's a readme in the tck folder in the JVM repo, so maybe we should also create mimic the folder structure, with api, tck, and examples folders.

[marcpiechura]

@cconstantin alright, I remove the entry
Ah yeah I forgot the Nuget part, will address your remarks this weekend.

[cconstantin]

@Silv3rcircl3 PublisherVerificationTest.required_spec109_mustIssueOnSubscribeForNonNullSubscriber_mustFailIfOnErrorHappensFirst needs to be enabled, and then we'll match the 103 tests enabled on the jvm side.

[cconstantin]

@ktoso @viktorklang this is good to go now.

[viktorklang]

@cconstantin Sorry for the delay.

I can merge this as-is because my C# skills is not enough for reviewing it.

What would be helpful to me is answering the following questions:

1. are the examples validated using the TCK?
2. have you applied the TCK to your own Reactive Streams implementations?
3. what has the strategy for migrating the TCK been and where, if at all, does it deviate from the original?

Thanks!
√

[marcpiechura]

@viktorklang

1. Yes and all Unicast specs are passing

2. Not yet, but it's definitely on my todo list, but can't say when it's done

3. I hadn't had any specific strategy in mind when I started the port, but my goal was to stay as close as possible to the jvm version. So in 95% of the time it really is a line by line port, out of my head these are the biggest differences to the original implementation:

   • Replaced Iterable<T> with IEnumerable<T>, they have the same purpose but a slightly different API
   • Removed the Executor
   • Replaced all "on the fly" created classes that override a base class with private classes in the same scope, of course with the same implementation, for example:

   @Override public Subscriber<Integer> createSubscriber() {
     return new AsyncSubscriber<Integer>(e) {
       @Override protected boolean whenNext(final Integer element) {
         return true;
       }
     };
   }

   becomes

   public override ISubscriber<int?> CreateSubscriber() => new Suscriber();
   
   private sealed class Suscriber : AsyncSubscriber<int?>
   {
        protected override bool WhenNext(int? element) => true;
   } 

   • Used int? instead of int in the examples, reason is that while Integer can be null int can't, therefore I used the nullable int? to allow specs that checks Next(null) to pass
   • Used Action or Action<T> for the abstract *TestRun classes.

   abstract class TestStageTestRun {
     public abstract void run(WhiteboxTestStage stage) throws Throwable;
   }

Hope that helps.

[marcpiechura]

@viktorklang That was easier than I thought, I have applied the TCK to our implementation and all specs are passing except IdentifierProcessorVerification.MustImmediatelyPassOnOnErrorEventsReceivedFromItsUpstreamToItsDownstream .
So I think we should wait before we merge this PR until I found out if it's an issue with the TCK or our implementation.

[marcpiechura]

@viktorklang fixed the last issue, now all specs are passing on our implementation too.

[viktorklang]

@Silv3rcircl3 Thanks for answering my pesky questions!

[viktorklang]

@Silv3rcircl3 Can you please also add yourself to the https://github.com/reactive-streams/reactive-streams-dotnet/blob/master/CopyrightWaivers.txt in this PR? (As per CONTRIBUTING.MD)

[marcpiechura]

@viktorklang you're welcome.
Signed the copyright statement.

[viktorklang]

@Silv3rcircl3 Thanks!

@cconstantin From your PoV, everything in here OK to merge?

[cconstantin]

@viktorklang yes, good to go. Should we update the changelog to describe 1.0.0, or you'd rather do that with a separate PR?

[viktorklang]

@cconstantin The first release when "everything is ready" needs to be 1.0.0-RC1 so people have a chance to give feedback on the TCK etc before 1.0.0 is released.

[cconstantin]

Sounds good. Should we include that in this PR, or separate?

[viktorklang]

@cconstantin I, personally, prefer if releases are made as first Issue, then PR, so people can discuss the release and the PR separately.

[cconstantin]

Sounds good @viktorklang . Then let's merge this and we'll open the issue. Will take a look at the jvm repo as a template for the issue.

[ktoso]

good! I actually forgot to set this in the JVM TCK it seems (which still checks the right thing, but not strictly following what this test was intednded to do). Thanks, I'll fix the JVM test

[marcpiechura]

Interesting :) I think I haven't noticed that it's not there in the jvm, because if so I have had said something.

[ktoso]

👍 good job with the TCK, I'll go over the rest, maybe I'll notice something, even though I'm not C# fluent logic is pretty similar so not a big issue :)

[ktoso]

I reviewed most of the PR, looks very good!
I may have missed some detail, but in general very solid port–good job @Silv3rcircl3!

LGTM from my side (no merge rights though).

[cconstantin]

@viktorklang this is good to go now, reviewed by both @ktoso and me.

[viktorklang]

Good work, gents!