+tck #181 provides default helperPublisher for subscriber tests #186

ktoso · 2014-12-24T14:24:52Z

This is a rather heavy PR, however it removes the need of implementing a (helper) Publisher when I'm only interested in implementing a Subscriber. This is done by providing a default implementation - based on the example implementations (reused).

The TCK automatically picks the default publisher if it is able to. If not (types don't match), it fails ASAP and tells the implementer that he/she has to implement a custom publisher (like we, until now, always forced implementers to).

Let me know what you think!

+ When subscriber verifications are extended using Integer
  we can use the existing publisher implementations from examples
  to drive the tests. This allows implementers who only care about
  implementing Subscribers to not care at all about implementing
  Publishers
+ In case the subscriber verification is extended using a custom signal
  type (defined as "not Integer") we do not magically generate a
  Publisher of this type, but instead fail the tests telling the
  implementer that he/she will need to implement a custom helperPublisher
  which is exactly what we have up until now always demanded from
  implementations.
+ We also provide tests to verify the test failures work as expected,
  and that the messages are indeed helpful.
+ Updated TCK README.md to clarify when to implement a custom publisher
+ fixed mistakenly added `<T>` on NumberPublisher - it is bound to Ints
+ TCK now depends on examples, as it uses the well documented async
  publisher to provide the default publisher for numbers. As well as
  in order to allow implementers to reuse those publishers when
  implementing custom Publishers to drive their Subscriber specs.

Refs #181
Merry 🎄 ;-)

ktoso · 2014-12-24T14:25:54Z

examples/src/main/java/org/reactivestreams/example/unicast/AsyncIterablePublisher.java

@@ -17,7 +17,7 @@

 * NOTE: The code below uses a lot of try-catches to show the reader where exceptions can be expected, and where they are forbidden.
 */
-class AsyncIterablePublisher<T> implements Publisher<T> {
+public class AsyncIterablePublisher<T> implements Publisher<T> {


those need to be public as we want to reuse them. // cc @viktorklang - I think you just missed making the public by accident? Or was it intentional?

Scala has spoiled me. Good catch

didn't want to say so directly :-) default public for the win! :-)

viktorklang · 2014-12-24T19:02:23Z

@ktoso I broke out the touchups to this: #187

viktorklang · 2014-12-24T19:03:52Z

@ktoso rebase on top of current master and you'll get the fixes in the PR above.

viktorklang · 2014-12-27T14:36:03Z

@ktoso Let me know when there's anything new to review. Thanks!

ktoso · 2014-12-27T14:37:01Z

Was planning yesterday but it made some tests fail in weird ways. Maybe
today then. I'll ping soon :-)

Konrad
(sent from mobile)
On 27 Dec 2014 15:36, "Viktor Klang (√)" [email protected] wrote:

@ktoso https://github.com/ktoso Let me know when there's anything new
to review. Thanks!

—
Reply to this email directly or view it on GitHub
#186 (comment)
.

ktoso · 2014-12-27T17:28:00Z

Hey @viktorklang so you can have a look at ktoso@c3dd8a1 while it's currently failing some tests and I didn't yet investigate why (perhaps making the helper async broke assumptions in those tests - pretty probable). I'm still around tomorrow but then off for a week of skiing so don't expect to be able to code much.. If you like the idea you can use these commits as a basis, or we'll continue when I'm back I guess

ktoso · 2014-12-29T18:32:25Z

@viktorklang / @reactive-streams/contributors pushed last commit which fixes our relying on the helper publisher to exhibit a certain behaviour (in terms on when to onComplete). In this specific test we should instead drive the publisher by ourselfes. The whitebox versions of these tests are OK. This change both makes sense and unbreaks the build ;-)

Let me know once you review so I can squash the changes (note: I have random internet access this week).

viktorklang · 2014-12-30T21:30:30Z

tck/src/main/java/org/reactivestreams/tck/SubscriberBlackboxVerification.java

+              private AtomicBoolean completed = new AtomicBoolean();
+
+              @Override public void request(long n) {
+                if (completed.compareAndSet(false, true)) {


no need for atomic, request is only allowed to be called from the subscriber anyway?

viktorklang · 2014-12-30T21:41:48Z

Aside from comments, LGTM!

viktorklang · 2014-12-30T21:42:35Z

@reactive-streams/contributors Please have a look/vote/ask for more time before the 2nd of January 2015, as this should/could go into the RC2, thanks!

…iber tests + When subscriber verifications are extended using Integer we can use the existing publisher implementations from examples to drive the tests. This allows implementers who only care about implementing Subscribers to not care at all about implementing Publishers + In case the subscriber verification is extended using a custom signal type (defined as "not Integer") we do not magically generate a Publisher of this type, but instead fail the tests telling the implementer that he/she will need to implement a custom helperPublisher which is exactly what we have up until now always demanded from implementations. + We also provide tests to verify the test failures work as expected, and that the messages are indeed helpful. + Updated TCK README.md to clarify when to implement a custom publisher + fixed mistakenly added `<T>` on NumberPublisher - it is bound to Ints + TCK now depends on examples, as it uses the well documented async publisher to provide the default publisher for numbers. As well as in order to allow implementers to reuse those publishers when implementing custom Publishers to drive their Subscriber specs. ! Moved away from createHelper to createElement style of testing subscribers

It should not use the helper publisher as it may signal asynchronously as well as onComplete in undefined ways - we need to control the publisher in this case because of the very specific test scenario

ktoso · 2014-12-30T22:37:17Z

I've addressed all comments except the Function one now.
Updated the README.md as well, but please sanity check.

viktorklang · 2015-01-02T18:40:03Z

Superseded by #192 (rebased on top of current master).

ktoso · 2015-01-02T21:01:39Z

rebasing and reopening to verify if validation would/will fail.

ktoso · 2015-01-02T21:01:59Z

Oh... no re-opening powers... :-(

ktoso reviewed Dec 24, 2014
View reviewed changes

ktoso force-pushed the provide-publisher-ktoso branch from 7a6b426 to c3dd8a1 Compare December 27, 2014 17:09

viktorklang mentioned this pull request Dec 28, 2014

Spec. 2. 12 TCK : Testing a unicast publisher #188

Closed

viktorklang modified the milestone: 1.0.0.RC2 Dec 28, 2014

ktoso force-pushed the provide-publisher-ktoso branch from 8f3350d to 4c5361d Compare December 30, 2014 16:13

viktorklang reviewed Dec 30, 2014
View reviewed changes

ktoso force-pushed the provide-publisher-ktoso branch from 4c5361d to 9592f96 Compare December 30, 2014 22:28

ktoso force-pushed the provide-publisher-ktoso branch from 9592f96 to c9ccec4 Compare December 30, 2014 22:30

+tck 2.9 blackbox verification should fully drive publisher

cf715fa

It should not use the helper publisher as it may signal asynchronously as well as onComplete in undefined ways - we need to control the publisher in this case because of the very specific test scenario

ktoso force-pushed the provide-publisher-ktoso branch from c9ccec4 to cf715fa Compare December 30, 2014 22:36

viktorklang mentioned this pull request Jan 2, 2015

Provide publisher ktoso rebased #192

Closed

viktorklang closed this Jan 2, 2015

ktoso mentioned this pull request Jan 2, 2015

+TCK: Provide helper publisher (rebased) #193

Merged

viktorklang reopened this Jan 2, 2015

viktorklang closed this Jan 2, 2015

ktoso deleted the provide-publisher-ktoso branch January 8, 2015 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

+tck #181 provides default helperPublisher for subscriber tests #186

+tck #181 provides default helperPublisher for subscriber tests #186

ktoso commented Dec 24, 2014

ktoso Dec 24, 2014

viktorklang Dec 24, 2014

ktoso Dec 24, 2014

viktorklang commented Dec 24, 2014

viktorklang commented Dec 24, 2014

viktorklang commented Dec 27, 2014

ktoso commented Dec 27, 2014

ktoso commented Dec 27, 2014

ktoso commented Dec 29, 2014

viktorklang Dec 30, 2014

viktorklang commented Dec 30, 2014

viktorklang commented Dec 30, 2014

ktoso commented Dec 30, 2014

viktorklang commented Jan 2, 2015

ktoso commented Jan 2, 2015

ktoso commented Jan 2, 2015

+tck #181 provides default helperPublisher for subscriber tests #186

+tck #181 provides default helperPublisher for subscriber tests #186

Conversation

ktoso commented Dec 24, 2014

ktoso Dec 24, 2014

Choose a reason for hiding this comment

viktorklang Dec 24, 2014

Choose a reason for hiding this comment

ktoso Dec 24, 2014

Choose a reason for hiding this comment

viktorklang commented Dec 24, 2014

viktorklang commented Dec 24, 2014

viktorklang commented Dec 27, 2014

ktoso commented Dec 27, 2014

ktoso commented Dec 27, 2014

ktoso commented Dec 29, 2014

viktorklang Dec 30, 2014

Choose a reason for hiding this comment

viktorklang commented Dec 30, 2014

viktorklang commented Dec 30, 2014

ktoso commented Dec 30, 2014

viktorklang commented Jan 2, 2015

ktoso commented Jan 2, 2015

ktoso commented Jan 2, 2015