-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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 window() with start/end selector not disposing/cancelling properly #6398
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #6398 +/- ##
============================================
+ Coverage 98.24% 98.26% +0.01%
+ Complexity 6294 6292 -2
============================================
Files 675 675
Lines 45156 45162 +6
Branches 6239 6243 +4
============================================
+ Hits 44365 44378 +13
+ Misses 247 243 -4
+ Partials 544 541 -3
Continue to review full report at Codecov.
|
|
||
TestSubscriber<Flowable<Integer>> ts = source.window(boundary, Functions.justFunction(Flowable.never())) | ||
.test(0L) | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why on it's own line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A habit if more methods are ever chained onto them.
// FIXME subject has subscribers because of the open window | ||
assertTrue(open.hasSubscribers()); | ||
// Disposing the outer sequence stops the opening of new windows | ||
assertFalse(open.hasSubscribers()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that we can fix this too!
This PR fixes the start-end selector variant of
Observable.window
andFlowable.window
to properlyFixes: #6397