Add Javadoc explanation to the TCK test methods about what they do #356
Conversation
|
Nice suff , wdyt @reactive-streams/contributors |
|
Looks OK to me. |
There was a problem hiding this comment.
Thanks for the documentation, @akarnokd!
I trust you have generated the docs to verify the formatting etc of the outputted html?
| * Asks for a {@code Publisher} that should emit exaclty one item and complete (both within a | ||
| * timeout specified by {@link org.reactivestreams.tck.TestEnvironment#defaultTimeoutMillis()}). | ||
| * <p>The test is not executed if {@link org.reactivestreams.tck.PublisherVerification#maxElementsFromPublisher()} returns zero. | ||
| * <p>This test mostly ensures that the {@code Publisher} implementation is actually operational. |
There was a problem hiding this comment.
This sentence is very vague, don't you agree?
There was a problem hiding this comment.
The sentence is totally fine. If the non-empty Publisher fails this test, there is a fundamental mistake in its implementation. See the hints below this javadoc line.
There was a problem hiding this comment.
What I meant was that "mostly ensures" and "actually operational" are vague. :)
There was a problem hiding this comment.
Removed those two sentences.
| void required_createPublisher1MustProduceAStreamOfExactly1Element() throws Throwable; | ||
| /** | ||
| * Asks for a {@code Publisher} that should emit exaclty three items and complete (all within a |
| * Asks for a {@code Publisher} that should emit exaclty three items and complete (all within a | ||
| * timeout specified by {@link org.reactivestreams.tck.TestEnvironment#defaultTimeoutMillis()}). | ||
| * <p>The test is not executed if {@link org.reactivestreams.tck.PublisherVerification#maxElementsFromPublisher()} is less than 3. | ||
| * <p>This test mostly ensures that the {@code Publisher} implementation is actually operational. |
There was a problem hiding this comment.
Too vague IMO. (Doesn't inspire confidence)
| void optional_spec105_emptyStreamMustTerminateBySignallingOnComplete() throws Throwable; | ||
| /** | ||
| * Currently, this test is skipped because it is unclear this rule can be effectively checked |
There was a problem hiding this comment.
Great explanation, thanks!
| /** | ||
| * Asks for an empty {@code Publisher} and calls and verifies if {@code onSubscribe} was called before any calls | ||
| * to the other {@code onXXX} methods. | ||
| * <p>Note that this test doesn't request anything yet an {@code onNext} is not considered as a failure. |
There was a problem hiding this comment.
This sentence reads weirdly, clarify?
There was a problem hiding this comment.
It is supposed to mean that in case the Publisher doesn't respect backpressure, this test will still pass. Otherwise the implementation is odd because without requesting, there is no point in verifying onNext has been called after an onSubscribe.
There was a problem hiding this comment.
I think it is the "anything yet an" part, is there a comma or something missing?
There was a problem hiding this comment.
I'll rephrase it with "anything, however, an ".
| void required_spec303_mustNotAllowUnboundedRecursion() throws Throwable; | ||
| /** | ||
| * Currently, this test is skipped because a {@code request} could enter into a synchronous computation via {@code onNext} | ||
| * legally and otherwise there is no common agreement on what constitutes as heavy computation. |
There was a problem hiding this comment.
There is a definition for "heavy computation" part of the spec now in the glossary?
There was a problem hiding this comment.
How about rewording as "and otherwise there is no common agreemend how to detect such heavy computation reliably."
| * <p>Note that this test expects the {@code IllegalArgumentException} being signalled through {@code onError}, not by | ||
| * throwing from {@code request()} (which is also forbidden) or signalling the error by any other means (i.e., through the | ||
| * Thread.currentThread().getUncaughtExceptionHandler() for example). | ||
| * <p>Note also that requesting and emission may happen concurrently and honoring this rule may require extra serialization within |
There was a problem hiding this comment.
"serialization" already has a bit of definition baggage in the JDK. I'd use "coordination" instead.
No, just looked at the result from the Eclipse's Javadoc view. |
|
Fixed spelling errors, clarified a couple of sentences. |
|
@ktoso Yay or nay? |
|
Moved continents in the last weeks, should be online this week and back to reviewing / contributing, thanks for your patience. Reading this one. |
|
@ktoso Thanks Konrad, it's not the end of the world if we need to postpone the 1.0.1 another week. |
|
Started reviewing, I think not all comments are accurate, I will have to really focus on this one - review should come up during my day today I hope. |
|
More comments coming now in a batch, however I think since if we're adding such comments, we should also transform the How about adding that as first sentence in all of those javadoc comments (then a )? |
I find those rule explanations not really helpful when considering the concrete unit tests. Perhaps we can link them as a second or later sentence. |
|
It is the spec after all, the source of truth - we should link to it. People should not just rely on the TCK comments, but get an understanding of the spec in which these comments can (and will) help :-) You're right about them not being the most important thing though when reading the comments - how about we add them as last sentence - with a so it's nicely separated etc. // finishing review now, overall very good - some nitpicks |
| * timeout specified by {@link org.reactivestreams.tck.TestEnvironment#defaultTimeoutMillis()}) | ||
| * in response to a request(1). | ||
| * <p>The test is not executed if {@link org.reactivestreams.tck.PublisherVerification#maxElementsFromPublisher()} returns zero. | ||
| * If this test fails (likely with a timeout error), the following could be checked within the {@code Publisher} implementation: |
There was a problem hiding this comment.
Let's avoid "likely with a timeout" and other phrasings like that IMHO, we simply provide hints of what to check - it could fail for different reasons, just being completely wrong and emitting values not asked for etc.
| * <li>in the {@code Publisher.subscribe(Subscriber)} method, the {@code Subscriber.onSubscribe} is called | ||
| * as part of the preparation process (usually before subscribing to other {@code Publisher}s),</li> | ||
| * <li>if the {@code Publisher} implementation works for a consumer that calls {@code request(1)},</li> | ||
| * <li>if the {@code Publisher} implementation is able to emit an {@code onComplete} without requests.</li> |
There was a problem hiding this comment.
The points are good, but focusing on the timeout case. Please add a point about "that it does not emit more than allowed elements (exactly one)". Trivial, but since we're adding such text, it should be included (many of the points are "trivial", so if we list them, we should list all that matter).
There was a problem hiding this comment.
Okay, but my experience is that it is rarely the case of emitting more than the requested amount but instead failing to emit or losing signal somewhere in the chain.
| /** | ||
| * Asks for a {@code Publisher} that should emit exactly three items and complete (all within a | ||
| * timeout specified by {@link org.reactivestreams.tck.TestEnvironment#defaultTimeoutMillis()}). | ||
| * <p>The test is not executed if {@link org.reactivestreams.tck.PublisherVerification#maxElementsFromPublisher()} is less than 3. |
There was a problem hiding this comment.
Please use the same style of
in javadoc as the rest of the repo, so https://github.com/reactive-streams/reactive-streams-jvm/blob/master/api/src/main/java/org/reactivestreams/Subscriber.java#L18 on an empty like like that
| * <li>in the {@code Publisher.subscribe(Subscriber)} method, the {@code Subscriber.onSubscribe} is called | ||
| * as part of the preparation process (usually before subscribing to other {@code Publisher}s),</li> | ||
| * <li>if the {@code Publisher} implementation works for a consumer that calls {@code request(1)} after consuming an item,</li> | ||
| * <li>if the {@code Publisher} implementation is able to emit an {@code onComplete} without requests.</li> |
There was a problem hiding this comment.
This hint is not correct. There is a request(1) issued before expecting the completion
There was a problem hiding this comment.
I was in an odd position because it was declared by one of you that onComplete must happen without requesting yet this test requests. Thus two options here: 1) leave this and fix the test or 2) change the wording and have a rule inconsistency.
| * <li>if the {@code Publisher} implementation works for a consumer that calls {@code request(1)},</li> | ||
| * <li>if the {@code Publisher} implementation is able to emit an {@code onComplete} without requests.</li> | ||
| * </ul> | ||
| * @throws Throwable allow arbitrary exceptions to be thrown by the implementation of this test |
There was a problem hiding this comment.
Not sure this adds much useful information, smells like documenting "what a getter is" to me, omit on all methods? It's a pretty known pattern to allow test methods to throw whatever they want
| void untested_spec304_requestShouldNotPerformHeavyComputations() throws Exception; | ||
| /** | ||
| * Currently, this test is skipped because there is no common agreement on what constitutes as heavy computation. |
There was a problem hiding this comment.
I don't like the phrasing of "no common agreement on what ... is heavy", isn't it more about "because there is no reliable agreed upon way to detect a heavy computation"?
| void required_spec307_afterSubscriptionIsCancelledAdditionalCancelationsMustBeNops() throws Throwable; | ||
| /** | ||
| * Asks for a short {@code Publisher} (length 10) and issues a {@code request(0)} which should trigger an {@code onError} call |
There was a problem hiding this comment.
I guess we could actually make that length smaller - not that it matters much in practice I guess...
There was a problem hiding this comment.
I proposed that many tests should actually try multiple lengths and there is an example PR of this proposal. In practice, a chain of operators may behave differently when the Publisher is known to be of length 0 or 1. 2+ is handled with the same algorithm but some peculiar concurrency bugs may appear above 10-20 because of buffers spanning multiple cache lines...
There was a problem hiding this comment.
Will review that one next then
| * <p>The test is not executed if {@link org.reactivestreams.tck.PublisherVerification#maxElementsFromPublisher()} is less than 10. | ||
| * <p>Note that this test expects the {@code IllegalArgumentException} being signalled through {@code onError}, not by | ||
| * throwing from {@code request()} (which is also forbidden) or signalling the error by any other means (i.e., through the | ||
| * Thread.currentThread().getUncaughtExceptionHandler() for example). |
| * in general. | ||
| * </ul> | ||
| * @throws Throwable allow arbitrary exceptions to be thrown by the implementation of this test | ||
| */ |
| void required_spec313_cancelMustMakeThePublisherEventuallyDropAllReferencesToTheSubscriber() throws Throwable; | ||
| /** | ||
| * Asks for a short {@code Publisher} (length 3) and requests {@code Long.MAX_VALUE} from it, verifying that the | ||
| * {@code Publisher} emits all of its items and completes normally. |
There was a problem hiding this comment.
"and does not keep spinning attempting to fulfil the MAX_VALUE demand by some means"
| * <p>The test is not executed if {@link org.reactivestreams.tck.PublisherVerification#maxElementsFromPublisher()} is less than 10. | ||
| * <p>Note that this test expects the {@code IllegalArgumentException} being signalled through {@code onError}, not by | ||
| * throwing from {@code request()} (which is also forbidden) or signalling the error by any other means (i.e., through the | ||
| * Thread.currentThread().getUncaughtExceptionHandler() for example). |
There was a problem hiding this comment.
Added {@code } here as well.
|
Changed the javadoc for most of the feedback from @ktoso. |
|
Thanks for addressing the comments - I'll have a last look in my morning
and should be good then.
Cheers
…--
Konrad Malawski
On 28 April 2017 at 02:07:03, David Karnok ***@***.***) wrote:
***@***.**** commented on this pull request.
------------------------------
In
tck/src/main/java/org/reactivestreams/tck/support/PublisherVerificationRules.java
<#356 (comment)>
:
> void stochastic_spec103_mustSignalOnMethodsSequentially() throws Throwable;
+ /**
+ * Verifies that if the call to ***@***.*** PublisherVerification.createErrorPublisher()} returns a non-null ***@***.*** Publisher},
+ * it calls ***@***.*** onSubscribe} exactly once followed by a single call to ***@***.*** onError()} without issuing any requests and otherwise
+ * not throwing any exception.
+ * <p>If this test fails, the following could be checked within the error ***@***.*** Publisher} implementation:
+ * <ul>
+ * <li>the ***@***.*** Publisher.subscribe(Subscriber)} method has actual implementation,</li>
+ * <li>in the ***@***.*** Publisher.subscribe(Subscriber)} method, if there is an upstream ***@***.*** Publisher},
+ * that ***@***.*** Publisher} is actually subscribed to,</li>
+ * <li>if the ***@***.*** Publisher} implementation is able to emit an ***@***.*** onError} without requests,</li>
+ * <li>if the ***@***.*** Publisher} is non-empty as this test requires a ***@***.*** Publisher} without items.</li>
Updated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#356 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHYk8NagCHjbosN4bhC-ql_TQP1wYeDks5r0Ms3gaJpZM4My_Us>
.
|
| * <li>if a {@code request()} call from {@code onSubscribe()} could trigger an asynchronous call to {@code onNext()} and if so, make sure | ||
| * such {@code request()} calls are deferred until the call to {@code onSubscribe()} returns normally.</li> | ||
| * </ul> | ||
| */ |
There was a problem hiding this comment.
please do add links to the spec in all tests as I mentioned previously.
We can then also delete the // Verifies rule: https://github.com/reactive-streams/reactive-streams-jvm#3.2 inline comments from the impls
|
Yes. |
|
Merged! Massive thanks @akarnokd! 👍 |
This PR adds some explanation about what each
PublisherVerificationRulesmethods do in respect to their expectation and request/emission pattern that could help tracking down the errors in thePublisherunder test and tries to hint about what could be wrong in thatPublisher.I run into situations when there were errors in my
Publishers but the error response didn't help much and had to look into what the test actually did (in C# it was even harder due to lack of nicely linked TCK sources) and single-stepping through is cumbersome because of the several layers of test equipment within the TCK.Let me address some concerns upfront:
Why
PublisherVerificationRulesand notPublisherVerification?Mainly because the Javadoc would need duplication for
IdentityProcessorVerificationwhich is double maintenance work.Why exposing the implementation details?
Knowing the request pattern or the expected emission pattern without digging into the TCK sources helps identify mistakes in the
Publishermuch faster, if not allow separate unit tests to be built with the same properties and debug thePublisherthrough it.What about
SubscriberWhiteboxVerificationRules?If there is interest for such kind of documentation enhacements, I can do that after this.