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

TCK Upgrade Story #99

Closed
ktoso opened this issue Aug 21, 2014 · 9 comments · Fixed by #246
Closed

TCK Upgrade Story #99

ktoso opened this issue Aug 21, 2014 · 9 comments · Fixed by #246

Comments

@ktoso
Copy link
Contributor

ktoso commented Aug 21, 2014

This issue came up during discussions on the "bring back the TCK" pull request: #91

In general we need some story about when we change the spec, and the tests, that implementors will be notified about some changed (perhaps optional!) behaviour. In the TCK tests which are optional are skipped if they fail to verify currently, this could cause adding optional behaviour in 0.x+1 simply be skipped to be verified, and the implementor may assume "oh good, my impl covers everything". This is not really limited to adding optional tests, but in general behaviour which we can't test for example too.

This issue means to start a discussion about if we want to provide more info for implementors in the TCK itself or simply assume implementors will read changelogs and apply all changes.

One idea on the more extreme side would be that each spec would contain a value conformsTo() { return Spec_0_4; }), then if the dependency is updated to 0.5, we fail the tests listing what has changed... This might be overkill, and simply "please read the change logs" could be our solution here.

I'm interested if you guys see this as a problem that needs solving or we simply document that "if you upgrade, please do read the changelog in detail".

@viktorklang
Copy link
Contributor

Perhaps we need to make sure that the tests based on the TCK doesn't compile if it is a breaking change in the TCK that needs to be addressed?

@ktoso
Copy link
Contributor Author

ktoso commented Dec 20, 2014

I wouldn't go as far as "not compile", we should simply fail the entire suite then with an error explaining the changes etc. See my original proposal of a conformsTo method, story would be:

  • X implemented 1.0
  • 1.1 contains "important change"
  • In TCK: we add a method SpecVersion conformsTo() { return null; }
  • X updates dependency to 1.1, doesn't change any sources - we see SpecVersion being null - we can notify "you're not on 1.1, and it has this and that important change"
  • X fixes impl, makes it return conformsTo() { return SpecVersion.v1_1; }

For further upgrades it would be like so:

  • 1.2 includes such changes again,
  • we make TCK check that it requires a v1_2 to be returned,
  • X upgrades dependency, does not change sources, gets notified with "between 1.1 and 1.2 we made important changes" - an error which fails the entire test suite.
  • X reads / updates / passes 1.2 test suite.

How about such story? The idea would be to make the update errors really useful. Whereas your proposal would, well, make it not compile, but wouldn't include much more hints "where to look for the changelog etc".

@viktorklang
Copy link
Contributor

@ktoso I think it sounds interesting, the question is how much time it'd take to get something in place.

@ktoso
Copy link
Contributor Author

ktoso commented Dec 21, 2014

So the hardest part is actually maintaining these while prepping a release.
The how-hard it is, depends on how much info we want to include. If we make sure to properly tag releases and put issues into github milestones, it could be just a link to a github milestone. We have not been doing this so far (pre 1.0), so it's more of a question how we want to deal with releases in general in the future (yes, even though we assume there won't be much need to release often).

I mean, I can easily code up what I've outlined above, the question is on broader feedback on the idea and how we want to tackle releases in the post 1.0 era of the project. To me it sounds these version bumps should be all together in the "bumping version" commit on each release - which is someone from contributors (so not me), so we need feedback from others on this one..

@rkuhn
Copy link
Member

rkuhn commented Jan 16, 2015

I’m not convinced that technical measure can fix the problem: patching a string in the source to say “1.2” and then running the verification successfully will always be possible. We need to trust implementors to watch the spec evolve and adapt accordingly. If an unimplemented spec update leads to runtime failures later then we should by definition be in a position to write a test that provokes those, but I estimate that the transition from “everybody does their best” to “the TCK covers everything” will take quite a while.

So, all in all I’m proposing to close this ticket.

@viktorklang
Copy link
Contributor

@rkuhn I would even go as far as say that I hope we never need to ship anything beyond 1.0.0.final :)

@ktoso
Copy link
Contributor Author

ktoso commented Mar 18, 2015

Just a bump here, the TCK readme contains a section on this which is a TODO (been looking and cleaning it up recently). If we want to write something in that section explicitly it should be before 1.0 I think.

Now we're using methods with required_ prefixes etc, so just deprecating / removing removing methods should be reasonable and good enough I feel, opinions?

@viktorklang
Copy link
Contributor

Open a PR :)

@ktoso
Copy link
Contributor Author

ktoso commented Mar 21, 2015

PR here: #246 (rebased and updated)

ktoso added a commit to ktoso/reactive-streams that referenced this issue Mar 25, 2015
akarnokd added a commit to akarnokd/reactive-streams that referenced this issue Nov 3, 2017
* Repairs formatting issue of tables in spec README

* Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException` be thrown.

Updates the TCK, Spec and example implementations.

* Fixes reactive-streams#211 by clarifying

* Fixes reactive-streams#210 by removing 1.12 and repurposing its TCK checks for 1.09

* Clarifies the signalling sequence in the spec and
 adds TCK verification to ensure signal ordering is proper,
 also amends the examples to reflect the spec change.

* Publish 1.0.0.RC2
fix reactive-streams#215

* Fixes reactive-streams#217 by including the examples project in the publish task

* =tck minor test name fixup, it is a required test

* fix reactive-streams#212 issue on spec 213 testing wrt Processor

*  RC3 release /w reactive-streams#222 fix

* remove rule 1:12 (produce same elements to all Subscribers)

This rule is in conflict with 1:11 which allows a Publisher to treat
multiple Subscribers as either as unicast or multicast recipients. The
verification of proper multicast behavior (which 1:12 specified) has
been retained, the test methods renamed accordingly.

* fix three left-over references to deleted rule 1:12

* Fixed wrong footnote reference in README.md

* Addresses a couple of typos in the examples for AsyncSubscriber and SyncSubscriber

* !TCK clarify what error publisher is
+ add better readme on what this method is
+ add better javadoc on this method
- removes reference to old style spec annotation from readme
+ proposing to change method name to "createFailed..." as it is the
  wording used in the spec and reactive manifesto (footnote 1.1)
+ more info in tck/README that it is not legal to signal on* before sub
Resolves reactive-streams#237, reactive-streams#235

* +tck reactive-streams#236 example subscriber whitebox tested, and whitebox fixed

* add space to javadoc

* +TCK verifyNoAsyncErrors now by default waits, fixes spec111
Resolves reactive-streams#239

* =tck general tck/readme.md cleanup so it matches current code / spec
Resolves reactive-streams#99
Depends on reactive-streams#241

* Addresses PR review comments for reactive-streams#246

* Update CopyrightWaivers.txt

* +tck explains createElement in more useful terms

resolves reactive-streams#231

* +tck reactive-streams#232 explain which tests are mendatory to be "compliant"

* Update SubscriberWhiteboxVerification.java

Fixes Javadoc generation on Java8+ by having to manually qualify nested classes.

* Fixes reactive-streams#233 by implementing support for triggered demand in in the SubscriberBlackboxVerification

* Travis PR validation using both JDK 6 and 8

By validating on both JDKs we know the project even builds on 8,
while not using features (classes) from JDK8 - so it's still usable for JDK6 projects.

Resolves reactive-streams#254

* Small touchups to the TCK README.md

* Release 1.0.0.RC4

* Cancel the subscription after receiving all of the pertinent emissions (reactive-streams#259).

* Test that 'required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue' completes in a timely manner for fully synchronous publishers (reactive-streams#259).

* =tck untested spec308 rule method name adjusted

* -tck rm undocumented and unused publisherReferenceGCTimeoutMillis method

* update version to 1.0.0.RC5

* Updating documentation to reflect the current version: RC5

* update ref to 1.0.0.final

* change 1.0.0.final to 1.0.0 and make sure OSGI manifest has the bundle version

*  OSGI fix

*  OSGI fix...

* Disambiguate "processing elements"

The document generally refers to "elements" as objects traversing a stream. I initially considered simply editing "processing elements" to read "processing components", but there's a section devoted to the definition of this, so better to link them.

* Added per request of @viktorklang in reactive-streams#269

* add CC0 label to README

* =tck reactive-streams#279 improve completion latch error message

* Rename SyncSybscriber.foreach to whenNext

* Update README.md

Spelling of the company name is Red Hat, not RedHat.

* I hereby represent [...] public domain [...] entirety of my contributions.

Requested by @viktorklang.

* Log test output events to the console

* Remove "preview" qualifier from README.

* Unbreaks TravisCI OpenJDK6 hostname too long crash

* Second attempt at unbreaking the Travis build

* Third attempt at fixing the Travis builds

* +tck reactive-streams#308 allow configuring "no events during N time" separately

* Update to Gradle 2.12

* Reintegrate dangling footnote in Publisher section.

- integrate the footnote in rule 1.9
- sign the Copyright Statement

* Asynchronous vs Synchronous Processing: reword "push-based stream"

* =tck fixes minor misalignment between code and comment, found via .NET port

Semantics remain exacly the same, the error we're testing here is about
signaling one more element if request comes in again (which we'll do
anyway, regardless of status of this flag)

* adjust Subscription.cancel javadoc because cancel command does not have to be called asynchronously

* Updating Typesafe to Lightbend

* Fix a typo in org.reactivestreams.example.unicast.AsyncSubscriber

* Add @seratch to CopyrightWaivers.txt

* Fixes reactive-streams#333 by adding license headers to /examples/*

* Adds a Glossary, Intent-sections and harmonizes verbiage

* Clarifying that object equality is a.equals(b) in Intent for 2.12

* add license header to API directory

* add license header to TCK

* Fix missing cancel() from in tests that don't consume the entire source

* Run with default TestEnvironment settings.

* Update CopyrightWaivers.txt

* =build reactive-streams#349 equal osgi manifest version as real version

To have a tangible PR to talk about.
Probably enough to resolve reactive-streams#349

Would be followed up with change to 1.0.1 eventually.

* Add Javadoc explanation to the TCK test methods about what they do

* Don't import org.reactivestreams.tck.TestEnvironment

* Fix missing Javadoc tags

* TCK: Request -1 in 309 instead of a random non-positive number

* Remove the Random instance as well.

* Keep the randomness.

* Fixing typos in README.md

* Minor rewording of 2.6 to make it easier to understand. (reactive-streams#342)

* Minor rewording of 2.6 to make it easier to understand.

* Fix spelling errors and clarify a couple of sentences

* extra coordination

* Remove vague statements, be more specific in others

* Update javadoc based on ktoso's feedback

* Use the wording eagery for error publisher test 104

* Address feedback, add links to the rules in the javadoc

* SubscriberBlackboxVerificationRules explained

* Non-BC for TCK: Corrects a typo in test method from *Compuatation to *Computation

* Adding a glossary item for external synchronization

* Repointing links to sources in README to current main release

* =tck reactive-streams#362 signal onComplete in 201 blackbox verification

* +tck reactive-streams#362 complete subscriber under test once done in 205

* +tck reactive-streams#362 wait for request signal in 209, and new additional tests

* =tck check isCancelled in 205 blackbox; sample the state sometimes

* =tck reactive-streams#362 blackbox 209 must issue onSubscribe before any other signal

* Clarifies the meaning of "stochastic" for skipStochasticTests()

* add additional test for optional_spec111.

* now test verifies https://github.com/reactive-streams/reactive-streams-jvm#1.11 and
https://github.com/reactive-streams/reactive-streams-jvm#1.5 for publishers, if they support multiple subscribers.

* add delegate to IdentityProcessorVerification.

* add tests for optional_spec111_registeredSubscribersMustReceiveOnNextOrOnCompleteSignals.

* additional happy and the failure cases.
* clear typos and change comments.
* add new PublisherVerification for multi-subscribers tests.

* removed onSubscribe constructor call.

renamed Demand -> CancelableSubscription.

* Change subscription remove logic.

* add myself to CopyrightWaivers.

* fix tests by using proxied subscriber,
thanks Viktor for helping push this fix

* Be consistent in reference style

We use the `#.##` style in referring to rules everywhere, this one ref was using a different style - fixed that.

* Switching to consistent use of apostrophe in spec

* More apostrophe fixes

* add patriknw to CopyrightWaivers

* Version 1.0.1

* =spec reactive-streams#384 amend spec to allow not mentioning rule number in exception message

* Update README.md

* =tck reactive-streams#384 dont check for cause message when checking 3.9

* Updating versions to 1.0.1-RC2 and clarifying changes in RELEASE-NOTES.md

* Fix links to "Terminal state" (reactive-streams#389)

* Fix links to "Terminal state"

* add angelsanz to CopyrightWaivers.txt

* Preparing 1.0.1 (reactive-streams#390)

* Bridge between Reactive-Streams and JDK 9 Flow API (reactive-streams#296)

* Bridge between Reactive-Streams and JDK 9 Flow API

* Apply changes based on ktoso's feedback

* Use oraclejdk9, resolve build.gradle conflict

* Change txt/code to use "Reactive Streams" as designator

* NPE to use the updated parameter name.

* Rename bridge class, tester class (+javadoc)

* Java 9 Flow bridge: add Subscriber converters (reactive-streams#399)

* Java 9 Flow bridge: add Subscriber converters

* Fix return type javadoc

* Example synchronous range Publisher (reactive-streams#395)

* Example synchronous range Publisher

* Udpated with rule numbers in comments

* Mentioning rule 3.9 again in emit()

* Move classes to the unicast package.

* [WIP] TCK for j.u.c.Flow types "directly" (reactive-streams#398)

* Add JDK9 TCK, using adapters

* Fixing wrapping and unwrapping of the wrappers themselves.

* Renames the converters to "toX" for RS and "toFlowX" for Flow.

Fixes so that the dist url for gradle is http iso https (TravisCI bug?)

Adds regression test for bridge converters.

* fix formatting

* cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants