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: untested rules #355

Open
akarnokd opened this issue Apr 4, 2017 · 10 comments
Open

TCK: untested rules #355

akarnokd opened this issue Apr 4, 2017 · 10 comments

Comments

@akarnokd
Copy link
Contributor

akarnokd commented Apr 4, 2017

I think the following tests could be meaningfully implemented:

  • untested_spec107_mustNotEmitFurtherSignalsOnceOnErrorHasBeenSignalled
    • Ask for a error publisher, let it fail, then request(10) and check for no further events
  • untested_spec108_possiblyCanceledSubscriptionShouldNotReceiveOnErrorOrOnCompleteSignals
    • Ask for a 0 length Publisher and cancel from within onSubscribe, then see if onComplete was emitted or not. If not, pass the test, if it was emitted, skip the test.
    • Ask for a 1 length Publisher, cancel in onNext after the first item, etc.
    • Ask for an error Publisher and cancel from within onSubscribe, etc.

(Also it depends on #354).

@viktorklang
Copy link
Contributor

Ask for a error publisher, let it fail, then request(10)

This would be illegal behavior from the Subscriber PoV. (Requesting after onError)

untested_spec108_possiblyCanceledSubscriptionShouldNotReceiveOnErrorOrOnCompleteSignals

Question is if that is workable, since the rule mandates that signals must eventually stop?

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 5, 2017

This would be illegal behavior from the Subscriber PoV. (Requesting after onError)

Which rule says that? Also why is onComplete + request TCK test valid then?

@viktorklang
Copy link
Contributor

@akarnokd 2.4: «Subscriber.onComplete() and Subscriber.onError(Throwable t) MUST consider the Subscription cancelled after having received the signal.
»

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 5, 2017

MUST consider the Subscription cancelled

3.6: After the Subscription is cancelled, additional Subscription.Request(long n) MUST be NOPs.
3.7: After the Subscription is cancelled, additional Subscription.Cancel() MUST be NOPs.

Or is there a difference between actual cancellation and considered cancellation?

@viktorklang
Copy link
Contributor

@akarnokd Those are Subscription rules, not Subscriber rules.

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 5, 2017

So the Subscription in 2.4 is unrelated to the Subscription in 3.6/3.7 because viewpoint?

@akarnokd
Copy link
Contributor Author

akarnokd commented Apr 5, 2017

Also why is this TCK test valid if an onComplete() should not followed by a request()?

  // Verifies rule: https://github.com/reactive-streams/reactive-streams-jvm#1.7
  @Override @Test
  public void required_spec107_mustNotEmitFurtherSignalsOnceOnCompleteHasBeenSignalled() throws Throwable {
    activePublisherTest(1, true, new PublisherTestRun<T>() {
      @Override
      public void run(Publisher<T> pub) throws Throwable {
        ManualSubscriber<T> sub = env.newManualSubscriber(pub);
        sub.request(10);
        sub.nextElement();
        sub.expectCompletion();

        sub.request(10);
        sub.expectNone();
      }
    });
  }

@viktorklang
Copy link
Contributor

@akarnokd Thinking more about it, there could definitely be room for issuing the request after receiving the completion (onError or onComplete) as the request could have been in-flight. Ping @ktoso

@ktoso
Copy link
Contributor

ktoso commented Jul 7, 2017

I thought I had replied here, sorry for that!

Absolutely, since request signals are async, such signal could arrive after the onComplete and the requirement in the spec is that such signal should be noop.

@viktorklang
Copy link
Contributor

@reactive-streams/contributors Should we close this?

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

No branches or pull requests

3 participants