[PR for discussion] Showing Reactive Streams TCK, proposing some #407

ktoso · 2018-01-31T13:50:15Z

The purpose of this PR is to show how to start using the reactive-streams tck (documented here: https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck ) to validate existing RS implementations in this library. Reactive streams implementations must pass the TCK in order to ensure good inter-op between all libraries.

This PR will fail, as not all implementations are currently compliant.

I can help making them compliant, but wanted to reach first with this preview PR to ask if you'd like to separate our the TCK tests into a separate project, or if you're ok keeping them side by side the other ones like this? They are a bit slow sometimes, as they test many runs, and also await for "things did not happen" etc.

Running the tests will show which specification rules have been violated which should help getting the library compliant pretty quickly, esp as the implementations I've seen are pretty simple so far.

Let me know if you'd want more assistance making all the implementations valid, or if you'd like to take the TCK for a spin yourself first -- I've seen you're aware of the specification requirements in some TODO/FIXME's, so perhaps this is something you've not yet gotten to yet?

Hope this helps, let me know what would be the best way to help you out here.

Refs #406 and #405

solutions

ktoso · 2018-01-31T13:51:02Z

core/src/main/java/software/amazon/awssdk/core/async/SingleByteArrayAsyncRequestProvider.java

+        // FIXME: onSubscribe is user code, and could be ill behaved, as library we should protect from this,
+        // FIXME: This is covered by spec rule 2.13; proposal:
+        // As per 2.13, this method must return normally (i.e. not throw).
+        // try {


guards such as this are required by the spec, they're not hard to add. The TCK will fail and show which rule has been broken in the existing implemenations pointing these out.

ktoso · 2018-01-31T13:51:43Z

core/src/main/java/software/amazon/awssdk/core/async/FileAsyncResponseHandler.java

@@ -82,6 +82,7 @@ public ResponseT complete() {
    /**
     * {@link Subscriber} implementation that writes chunks to a file.
     */
+    // FIXME cover with Reactive Streams TCK, looks ok from a first brief look, but could be missing some edge cases


This PR is not (at least yet), intended to be merged, please assume those are just markers for myself to know where/how many implementations are there and what we should cover with tests.

filesystem

ktoso

Added some comments in-line.

Mostly awaiting info if you want to give it a shot to fix the spec violations on your own using the TCK or would ask for some more help with it.

Thanks in advance!

ktoso · 2018-01-31T13:52:09Z

core/src/main/java/software/amazon/awssdk/core/util/UserAgentUtils.java

@@ -72,7 +72,7 @@ static String userAgent() {

        ua = ua
                .replace("{platform}", "java")
-                .replace("{version}", VersionInfo.SDK_VERSION)
+                .replace("{version}", "MOCK")//VersionInfo.SDK_VERSION)


FIXME: had a compile error here, remove in case we'd intend to merge this PR

If you run a mvn generate-sources -pl :core it should generate the VersionInfo class for you and add it to the generated-sources directory.

ktoso · 2018-01-31T13:54:52Z

core/src/test/java/software/amazon/awssdk/core/async/FileAsyncRequestPublisherTckTest.java

+    public Publisher<ByteBuffer> createFailedPublisher() {
+        return AsyncRequestProvider.fromFile(fileDoesNotExist.toPath());
+    }
+}


simple usage of the TCK. If you want me to submit a complete PR for these things, I'd suggest we use JimFS here, so we can avoid real IO in the testsuite. Would it be ok to include it in the test dependencies? https://github.com/google/jimfs

We're using jimfs for testing such Publishers etc in Akka and it's been pretty reliable.

Shouldn't have any problems using this from a licensing perspective (it's Apache 2.0) - and I'd be OK adding as long as it's only in the test scope.

Excellent, thanks for confirming

kiiadi · 2018-01-31T23:56:21Z

@ktoso I like the idea of including tests that validate that we are in compliance with the reactive spec. When you say slow - what are we talking about here? ideally unit tests would be fast and not perform I/O - if this is the case I'd say they could run as part of the regular mvn install lifecycle. However if they are slow then we should put them in the it source folder and have them run as integration tests.

kiiadi · 2018-01-31T23:57:27Z

core/pom.xml

+        <dependency>
+            <groupId>org.reactivestreams</groupId>
+            <artifactId>reactive-streams</artifactId>
+            <version>${reactive-streams.version}</version>


It's interesting that you had to explicitly pull this in - was this required or was it just a best practise thing?

Mostly just since I wanted to be on 1.0.2 and I think the Netty adapter library is on previous version. I will double check. It is backwards compatible, so we can use a newer version here (both semantically and binary)

kiiadi · 2018-01-31T23:58:38Z

core/src/main/java/software/amazon/awssdk/core/util/UserAgentUtils.java

@@ -72,7 +72,7 @@ static String userAgent() {

        ua = ua
                .replace("{platform}", "java")
-                .replace("{version}", VersionInfo.SDK_VERSION)
+                .replace("{version}", "MOCK")//VersionInfo.SDK_VERSION)


If you run a mvn generate-sources -pl :core it should generate the VersionInfo class for you and add it to the generated-sources directory.

ktoso · 2018-02-03T03:42:32Z

Got a long inter continental flight coming up now so I’ll work on this a bit more in flight I think.

ktoso · 2018-02-03T07:43:24Z

When you say slow - what are we talking about here? ideally unit tests would be fast and not perform I/O - if this is the case I'd say they could run as part of the regular mvn install lifecycle.

I'd say "tens of seconds". We can avoid real IO thanks to Jimfs, so I'll do that and see how long things take. Will move them to it if indeed still slow -- likely a good idea anyway. The reason they're slow is mostly that they have many checks in the form of "wait 100ms hoping that no more signals happen" etc. (checking that no stray, or "late" onNext signals are sent, after stream completion etc)

ktoso · 2018-02-03T19:09:32Z

Worked a bit on this during my flight; not complete yet, more cleanups/fixes to follow.

joost-de-vries · 2018-04-13T12:13:41Z

@ktoso I made the FileAsyncRequestPublisherTckTest pass. (Apart from required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue since creating a file with int max_value blocks is not an option)
And fixed the merge conflicts. You can find my fork here.
@kiiadi Is this sufficient for merging?

dagnir · 2018-05-30T17:54:31Z

@ktoso LGTM! Thank you for helping us get started with this! Happy to merge this in! If you would, could you update you rebase your branch against the latest tip of master?

dagnir · 2018-06-12T22:13:25Z

Closing this since the work here was incorporated into #519.

…f0fc7276 Pull request: release <- staging/4bbfb7ac-0e2a-494d-bf52-2876f0fc7276

PR for discussion, showing Reactive Streams TCK, proposing some

7a8b91b

solutions

ktoso commented Jan 31, 2018

View reviewed changes

include sample content in test file, FIXME: these should use a virtual

c1c8df7

filesystem

ktoso commented Jan 31, 2018

View reviewed changes

kiiadi reviewed Jan 31, 2018

View reviewed changes

ktoso added 2 commits February 3, 2018 20:01

SimpleSubscriber is now spec compliant

b2e5d0f

SingleByteArrayAsyncRequestProvider is now compliant

92221d0

joost-de-vries mentioned this pull request Apr 6, 2018

Update to AWS SDK 2.0 akka/alpakka#372

Closed

dagnir mentioned this pull request Jun 4, 2018

Add TCK tests for reactive streams implementations #519

Merged

12 tasks

dagnir closed this Jun 12, 2018

aws-sdk-java-automation pushed a commit that referenced this pull request Feb 8, 2019

Merge pull request #407 from aws/staging/4bbfb7ac-0e2a-494d-bf52-2876…

50746d0

…f0fc7276 Pull request: release <- staging/4bbfb7ac-0e2a-494d-bf52-2876f0fc7276

[PR for discussion] Showing Reactive Streams TCK, proposing some #407

[PR for discussion] Showing Reactive Streams TCK, proposing some #407

Uh oh!

Conversation

ktoso commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiiadi commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso commented Feb 3, 2018

Uh oh!

ktoso commented Feb 3, 2018

Uh oh!

ktoso commented Feb 3, 2018

Uh oh!

joost-de-vries commented Apr 13, 2018

Uh oh!

dagnir commented May 30, 2018

Uh oh!

dagnir commented Jun 12, 2018

Uh oh!

Uh oh!