Skip to content

RC1 TCK: IdentityProcessorVerification §2.13 with primitive values #29

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.

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

Closed
akarnokd opened this issue Mar 9, 2017 · 8 comments · Fixed by #34
Closed

RC1 TCK: IdentityProcessorVerification §2.13 with primitive values #29

akarnokd opened this issue Mar 9, 2017 · 8 comments · Fixed by #34

Comments

@akarnokd
Copy link
Contributor

akarnokd commented Mar 9, 2017

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
Copy link
Contributor

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
Copy link
Contributor Author

akarnokd commented Mar 9, 2017

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

if (typeof(T).IsClass) {
   // run the test
}

@marcpiechura
Copy link
Contributor

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
Copy link
Contributor Author

akarnokd commented Mar 9, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

ktoso commented Mar 31, 2017

Yeah I guess so.

marcpiechura added a commit to marcpiechura/reactive-streams-dotnet that referenced this issue Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants