reactive-streams / reactive-streams-dotnet

Reactive Streams for .NET
MIT No Attribution
197 stars 28 forks source link

RC1 TCK: IdentityProcessorVerification §2.13 with primitive values #29

Closed akarnokd closed 7 years ago

akarnokd commented 7 years ago

If I test an IProcessor<int, int> implementation the test

Reactive.Streams.TCK.SubscriberWhiteboxVerification`1.<Required_spec213_onNext_mustThrowNullPointerExceptionWhenParametersAreNull

Fails because int is non-nullable and the test possibly calls OnNext(0) that is a legal value. The same impementation over IProcessor<object, object> passes this particular test.

marcpiechura commented 7 years ago

not sure I understand the problem, this test can't pass with int as type parameter since it can't be null, it will work if you use int? instead.

Or is your issue that this test is executed for a value type at all?

https://github.com/reactive-streams/reactive-streams-dotnet/blob/master/src/tck/Reactive.Streams.TCK/SubscriberWhiteboxVerification.cs#L338

akarnokd commented 7 years ago

IMO the test shouldn't run for non-nullable types such as int, i.e.,

if (typeof(T).IsClass) {
   // run the test
}
marcpiechura commented 7 years ago

But that could lead to implementations where this behavior isn't verified at all. For example, I had the same problem while implementing the SubsciberVerification tests in Akka.Streams.TCK.Tests and all of them use int?, if this spec hasn't failed at the beginning I would have used int in all of them too and alle would have skipped this verification.

Also what is the behavior on the JVM? The implementation calls onNext with null but how is that possible if int can't be null?

akarnokd commented 7 years ago

The JVM doesn't have structs thus every type is a class (or interface) with a nullable reference. I found this that might help.

marcpiechura commented 7 years ago

I see, so if we change it, we could simply store the result of default(t) in a variable and check if it is null, i.e.

var element = default(T);
if (element == null)
    OnNext(element);
else
    Ignore();

But I don't want to decide which option we take so I leave this for @viktorklang and @ktoso ;-)

marcpiechura commented 7 years ago

A third option would be to throw by default, including a more meaningful message and add a option to the TestEnvironment which can be used to skip those specs for value types.

viktorklang commented 7 years ago

Not a .NET expert here, but I think it's fine to skip testing null for types that cannot possibly be inhabited by null. Ping @ktoso

ktoso commented 7 years ago

Yeah I guess so.