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

fix(throttleTime): emit single value with trailing enabled #4564

Conversation

MatthiasKunnen
Copy link
Contributor

throttleTime didn't emit single values when leading was disabled and trailing enabled.

e.g.:

source:   -a--------------------|
time:     ----|                  
previous: ----------------------|
current:  -----a----------------|

I've added tests to verify this behavior and added another commit to fix it.

Closes #2859 and #4491.

perhaps irrelevant
I have another branch ready to fix some other problems related to throttleTime which I will create a PR for when this one is merged.

Test if throttleTime emits when only a single value is emitted from the
source with leading enabled/disabled and trailing enabled.
…nabled

Emit single value with leading disabled, trailing enabled.

Closes ReactiveX#2859 and ReactiveX#4491.
@MatthiasKunnen MatthiasKunnen changed the title fix (throttleTime): trailing single value fix(throttleTime): emitted single value with trailing enabled Feb 14, 2019
@coveralls
Copy link

coveralls commented Feb 14, 2019

Pull Request Test Coverage Report for Build 8355

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 32 unchanged lines in 10 files lost coverage.
  • Overall coverage decreased (-0.2%) to 96.652%

Files with Coverage Reduction New Missed Lines %
src/internal/scheduler/VirtualTimeScheduler.ts 1 83.52%
src/internal/operators/partition.ts 1 75.0%
src/internal/observable/combineLatest.ts 1 96.34%
src/internal/operators/throwIfEmpty.ts 1 93.94%
src/internal/observable/from.ts 1 84.09%
src/internal/OuterSubscriber.ts 1 50.0%
src/internal/observable/race.ts 1 95.0%
src/internal/Observable.ts 3 91.67%
src/internal/util/not.ts 4 20.0%
src/internal/observable/ConnectableObservable.ts 18 55.46%
Totals Coverage Status
Change from base Build 8066: -0.2%
Covered Lines: 5254
Relevant Lines: 5436

💛 - Coveralls

@MatthiasKunnen MatthiasKunnen changed the title fix(throttleTime): emitted single value with trailing enabled fix(throttleTime): emit single value with trailing enabled Feb 14, 2019
@MatthiasKunnen
Copy link
Contributor Author

@cartant, I'm notifying you because I am a bit desperate for a review and you commented on #4491 of which this PR is a replacement.

I'm hoping you don't mind the notification.

@cartant cartant self-requested a review April 5, 2019 16:23
@cartant
Copy link
Collaborator

cartant commented Apr 13, 2019

I've reviewed this, but I'd like to make some changes to the test descriptions - some of which are separate to the changes you've made in this PR. I'm unable to push make changes to your PR, so you can either ensure that this box is checked:

Allow

Or you can apply the following patch that includes the changes:

From 43005d331227b879591ee197704a7c5089ce0d54 Mon Sep 17 00:00:00 2001
From: Nicholas Jamieson <[email protected]>
Date: Sat, 13 Apr 2019 15:11:47 +1000
Subject: [PATCH] chore: improve test descriptions

And remove an asDiagram call made with the same argument.
---
 spec/operators/throttleTime-spec.ts | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/spec/operators/throttleTime-spec.ts b/spec/operators/throttleTime-spec.ts
index bbaab515..4e65054a 100644
--- a/spec/operators/throttleTime-spec.ts
+++ b/spec/operators/throttleTime-spec.ts
@@ -141,10 +141,10 @@ describe('throttleTime operator', () => {
   });
 
   describe('throttleTime(fn, { leading: true, trailing: true })', () => {
-    asDiagram('throttleTime(fn, { leading: true, trailing: true })')('should immediately emit the first value in each time window', () =>  {
+    asDiagram('throttleTime(fn, { leading: true, trailing: true })')('should immediately emit the first and last values in each time window', () =>  {
       const e1 =   hot('-a-xy-----b--x--cxxx--|');
       const e1subs =   '^                     !';
-      const t =  time( '----|                 ');
+      const t =  time( '----|                  ');
       const expected = '-a---y----b---x-c---x-|';
 
       const result = e1.pipe(throttleTime(t, rxTestScheduler, { leading: true, trailing: true }));
@@ -153,7 +153,7 @@ describe('throttleTime operator', () => {
       expectSubscriptions(e1.subscriptions).toBe(e1subs);
     });
 
-    it('should emit the first value if only a single one is given', () => {
+    it('should emit the value if only a single one is given', () => {
       const e1 =   hot('-a--------------------|');
       const t =   time('----|                  ');
       const expected = '-a--------------------|';
@@ -165,10 +165,10 @@ describe('throttleTime operator', () => {
   });
 
   describe('throttleTime(fn, { leading: false, trailing: true })', () => {
-    asDiagram('throttleTime(fn, { leading: false, trailing: true })')('should immediately emit the first value in each time window', () =>  {
+    asDiagram('throttleTime(fn, { leading: false, trailing: true })')('should immediately emit the last value in each time window', () =>  {
       const e1 =   hot('-a-xy-----b--x--cxxx--|');
       const e1subs =   '^                     !';
-      const t =  time( '----|                 ');
+      const t =  time( '----|                  ');
       const expected = '-----y--------x-----x-|';
 
       const result = e1.pipe(throttleTime(t, rxTestScheduler, { leading: false, trailing: true }));
@@ -177,10 +177,10 @@ describe('throttleTime operator', () => {
       expectSubscriptions(e1.subscriptions).toBe(e1subs);
     });
 
-    asDiagram('throttleTime(fn, { leading: false, trailing: true })')('should emit the last throttled value when complete', () => {
+    it('should emit the last throttled value when complete', () => {
       const e1 =   hot('-a-xy-----b--x--cxx|');
       const e1subs =   '^                  !';
-      const t =   time('----|              ');
+      const t =   time('----|               ');
       const expected = '-----y--------x----(x|)';
 
       const result = e1.pipe(throttleTime(t, rxTestScheduler, { leading: false, trailing: true }));
@@ -189,7 +189,7 @@ describe('throttleTime operator', () => {
       expectSubscriptions(e1.subscriptions).toBe(e1subs);
     });
 
-    it('should emit the first value if only a single one is given', () => {
+    it('should emit the value if only a single one is given', () => {
       const e1 =   hot('-a--------------------|');
       const t =   time('----|                  ');
       const expected = '-----a----------------|';
-- 
2.17.1.windows.2


And remove an asDiagram call made with the same argument.

Signed-off-by: Matthias Kunnen <[email protected]>
@MatthiasKunnen
Copy link
Contributor Author

Thank you for the review, I've applied the patch.

@benlesh benlesh merged commit fd690a6 into ReactiveX:master Apr 23, 2019
@MatthiasKunnen
Copy link
Contributor Author

Thank you for the review and merge @benlesh. I'll create another PR addressing other issues with throttleTime later today.

@MatthiasKunnen
Copy link
Contributor Author

@benlesh and @cartant, the follow-up PR is #4727.

BioPhoton pushed a commit to BioPhoton/rxjs that referenced this pull request May 15, 2019
…#4564)

* test(throttleTime): test single value with trailing enabled

Test if throttleTime emits when only a single value is emitted from the
source with leading enabled/disabled and trailing enabled.

* fix(throttleTime): fix single value with leading disabled, trailing enabled

Emit single value with leading disabled, trailing enabled.

Closes ReactiveX#2859 and ReactiveX#4491.

* chore: improve test descriptions

And remove an asDiagram call made with the same argument.

Signed-off-by: Matthias Kunnen <[email protected]>
@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

throttle and throttleTime broken for single values when trailing is true.
4 participants