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

2.x: fix assertValueSequence reversed error message #5594

Merged
merged 3 commits into from Sep 12, 2017

Conversation

akarnokd
Copy link
Member

The error message/variable names in TestBaseConsumer.assertValueSequence was reversed: if more items were expected, it said "Fewer" and vice versa.

@codecov
Copy link

codecov bot commented Sep 10, 2017

Codecov Report

Merging #5594 into 2.x will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5594      +/-   ##
============================================
- Coverage     96.23%   96.22%   -0.02%     
- Complexity     5826     5827       +1     
============================================
  Files           632      632              
  Lines         41467    41467              
  Branches       5745     5745              
============================================
- Hits          39906    39900       -6     
- Misses          613      620       +7     
+ Partials        948      947       -1
Impacted Files Coverage Δ Complexity Δ
.../java/io/reactivex/observers/BaseTestConsumer.java 99.65% <100%> (ø) 113 <0> (ø) ⬇️
...ernal/operators/flowable/FlowableFlatMapMaybe.java 91.3% <0%> (-5.32%) 2% <0%> (ø)
.../io/reactivex/internal/functions/ObjectHelper.java 94.73% <0%> (-5.27%) 20% <0%> (-1%)
...ava/io/reactivex/processors/BehaviorProcessor.java 88.49% <0%> (-4.87%) 62% <0%> (ø)
.../internal/operators/flowable/FlowableInterval.java 94.44% <0%> (-2.78%) 3% <0%> (ø)
...tivex/internal/schedulers/TrampolineScheduler.java 92.3% <0%> (-2.57%) 6% <0%> (ø)
...ex/internal/subscribers/InnerQueuedSubscriber.java 94.11% <0%> (-1.97%) 17% <0%> (-1%)
.../io/reactivex/disposables/CompositeDisposable.java 97.24% <0%> (-1.84%) 39% <0%> (-1%)
...nal/operators/flowable/FlowableConcatMapEager.java 97.1% <0%> (-1.16%) 2% <0%> (ø)
...ivex/internal/operators/maybe/MaybeMergeArray.java 96.62% <0%> (-1.13%) 6% <0%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b037c7...33be81d. Read the comment docs.

@@ -958,13 +958,15 @@ public void assertValueSequence() {
throw new RuntimeException("Should have thrown");
} catch (AssertionError ex) {
// expected
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's remove this then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I say let's keep it to indicate catching te AssertionError is not an accident.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually that's done by naming exception as expected:

try {
  // …
  fail();
} catch (AssertionError expected) {
  // Assert exception message, etc.
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine.

Copy link
Contributor

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

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

LGTM, but one more test pls

@@ -958,13 +958,15 @@ public void assertValueSequence() {
throw new RuntimeException("Should have thrown");
} catch (AssertionError ex) {
// expected
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually that's done by naming exception as expected:

try {
  // …
  fail();
} catch (AssertionError expected) {
  // Assert exception message, etc.
}


if (!ObjectHelper.equals(u, v)) {
throw fail("Values at position " + i + " differ; Expected: " + valueAndClass(u) + ", Actual: " + valueAndClass(v));
throw fail("Values at position " + i + " differ; Expected: " + valueAndClass(v) + ", Actual: " + valueAndClass(u));
Copy link
Contributor

Choose a reason for hiding this comment

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

This change also needs a test

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no behavior change here, just a swapped variable name. I'll restore the original u - expected, v - actual naming

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Well at least we have a valid reason to start working on RxJava 3 — kill these one-letter variable names :trollface:

@akarnokd akarnokd merged commit 797301b into ReactiveX:2.x Sep 12, 2017
@akarnokd akarnokd deleted the AssertValueSequenceFix branch September 12, 2017 11:28
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.

None yet

3 participants