Removes the use of annotations, switches to test prefix naming

[viktorklang]

Introduces interfaces to specify which tests should be implemented for the different *Verification classes, which makes it much easier to keep track that test cases have been properly implemented.

IMPORTANT Fixes #188 by making the 2.12 rule "untested" in the TCK as it is not reasonably testable.

Removes the annotations that were used for TCK documentation, since they were confusing and not up to date. Replaces them with prefix naming. ("optional"/"required"/"untested"/"stochastic")

[smaldini]

Trying

[smaldini]

Mhhh looks like it still tries the test, should I override myself in the end-test with @Additional too ?

[viktorklang]

Shouldn't have to, but try.

Cheers,
√
On 29 Dec 2014 13:43, "Stephane Maldini" [email protected] wrote:

Mhhh looks like it still tries the test, should I override myself in the
end-test with @Additional too ?

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

[smaldini]

Ahaha can't its package scope :) Note to be sure I have incremented the version to RC2.BUILD-SNAPSHOT.

This works

    @Override
    @org.testng.annotations.Test
    public void spec212_mustNotCallOnSubscribeMoreThanOnceBasedOnObjectEquality_specViolation() throws Throwable {
    }

[smaldini]

I can also implement some logic to switch to an hot stream in this override but I suppose the goal is to have the TCK itself flexible on that rule.

[viktorklang]

Does this work?

@Override
    @org.testng.annotations.Test
    @Additional
    public void spec212_mustNotCallOnSubscribeMoreThanOnceBasedOnObjectEquality_specViolation() throws Throwable {
       super.spec212_mustNotCallOnSubscribeMoreThanOnceBasedOnObjectEquality_specViolation();
    }

[viktorklang]

I just pushed a new commit where I introduce interfaces to keep track that the right methods are reflected in both the Publisher, Subscriber and Processor verifications.

[viktorklang]

Can you try this branch?

[smaldini]

@Additional is package scoped like other TCK annotations. Going to try the latest commit in a min.

[viktorklang]

@smaldini nevermind the annotations, they are only documentation AFAIK

[smaldini]

Still testing (and failing) 2.12 for me without override Noop.

[ktoso]

The annotations are just docs. See RetentionPolicy.SOURCE: https://github.com/reactive-streams/reactive-streams/blob/master/tck/src/main/java/org/reactivestreams/tck/Annotations.java#L16

[smaldini]

As annotation is just doc, maybe we should remove @Test from 212 and leave the implementation override and add the test scope. Or we add the annotations handling and an Environment boolean to enable/disable testing for @Additional specs.

[ktoso]

// Sorry, my crappy internet connection made me think I sent an additional comment here but it seems I did not. I'm in the mountains for a week - sorry for not being too responsive over this week!

I'll open a separate issue about what to do with those annotations. Seems that they being just docs can be quite misleading - we're all accustomed to annotations "doing all the things" I guess. An idea would be to instead of separate optionalSubscriberTestWithoutSetup / subscriberTest make it simply use these annotations..

Made it a ticket: #190

[viktorklang]

This PR should also fix #190

[viktorklang]

@reactive-streams/contributors Please review/vote/ask for more time before the 2nd of January 2015, this change needs to go into an RC2.

[viktorklang]

@ktoso would like you take on this comment

[ktoso]

oh, this is a case of confusing naming I think. It is indeed optional - the way errorPublisherTest works is "if the publisher I get from createErrorPublisher() (user does this), is not null, I'm doing to run this test"). Let's rename the method name to optionalErrorPublisherTest and we're good here IMO

[viktorklang]

@ktoso would like you take on this comment

[ktoso]

The optional method is here because "same order of elements" in this test is checked after a number of elements being signalled. But what if the publisher can only ever signal 1 time (e.g. FutureSource), in that case the TCK skips this test even though its "required", but well, this Publisher cannot be tested using this test.

[ktoso]

This PR misses updating the tck/README.md about our wording used here.
Possibly add a word or two about how we plan to evolve the tck using thie style of methods?

[viktorklang]

@ktoso Nice catch, I'll have a look at the RCK README and see if I can improve it.

[ktoso]

I also touched the README in my PR, I think we'll get conflicts on that one hm..
I'm out of brain power for today to go over the other lines in this PR without making a mistake now I fear, I'll get to those soon I hope - in general I'd say "trust the code, not the annotation" in cases we had a mistake.

[viktorklang]

Renamed "unverified" to "untested" (same length as "optional" and "required")
Changed ___ to _

[ktoso]

Went through the FIXMEs, it's those cases we've added recently to support weird publishers "can only signal 1 element" and "can never signal oncomplete" etc. Those tests are required in general, but can be forced to skip if you configure the TCK by telling it "my publisher can not signal completion" etc.

These are explained in the tck/README.md section 'Testing Publishers with restricted capabilities'. Let me know if we should improve the wording there or do something else.

[ktoso]

Single underscore + rename works great, thanks! :)

[viktorklang]

Pushed changes based on @ktoso's feedback

[ktoso]

LGTM :-)

[viktorklang]

No objections, merging :)