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

Bring back TCK for 0.4.0 #91

Merged
merged 17 commits into from
Sep 19, 2014

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Jul 29, 2014

Initial draft of 0.4.0 TCK.
Submitting this PR "early", as in it's not completely done, but I want to get your feedback as soon as possible.

Please have a look at PublisherVerification and SubscriberVerification first - the specs are documented and link to the respective points in the spec.

IdentityProcessorVerification delegates to subscriber and publisher tests, as well as contains some additional specs which I have not yet reviewed if they make sense under the 0.4.0.M2 spec.

Some initial ideas around #87 are also included in this PR, and I would welcome opinions about it. The idea is around maxElementsFromPublisher, which implementors can override, and then if it's 1 for example, but my test requires at least 3 elements, we can mark this test as skipped.

In order to run the TCK against your implementations (please do report if I got anything wrong) you can publish it locally: sbt publishLocal (to local ivy repo) or sbt publishM2 (to local maven repo).

This PR aims to eventually resolve #85
Also addresses "limited publishers"- #87

Thanks in advance for your feedback and comments!

Depends on:

Implementations passing the TCK:

  • akka streams - passes on branch, needs rebase // currently WIP
  • reactor - passes
  • ratpack - currently WIP
  • rx - soon to be WIP
  • vert.x - WIP on TCK compat

import java.util.HashSet;
import java.util.Set;

public abstract class IdentityProcessorVerification<T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not yet fully cleaned up (contains tests from old tck)

@ktoso
Copy link
Contributor Author

ktoso commented Aug 4, 2014

Pushed some updates, the int => long migration is a separate PR now, and this PR depends on it – I'll rebase again once it's in (commit ed9465a). Still needs work around supporting 1 element publishers as well as more "stochastic" tests if we want to go that road.

* @param t throwable to be matched for fatal-ness
* @return true if is a non-fatal throwable, false otherwise
*/
public static boolean apply(Throwable t) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the Java audience I’d rename this one to isNonFatal to be able to statically import it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also made constructor private – no need for instances of NonFatal)

@rhart
Copy link

rhart commented Sep 5, 2014

Hi, I've been looking at Subscriber blackbox tests today. Has been pretty pain free so far, thanks for all this hard work!

I wonder whether the TCK could provide the helper Publisher? Why does the implementator need to provide this?

@smaldini
Copy link
Contributor

smaldini commented Sep 5, 2014

Making an agnostic publisher is not that easy as it should support a minimum scope of the rules itself. If that is an issue because the publisher in ratpack is tied to a web context, feel free to pull a dependency onto reactor-core 👍 (hehe) and use:

Streams.defer(....); to create your publisher (Stream implements Publisher).

@rhart
Copy link

rhart commented Sep 5, 2014

Good plug for Reactor :) Good idea though too, might be worth a mention in the readme?

@ktoso
Copy link
Contributor Author

ktoso commented Sep 6, 2014

The initial idea of the helperPublisher is to be able to test your code in presence of different types publishers because even with the spec there's some room for nuances in implementations which you may want to try to test against.

If there's enough demand I think we could provide a "simpleIterablePublisher" but I'd opt for keeping the method abstract, so you actively think about and select what publisher you're running with. Opinions?

@ldaley
Copy link
Contributor

ldaley commented Sep 6, 2014

I agree that the tester should supply the impl.

On Sun, Sep 7, 2014 at 1:50 AM, Konrad Malawski notifications@github.com
wrote:

Tthe initial idea of the helperPublisher is to be able to test your code in presence of different types publishers because even with the spec there's some room for nuances in implementations which you may want to try to test against.

If there's enough demand I think we could provide a "simpleIterablePublisher" but I'd opt for keeping the method abstract, so you actively think about and select what publisher you're running with. Opinions?

Reply to this email directly or view it on GitHub:
#91 (comment)

if your impl is not able to support multiple subscribers
@ktoso
Copy link
Contributor Author

ktoso commented Sep 15, 2014

@reactive-streams/contributors Ping. Any opinions if we can merge this and perhaps release another milestone? We need a few more votes I believe.

It would make things easier for devs who are now trying to implement their libraries, and test drive the TCK.

@viktorklang
Copy link
Contributor

I think it makes total sense to merge this and cut another milestone release of 0.4 as it'd make it much easier for implementors to try it out and give feedback.

@smaldini
Copy link
Contributor

I would expect at least another implementor to use it. What about Ratpack and Vert.x @alkemist @purplefox ?
Also @benjchristensen work on reactiveStreams on top of RxJava should complete the feedback round very nicely.

@ldaley
Copy link
Contributor

ldaley commented Sep 15, 2014

We're already using it, if that's what you mean.

https://github.com/ratpack/ratpack/tree/master/ratpack-core/src/test/groovy/ratpack/stream/tck

@smaldini
Copy link
Contributor

Ha does it fully pass ?

@ldaley
Copy link
Contributor

ldaley commented Sep 15, 2014

The tests that are there are passing yes. We have a few more tests to add though.

@smaldini
Copy link
Contributor

If you don't have any specific feedbacks I think we have our 3 implementors to go for 0.4.0. We might wait for Vert.x and RxJava before 1.0 then :)

@viktorklang
Copy link
Contributor

@smaldini We were only talking about releasing an M3 of 0.4 so far :), and then get some extra feedback before we release 0.4, then we need to achieve the goals for 1.0 before we can ship 1.0-RC1.

I'm really proud of what we have achieved so far!

@smaldini
Copy link
Contributor

I think nothing prevents us to go for 0.4 but if the scope is only M3 we should really do it like today :)

@viktorklang
Copy link
Contributor

@smaldini I'd be happy with a 0.4.0-M3 today :)

@rkuhn
Copy link
Member

rkuhn commented Sep 18, 2014

@benjchristensen @purplefox @tmontgomery @mariusaeriksen we need two more LGTM in order to proceed with this (i.e. merge it and release the artifacts as 0.4.0.M3 so that further TCK validation can proceed more easily), is there anything holding this back?

@benjchristensen
Copy link
Contributor

Nothing I'm aware of but I haven't had time yet to try it. I imagine I'll want the new artifacts when I try so I'm good releasing since this isn't 1.0.

@tmontgomery
Copy link

I'll defer to you guys on this.

@smaldini
Copy link
Contributor

All right I guess we have our two chaps. Let's accept the Konrad PR and proceed then.

smaldini added a commit that referenced this pull request Sep 19, 2014
@smaldini smaldini merged commit 9a6df52 into reactive-streams:master Sep 19, 2014
@ktoso
Copy link
Contributor Author

ktoso commented Sep 19, 2014

So great to see this merged, thanks a lot for all the feedback and cooperation guys (and for the upcoming feedback once others try it :-))!

@smaldini
Copy link
Contributor

Thanks to you to make it real :) that was great to just polish a couple detail and beta test the beast. Really impressive work. Waiting for Maven Central to sync on 0.4.0

@viktorklang
Copy link
Contributor

Thanks everyone, I can't believe how close we are now! :)

@ktoso ktoso deleted the bring-back-TCK-ktoso branch October 1, 2014 10:21
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

Successfully merging this pull request may close these issues.

Migrate TCK from 0.3.0 to 0.4.0