Skip to content
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

TCK Improvement: Throw meaningful error in white box subscriber verification when Probe.registerOnSubscribe isn't called #416

Closed
jroper opened this Issue Nov 29, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@jroper
Copy link
Contributor

jroper commented Nov 29, 2017

Currently, none of the the SubscriberWhiteboxVerification tests check whether WhiteboxSubscriberProbe.registerOnSubscribe is invoked before it starts trying to use the puppet supplied by that invocation. So, if it wasn't invoked, it just throws an NPE:

java.lang.NullPointerException
	at org.reactivestreams.tck.SubscriberWhiteboxVerification$1.run(SubscriberWhiteboxVerification.java:83)
	at org.reactivestreams.tck.SubscriberWhiteboxVerification.subscriberTest(SubscriberWhiteboxVerification.java:497)
	at org.reactivestreams.tck.SubscriberWhiteboxVerification.required_exerciseWhiteboxHappyPath(SubscriberWhiteboxVerification.java:80)

It would be nice if this was checked, and a meaningful error message (saying that registerOnSubscribe hasn't been invoked) was thrown.

@ktoso

This comment has been minimized.

Copy link
Contributor

ktoso commented Nov 29, 2017

Sounds like a good improvement, are you giving it a shot or should I try to next week (won't be able this week)?

@jroper

This comment has been minimized.

Copy link
Contributor Author

jroper commented Nov 29, 2017

I'll take a look today.

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Nov 29, 2017

@jroper If the promise hasn't been completed at the time of trying to use it, it should yield a test failure:

public T value() {
  if (isCompleted()) {
    return _value;
  } else {
    env.flop("Cannot access promise value before completion");
    return null;
  }
}

Perhaps that failure is hidden by an NPE? In which case, we might want to both flop AND throw?

@viktorklang

This comment has been minimized.

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Nov 30, 2017

@jroper I'm planning on issuing the 1.0.2-RC1 within 24h and would love to include this improvement as well. Will you have time to take a stab at it, or can provide a "failing test" someone could use to verify if it gets fixed? Thank you!

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Dec 1, 2017

@jroper 6h to go

@viktorklang viktorklang closed this Dec 1, 2017

@viktorklang viktorklang reopened this Dec 1, 2017

@akarnokd

This comment has been minimized.

Copy link
Contributor

akarnokd commented Dec 4, 2017

@viktorklang Do you want the fix for this included in RC1? If so, I can have a look and post a fix today.

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Dec 4, 2017

@akarnokd That'd be very nice. If the fix is small then it should be quick to inorporate (in time for RC1)

@akarnokd

This comment has been minimized.

Copy link
Contributor

akarnokd commented Dec 4, 2017

The OP is not very detailed on how to trigger this failure so I have to figure that out too. Plus, "none of the the SubscriberWhiteboxVerification tests check" indicates all of them is affected and should be tested/modified.

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Dec 4, 2017

Ideally the fix can be applied in one place, but let's see what it turns out to need.

@ktoso

This comment has been minimized.

Copy link
Contributor

ktoso commented Dec 5, 2017

Please assign milestone @viktorklang ?

@viktorklang viktorklang added this to the 1.0.2 milestone Dec 5, 2017

@viktorklang

This comment has been minimized.

Copy link
Contributor

viktorklang commented Dec 5, 2017

@ktoso Done!

viktorklang added a commit that referenced this issue Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.