-
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
1.x: Fix multiple values produced by throttleFirst with TestScheduler #4397
1.x: Fix multiple values produced by throttleFirst with TestScheduler #4397
Conversation
@@ -172,4 +174,44 @@ public void timed() { | |||
ts.assertNoErrors(); | |||
ts.assertCompleted(); | |||
} | |||
|
|||
@Test | |||
public void testThrottleWithoutAdvancingTimeOfTestScheduler() { |
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.
nit: no need for test prefix
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.
Done
When throttleFirst was operating on a TestScheduler, it delivered all items passed to it untill TestScheduler's time would change to a non-zero value.
36cf057
to
d6014ec
Compare
Current coverage is 84.29% (diff: 100%)@@ 1.x #4397 diff @@
==========================================
Files 270 270
Lines 17515 17516 +1
Methods 0 0
Messages 0 0
Branches 2677 2677
==========================================
- Hits 14770 14765 -5
- Misses 1885 1897 +12
+ Partials 860 854 -6
|
👍 and thanks for the fix. |
} | ||
|
||
@Test | ||
public void testThrottleWithTestSchedulerTimeOfZero() { |
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.
Not sure if this test is required
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.
The intention was not to rely implicitly on the fact that TestScheduler's advanceTimeBy(0) is a no-op.
I don't think it's absolutely necessary, though.
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.
Ok, that's reasonable
On Sun, 21 Aug 2016, 22:04 Anton Rutkevich, [email protected]
wrote:
In src/test/java/rx/internal/operators/OperatorThrottleFirstTest.java
#4397 (comment):
PublishSubject<Integer> o = PublishSubject.create();
o.throttleFirst(500, TimeUnit.MILLISECONDS, s).subscribe(observer);
// send events without calling advanceTimeBy/To
o.onNext(1); // deliver
o.onNext(2); // skip
o.onNext(3); // skip
o.onCompleted();
verify(observer).onNext(1);
verify(observer).onCompleted();
verifyNoMoreInteractions(observer);
- }
- @test
- public void testThrottleWithTestSchedulerTimeOfZero() {
The intention was not to rely implicitly on the fact that TestScheduler's
advanceTimeBy(0) is a no-op.
I don't think it's absolutely necessary, though.—
You are receiving this because you commented.Reply to this email directly, view it on GitHub
https://github.com/ReactiveX/RxJava/pull/4397/files/36cf057a7eaea1e013eb320ccbb644841fa75f77#r75601379,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA7B3KW6ptfbQe-Y1WMl8cfndGK8ZPlMks5qiKEsgaJpZM4JpUFZ
.
When throttleFirst was operating on a TestScheduler, it delivered all items passed to it untill TestScheduler's time would change to a non-zero value.