Skip to content

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

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

Merged
merged 3 commits into from
Apr 12, 2017

Conversation

akarnokd
Copy link
Contributor

@akarnokd akarnokd commented Apr 4, 2017

Make required_spec309_requestNegativeNumberMustSignalIllegalArgumentException deterministically request -1 as the negative number.

@@ -884,8 +884,7 @@ public void required_spec309_requestNegativeNumberMustSignalIllegalArgumentExcep
@Override
public void run(Publisher<T> pub) throws Throwable {
final ManualSubscriber<T> sub = env.newManualSubscriber(pub);
final Random r = new Random();
sub.request(-r.nextInt(Integer.MAX_VALUE));
sub.request(-1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

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

Choose a reason for hiding this comment

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

I meant "subtract 1 from the random value"

Copy link
Contributor

Choose a reason for hiding this comment

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

0 is also invalid tho :)

Copy link
Contributor Author

@akarnokd akarnokd Apr 4, 2017

Choose a reason for hiding this comment

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

0 is explicitly tested with required_spec309_requestZeroMustSignalIllegalArgumentException.

Restored randomness and subtracted 1.

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, good improvement; should be good to squash/merge

@smaldini smaldini added this to the 1.0.1 milestone Apr 11, 2017
@smaldini
Copy link
Contributor

if @reactive-streams/contributors agree we can merge

@viktorklang
Copy link
Contributor

Thanks @akarnokd!

@viktorklang viktorklang merged commit c2e75b2 into reactive-streams:master Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants