!tck #12 Migrated TCK to plain Java #15

ktoso · 2014-04-14T07:05:16Z

Migrated TCK to plain Java.
Had to replicate trait behaviour by delegating manually, so there's a bit more duplication (Processor test), but we're not locked to a concrete Scala version.

Please review and I'll rename the classes Java* => * and remove the Scala versions of them - then let's merge.

// cc @viktorklang

ktoso · 2014-04-14T07:05:57Z

tck/build.sbt

 libraryDependencies += "org.testng" % "testng" % "5.14.10"
+


will add auto autoScalaLibrary := false when removing the Scala versions of the classes

ktoso · 2014-04-14T07:07:35Z

Implementing from Scala now looks like this:

class ActorProducerTest(_system: ActorSystem, env: JavaTestEnvironment, publisherShutdownTimeout: Long)
  extends JavaPublisherVerification[Int](env, publisherShutdownTimeout)
  with WithActorSystem with TestNGSuiteLike {

  implicit val system = _system
  import system.dispatcher

  def this(system: ActorSystem) {
    this(system, new JavaTestEnvironment(Timeouts.defaultTimeoutMillis(system)), Timeouts.publisherShutdownTimeoutMillis)
  }
  def this() {
    this(ActorSystem(classOf[ActorProducerTest].getSimpleName, AkkaSpec.testConf))
  }

This is in order to make defaultTimeoutMillis dilated (so the actor system must come in from the constructor, and not from a trait.

viktorklang · 2014-04-14T07:53:21Z

Just before I go through the code; have we've verified that the current akka-streams impl passes the migrated TCK?

ktoso · 2014-04-14T07:54:34Z

Yes, I've got it migrated to use the new TCK and it's all green.
Will pull request there once this one is OK-ed (I want to trim the Java... prefixes from the classes here).

viktorklang · 2014-04-14T07:56:47Z

Ok, just wanted to make sure that it flies :)

viktorklang · 2014-04-14T08:07:03Z

tck/src/main/java/org/reactivestreams/tck/JavaIdentityProcessorVerification.java

+
+      T x = nextT();
+      sendNext(x);
+      expectNextElement(sub, x);


These three lines seem common enough to warrant a helper

+1, added sendNextTFromUpstream

ktoso · 2014-04-14T09:29:14Z

Updated PR

viktorklang · 2014-04-14T09:31:51Z

tck/src/main/java/org/reactivestreams/tck/JavaIdentityProcessorVerification.java

+   * Override in your test if you want to enable completed-state verification.
+   * If you don't override the respective tests will be ignored.
+   */
+  public Publisher<T> createCompletedStatePublisher() {


I think these should be abstract methods so the implementor has to implement them. i.e. no hidden surprises.

make abstract but keep the style of return null == ignore these tests? Akka streams don't use the error publisher for exampe

Document on the method what one should do. :)

smaldini · 2014-04-14T11:45:10Z

Can't wait for this :) thanks a lot.

viktorklang · 2014-04-14T11:47:57Z

tck/src/main/java/org/reactivestreams/tck/IdentityProcessorVerification.java

+    new TestSetup(env, testBufferSize) {{
+      TestEnvironment.ManualSubscriber<T> sub = newSubscriber();
+      sub.cancel();
+      expectCancelling();


Btw, how does expectCancelling know which Subscriber we expect to get cancelled?

The test does something slightly different - so if all down-stream's are cancelled, we call cancel on the upstream one. (so expectCancelling actually only cares if the ManualPublisher decides to cancel it's upstream subscription).

Ah, that's pretty hard to see from just looking at the code.

patriknw · 2014-04-14T12:20:46Z

LGTM, great work @ktoso

viktorklang · 2014-04-14T12:55:39Z

tck/src/test/java/org/reactivestreams/tck/IdentityProcessorVerificationTest.java

+
+    // then
+    for (String publisherTest : publisherTests) {
+      String msg = String.format("%d tests in %s did not include %s!", processorTests.size(), IdentityProcessorVerification.class, publisherTest);


This output message doesn't seem to make sense for the processorTests.size(), am I missing something?

Hm, read as "[n tests, in the "delegator test suite] test in "delegator" did not include [expected test to be delegated to]"; seems ok to me? // Extracted this loop as method.

bantonsson · 2014-04-14T12:55:42Z

LGTM

viktorklang · 2014-04-14T12:59:19Z

tck/src/main/java/org/reactivestreams/tck/support/Optional.java

+  private static final Optional NONE = new Optional() {
+    @Override
+    public Object get() {
+      throw new RuntimeException(".get call on None!");


NoSuchElementException :)

* In which we trade off less-DRY in order to be scala-version independent. * TCK does not depend on Scala at all now. * Added tests testing the TCK, if we properly delegate all tests from Processor -> {Producer, Subscriber} * Made SBT run the TestNG test * Made the "pending" tests (decided by returning null in the special create... methods) actualy visible as PENDING to testNG

ktoso · 2014-04-14T15:42:00Z

Pushed final squash which addresses all of Viktor's latest feedback, so should be good to go (also tested akka-streams with it and we're green). Highlights include:

some string interpolation was left in the migrated Java code - is now String.format
by throwing SkipException() we're able to let TestNG know that a test is "pending". Tested it in akka-stream and it works as expected (versus previously we'd just "not run" these tests)
Replaced instances of == with equals where checking what a stream produced - no one forces a stream to return the same instance of something
Includes test to check that the Processor test delegates to all test methods in Subscriber and Producer
sbt now runs this test in TCK

Thanks for the awesome code review!

viktorklang · 2014-04-14T15:42:48Z

Fantastic work, Konrad!

LGTM 👍

viktorklang · 2014-04-14T19:29:06Z

I guess we're awaiting a couple of extra +1's before we merge! :-)

rkuhn · 2014-04-14T19:30:53Z

I’m trying out the publishing and cannot build the javadoc due to source duplication, will need to fix the build. But I’ll open a new PR for that once this one is merged, which I’d therefore like to do soon-ish.

viktorklang · 2014-04-14T19:36:07Z

Excellent, just in time!

ktoso · 2014-04-14T19:38:14Z

@rkuhn what command is failing? I worked using publishLocal, that built javadoc well too, hmm...

rkuhn · 2014-04-14T19:39:50Z

I happened to have the old generated .java files still lying around and could therefore reproduce what @smaldini had seen last week, and this time I noticed the duplication in the logs. It turns out that we can now just remove all genjavadoc stuff, you can also do that on this PR. (i.e. delete 1 file and 2 lines)

viktorklang · 2014-04-14T19:43:33Z

Sounds great

ktoso · 2014-04-14T20:21:19Z

Included @rkuhn's javadoc patch; Waiting for last LGTMs / merge and we'll release

rkuhn · 2014-04-14T20:21:33Z

Will merge and publish a 0.3 now (since the first sync to Maven Central can take a lot longer). We can in any case publish a 0.4 if there are things to be fixed.

!tck #12 Migrated TCK to plain Java

viktorklang · 2014-04-14T20:34:05Z

Perfect. Great turn-around time for this one, guys!

ktoso · 2014-04-16T12:12:32Z

lol :) Ok, will make those public; (in a few minutes)

sirthias · 2014-04-16T12:14:36Z

Great, thanks, Konrad!

ktoso reviewed Apr 14, 2014
View reviewed changes

viktorklang reviewed Apr 14, 2014
View reviewed changes

JavaDoc changes in order to be able to publish

c687271

rkuhn added a commit that referenced this pull request Apr 14, 2014

Merge pull request #15 from ktoso/12-port-tck-to-java-ktoso

b42fec3

!tck #12 Migrated TCK to plain Java

rkuhn merged commit b42fec3 into reactive-streams:master Apr 14, 2014

ktoso deleted the 12-port-tck-to-java-ktoso branch April 14, 2014 20:54

rkuhn mentioned this pull request Apr 15, 2014

port TCK to Java #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!tck #12 Migrated TCK to plain Java #15

!tck #12 Migrated TCK to plain Java #15

ktoso commented Apr 14, 2014

ktoso Apr 14, 2014

ktoso commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 14, 2014

viktorklang commented Apr 14, 2014

viktorklang Apr 14, 2014

ktoso Apr 14, 2014

ktoso commented Apr 14, 2014

viktorklang Apr 14, 2014

ktoso Apr 14, 2014

viktorklang Apr 14, 2014

smaldini commented Apr 14, 2014

viktorklang Apr 14, 2014

ktoso Apr 14, 2014

viktorklang Apr 14, 2014

patriknw commented Apr 14, 2014

viktorklang Apr 14, 2014

ktoso Apr 14, 2014

bantonsson commented Apr 14, 2014

viktorklang Apr 14, 2014

ktoso Apr 14, 2014

ktoso commented Apr 14, 2014

viktorklang commented Apr 14, 2014

viktorklang commented Apr 14, 2014

rkuhn commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 14, 2014

rkuhn commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 14, 2014

rkuhn commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 16, 2014

sirthias commented Apr 16, 2014

!tck #12 Migrated TCK to plain Java #15

!tck #12 Migrated TCK to plain Java #15

Conversation

ktoso commented Apr 14, 2014

Choose a reason for hiding this comment

ktoso commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 14, 2014

viktorklang commented Apr 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Apr 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaldini commented Apr 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw commented Apr 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bantonsson commented Apr 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso commented Apr 14, 2014

viktorklang commented Apr 14, 2014

viktorklang commented Apr 14, 2014

rkuhn commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 14, 2014

rkuhn commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 14, 2014

rkuhn commented Apr 14, 2014

viktorklang commented Apr 14, 2014

ktoso commented Apr 16, 2014

sirthias commented Apr 16, 2014