+TCK: Provide helper publisher (rebased) #193

ktoso · 2015-01-02T21:20:55Z

This is a rebase of: #186
With the additional commit of making travis failures actually useful - we need --info there, otherwise the build log won't include any stacktraces of errors.

Locally it works fine for me, so need to see what makes it fail in Viktor's rebase-PR over here: #192

…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 · 2015-01-02T21:30:35Z

Yes, we have trouble - here's the failures:

Gradle test > org.reactivestreams.example.unicast.IterablePublisherTest.required_spec317_mustSignalOnErrorWhenPendingAboveLongMaxValue FAILED
    java.lang.AssertionError: Expected onError(java.lang.IllegalStateException) within 100 ms
        Caused by:
        java.lang.AssertionError: Expected onError(java.lang.IllegalStateException) within 100 ms

Gradle test > org.reactivestreams.example.unicast.IterablePublisherTest.stochastic_spec103_mustSignalOnMethodsSequentially FAILED
    java.lang.AssertionError: Async error during test execution: org.reactivestreams.tck.PublisherVerification$25$1@3232bf23 violated the Reactive Streams rule 3.17 by demanding more elements than Long.MAX_VALUE.
        Caused by:
        java.lang.IllegalStateException: org.reactivestreams.tck.PublisherVerification$25$1@3232bf23 violated the Reactive Streams rule 3.17 by demanding more elements than Long.MAX_VALUE.
Gradle test > org.reactivestreams.example.unicast.IterablePublisherTest.untes


Gradle test > org.reactivestreams.example.unicast.UnboundedIntegerIncrementPublisherTest.required_spec306_afterSubscriptionIsCancelledRequestMustBeNops STANDARD_ERROR
    java.lang.IllegalStateException: org.reactivestreams.tck.PublisherVerification$15$2@257b746 violated the Reactive Streams rule 2.13 by throwing an exception from onNext or onComplete.
        at org.reactivestreams.example.unicast.AsyncIterablePublisher$SubscriptionImpl.doSend(AsyncIterablePublisher.java:168)
        at org.reactivestreams.example.unicast.AsyncIterablePublisher$SubscriptionImpl.doRequest(AsyncIterablePublisher.java:88)
        at org.reactivestreams.example.unicast.AsyncIterablePublisher$SubscriptionImpl.run(AsyncIterablePublisher.java:197)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1146)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
        at java.lang.Thread.run(Thread.java:701)
    Caused by: org.reactivestreams.tck.support.SubscriberBufferOverflowException: Received more than bufferSize (32) onNext signals. The Publisher probably emited more signals than expected!
        at org.reactivestreams.tck.TestEnvironment$ManualSubscriber.onNext(TestEnvironment.java:236)
        at org.reactivestreams.tck.TestEnvironment$ManualSubscriberWithSubscriptionSupport.onNext(TestEnvironment.java:442)
        at org.reactivestreams.tck.PublisherVerification$15$2.onNext(PublisherVerification.java:600)
        at org.reactivestreams.example.unicast.AsyncIterablePublisher$SubscriptionImpl.doSend(AsyncIterablePublisher.java:154)
        ... 5 more
    Caused by: java.lang.IllegalStateException: Queue full
        at java.util.AbstractQueue.add(AbstractQueue.java:99)
        at java.util.concurrent.ArrayBlockingQueue.add(ArrayBlockingQueue.java:237)
        at org.reactivestreams.tck.TestEnvironment$Receptacle.add(TestEnvironment.java:791)
        at org.reactivestreams.tck.TestEnvironment$ManualSubscriber.onNext(TestEnvironment.java:233)
        ... 8 more

I think they all originate back to the publisher now being async, need to look into this in-depth (setting expectations here though - in-depth looking I'll probably be only able to do from the 4th Jan on).

@viktorklang

viktorklang · 2015-01-02T21:37:23Z

Gradle test > org.reactivestreams.example.unicast.IterablePublisherTest.required_spec317_mustSignalOnErrorWhenPendingAboveLongMaxValue FAILED
java.lang.AssertionError: Expected onError(java.lang.IllegalStateException) within 100 ms
Caused by:
java.lang.AssertionError: Expected onError(java.lang.IllegalStateException) within 100 ms

Isn't this just a case of TravisCI being slower than our local machines?

viktorklang · 2015-01-02T21:38:27Z

@ktoso and the next failure is just due to the first failure, since that ABQ is just 1 element big?

ktoso · 2015-01-02T23:54:09Z

Brain stormed with @viktorklang and fixed the "timeout is too little for travis" problem by introducing env overrides for all timeouts. They also got default values now, please check ktoso@8769da9 for changes around that. In general it's now possible to export DEFAULT_TIMEOUT_MILLIS=300 and the TCK will use it.

Tests passed on Travis now with increased timeouts.
We can still see the failure from the async publisher in required_spec306_afterSubscriptionIsCancelledRequestMustBeNops but it's not failing the test, need to look deeper into that one once I get off down from the moutnains :-)

But in general this PR should be OK now, unless I made some silly typo... Please review :-)

viktorklang · 2015-01-03T19:02:30Z

Since it is the following test that fails:

@Test // Must be here for TestNG to find and run this, do not remove
public class UnboundedIntegerIncrementPublisherTest extends PublisherVerification<Integer> {

  private ExecutorService e;
  @BeforeClass void before() { e = Executors.newFixedThreadPool(4); }
  @AfterClass void after() { if (e != null) e.shutdown(); }

  public UnboundedIntegerIncrementPublisherTest() {
    super(new TestEnvironment());
  }

  @Override public Publisher<Integer> createPublisher(long elements) {
    return new InfiniteIncrementNumberPublisher(e);
  }

  @Override public Publisher<Integer> createErrorStatePublisher() {
    return null;
  }

  @Override public long maxElementsFromPublisher() {
    return super.publisherUnableToSignalOnComplete();
  }
}

I'd suggest you have another look at how publisherUnableToSignalOnComplete affects the PublisherVerification, I suspect there's a missing check somewhere and then we request a huge number (I increased the ABQ size to 32k and it still overflows).

ktoso · 2015-01-07T15:15:56Z

@viktorklang latest commit fixes a "leaked still alive and kicking publisher" which leaked from spec303, yet caused the stacktrace we noticed in required_spec306_afterSubscriptionIsCancelledRequestMustBeNops (since it's async, and keeps publishing, it accidentally blows up during the execution of spec306 - which is run after the 303 spec).

The new impl is much safer in face of different Publishers, please review ktoso@f58cc7c - the commit message has more details and there's some comments in line explaining rationale on skipping super.onNext() for example.

viktorklang · 2015-01-07T22:28:39Z

Awesome that you found the issue, @ktoso!

ktoso · 2015-01-08T00:56:26Z

Addressed feedback, simplified code a bit - no need for additional configuration method, we're using final long oneMoreThanBoundedLimit = boundedDepthOfOnNextAndRequestRecursion() + 1; as "ok, enough elements signalled, I can stop the test already"

ktoso · 2015-01-08T09:09:27Z

resolves #181

viktorklang · 2015-01-08T19:40:40Z

examples/src/test/java/org/reactivestreams/example/unicast/AsyncSubscriberTest.java

+    return element;
+  }
+
+  @Override public Publisher<Integer> createHelperPublisher(long elements) {


This override seems meaningless :)

thanks, removed!

viktorklang · 2015-01-08T19:47:55Z

👍, this looks excellent Konrad!

@reactive-streams/contributors please vote for this TCK improvement before monday the 12th of January, or ask for more time to think about this before the 12th. Thanks!

viktorklang · 2015-01-12T14:22:04Z

.travis.yml

@@ -1,11 +1,13 @@
 language: java
 script:
- ./gradlew check
+  - ./gradlew check --info


Should this be left in?

I think so, yes - because +travis run gradle with --info; otherwise we don't get any meaningful stacktraces out of the travis test runs. Without it we get failures on travis which are as helpful as "there was a failure", with --info we get a stacktrace of the failure.

Fair enough :)

--info adds a lot of noise.

Add this to about line 27 of build.gradle…

tasks.withType(Test) { testLogging { exceptionFormat "full" events "failed", "started" } }

Nice, thanks @alkemist! I'll include it in this PR then
started makes more sense than passed for us I guess, since we care about the skipped ones being run as well.

viktorklang · 2015-01-12T14:25:02Z

Aside from comments, LGTM!

Since the Publisher could stay alive and keep publishing, and keep getting demand in the recursion depth detecting test it would exceed the buffer size by accumulating these onNext signals, and then cause an error during *a different tests execution* Now we do not accumulate these elements, and also set an explicit limit on how long we try to blow up the recursive stack depth. Stack depth is now also logged in as debug information which should help implementers notice how their imple behave in this scenario. The publisher is now always canceled at the end of this test, in case it did not reach the max nr of elements it is allowed to publish here, nor has it signalled completion.

@alkemist

Thanks to @alkemist for pointing this out

viktorklang · 2015-01-13T14:04:56Z

Alright, LGTM, merging this since review feedback has been addressed.

+TCK: Provide helper publisher (rebased)

ktoso added 2 commits January 2, 2015 21:59

+tck 2.9 blackbox verification should fully drive publisher

e9ee2cd

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 mentioned this pull request Jan 2, 2015

Provide publisher ktoso rebased #192

Closed

ktoso force-pushed the provide-publisher-ktoso-rebased branch 2 times, most recently from bc4729b to 8769da9 Compare January 2, 2015 23:48

ktoso force-pushed the provide-publisher-ktoso-rebased branch from 8ed7a41 to f58cc7c Compare January 7, 2015 18:21

ktoso force-pushed the provide-publisher-ktoso-rebased branch 2 times, most recently from 4af56c3 to a588fa0 Compare January 8, 2015 00:54

ktoso force-pushed the provide-publisher-ktoso-rebased branch from a588fa0 to f6367fe Compare January 8, 2015 09:23

viktorklang reviewed Jan 8, 2015
View reviewed changes

ktoso force-pushed the provide-publisher-ktoso-rebased branch from 0138433 to 469d576 Compare January 9, 2015 00:46

viktorklang reviewed Jan 12, 2015
View reviewed changes

ktoso force-pushed the provide-publisher-ktoso-rebased branch from 469d576 to 3822a3a Compare January 12, 2015 23:17

ktoso and others added 2 commits January 13, 2015 00:26

+tck timeouts now have default values as well as are ENV overrideable

b17a364

ktoso force-pushed the provide-publisher-ktoso-rebased branch from 3822a3a to b117f5a Compare January 12, 2015 23:27

ktoso force-pushed the provide-publisher-ktoso-rebased branch 2 times, most recently from adb4fb0 to 945bda4 Compare January 12, 2015 23:29

+build make buildlog include tests run and fail stacktraces

fdf9a78

Thanks to @alkemist for pointing this out

ktoso force-pushed the provide-publisher-ktoso-rebased branch from 945bda4 to fdf9a78 Compare January 12, 2015 23:30

viktorklang added a commit that referenced this pull request Jan 13, 2015

Merge pull request #193 from ktoso/provide-publisher-ktoso-rebased

737c1f2

+TCK: Provide helper publisher (rebased)

viktorklang merged commit 737c1f2 into reactive-streams:master Jan 13, 2015

viktorklang deleted the provide-publisher-ktoso-rebased branch January 13, 2015 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

+TCK: Provide helper publisher (rebased) #193

+TCK: Provide helper publisher (rebased) #193

ktoso commented Jan 2, 2015

ktoso commented Jan 2, 2015

viktorklang commented Jan 2, 2015

viktorklang commented Jan 2, 2015

ktoso commented Jan 2, 2015

viktorklang commented Jan 3, 2015

ktoso commented Jan 7, 2015

viktorklang commented Jan 7, 2015

ktoso commented Jan 8, 2015

ktoso commented Jan 8, 2015

viktorklang Jan 8, 2015

ktoso Jan 9, 2015

viktorklang commented Jan 8, 2015

viktorklang Jan 12, 2015

ktoso Jan 12, 2015

viktorklang Jan 12, 2015

ldaley Jan 12, 2015

ktoso Jan 12, 2015

viktorklang commented Jan 12, 2015

viktorklang commented Jan 13, 2015

+TCK: Provide helper publisher (rebased) #193

+TCK: Provide helper publisher (rebased) #193

Conversation

ktoso commented Jan 2, 2015

ktoso commented Jan 2, 2015

viktorklang commented Jan 2, 2015

viktorklang commented Jan 2, 2015

ktoso commented Jan 2, 2015

viktorklang commented Jan 3, 2015

ktoso commented Jan 7, 2015

viktorklang commented Jan 7, 2015

ktoso commented Jan 8, 2015

ktoso commented Jan 8, 2015

viktorklang Jan 8, 2015

Choose a reason for hiding this comment

ktoso Jan 9, 2015

Choose a reason for hiding this comment

viktorklang commented Jan 8, 2015

viktorklang Jan 12, 2015

Choose a reason for hiding this comment

ktoso Jan 12, 2015

Choose a reason for hiding this comment

viktorklang Jan 12, 2015

Choose a reason for hiding this comment

ldaley Jan 12, 2015

Choose a reason for hiding this comment

ktoso Jan 12, 2015

Choose a reason for hiding this comment

viktorklang commented Jan 12, 2015

viktorklang commented Jan 13, 2015