Skip to content

Commit

Permalink
Fix take and takeLast (#5326)
Browse files Browse the repository at this point in the history 
* fix(take): add runtime argument checks

BREAKING CHANGE: take will not throw runtime error for arguments that are negative or NaN, this includes non-TS calls like `take()`.

* chore: update takeLast tests

* fix(takeLast): add runtime assertions for invalid arguments

BREAKING CHANGE: Calling takeLast without arguments or with an argument that is NaN will throw a TypeError
  • Loading branch information
@benlesh
benlesh authored May 14, 2020
1 parent 60fa798 commit 5efc474
Show file tree
Hide file tree
Showing 4 changed files with 195 additions and 133 deletions.
25 changes: 22 additions & 3 deletions spec/operators/take-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import { range, ArgumentOutOfRangeError, of, Observable, Subject } from 'rxjs';
import { TestScheduler } from 'rxjs/testing';
import { observableMatcher } from '../helpers/observableMatcher';

declare function asDiagram(arg: string): Function;

/** @test {take} */
describe('take operator', () => {
let testScheduler: TestScheduler;
Expand All @@ -14,7 +12,28 @@ describe('take operator', () => {
testScheduler = new TestScheduler(observableMatcher);
});

asDiagram('take(2)')('should take two values of an observable with many values', () => {
it('should error when a non-number is passed to it, or when no argument is passed (Non-TS case)', () => {
expect(() => {
of(1, 2, 3).pipe(
(take as any)()
);
}).to.throw(TypeError, `'count' is not a number`);

expect(() => {
of(1, 2, 3).pipe(
(take as any)('banana')
);
}).to.throw(TypeError, `'count' is not a number`);

// Standard type coersion behavior in JS.
expect(() => {
of(1, 2, 3).pipe(
(take as any)('1')
);
}).not.to.throw();
});

it('should take two values of an observable with many values', () => {
testScheduler.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold(' --a-----b----c---d--|');
const e1subs = ' ^-------!------------';
Expand Down
238 changes: 145 additions & 93 deletions spec/operators/takeLast-spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,164 +1,216 @@
import { expect } from 'chai';
import { hot, cold, expectObservable, expectSubscriptions } from '../helpers/marble-testing';
import { takeLast, mergeMap } from 'rxjs/operators';
import { range, ArgumentOutOfRangeError, of } from 'rxjs';

declare function asDiagram(arg: string): Function;
import { TestScheduler } from 'rxjs/testing';
import { assertDeepEquals } from '../helpers/test-helper';

/** @test {takeLast} */
describe('takeLast operator', () => {
asDiagram('takeLast(2)')('should take two values of an observable with many values', () => {
const e1 = cold('--a-----b----c---d--| ');
const e1subs = '^ ! ';
const expected = '--------------------(cd|)';
let rxTest: TestScheduler;

beforeEach(() => {
rxTest = new TestScheduler(assertDeepEquals);
});

it('should error for invalid arguments', () => {
expect(() => {
of(1, 2, 3).pipe((takeLast as any)());
}).to.throw(TypeError, `'count' is not a number`);

expect(() => {
of(1, 2, 3).pipe((takeLast as any)('banana'));
}).to.throw(TypeError, `'count' is not a number`);

expect(() => {
of(1, 2, 3).pipe((takeLast as any)('3'));
}).not.to.throw();
});

it('should take two values of an observable with many values', () => {
rxTest.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold('--a-----b----c---d--| ');
const e1subs = ' ^-------------------! ';
const expected = '--------------------(cd|)';

expectObservable(e1.pipe(takeLast(2))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(2))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should take last three values', () => {
const e1 = cold('--a-----b----c---d--| ');
const e1subs = '^ ! ';
const expected = '--------------------(bcd|)';
rxTest.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold(' --a-----b----c---d--| ');
const e1subs = ' ^-------------------! ';
const expected = '--------------------(bcd|)';

expectObservable(e1.pipe(takeLast(3))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(3))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should take all element when try to take larger then source', () => {
const e1 = cold('--a-----b----c---d--| ');
const e1subs = '^ ! ';
const expected = '--------------------(abcd|)';
rxTest.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold(' --a-----b----c---d--| ');
const e1subs = ' ^-------------------! ';
const expected = '--------------------(abcd|)';

expectObservable(e1.pipe(takeLast(5))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(5))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should take all element when try to take exact', () => {
const e1 = cold('--a-----b----c---d--| ');
const e1subs = '^ ! ';
const expected = '--------------------(abcd|)';
rxTest.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold(' --a-----b----c---d--| ');
const e1subs = ' ^-------------------! ';
const expected = '--------------------(abcd|)';

expectObservable(e1.pipe(takeLast(4))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(4))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should not take any values', () => {
const e1 = cold('--a-----b----c---d--|');
const expected = '|';
rxTest.run(({ cold, expectObservable }) => {
const e1 = cold(' --a-----b----c---d--|');
const expected = '|';

expectObservable(e1.pipe(takeLast(0))).toBe(expected);
expectObservable(e1.pipe(takeLast(0))).toBe(expected);
});
});

it('should work with empty', () => {
const e1 = cold('|');
const e1subs = '(^!)';
const expected = '|';
rxTest.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold(' |');
const e1subs = ' (^!)';
const expected = '|';

expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should go on forever on never', () => {
const e1 = cold('-');
const e1subs = '^';
const expected = '-';
rxTest.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold(' -');
const e1subs = ' ^';
const expected = '-';

expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should be empty on takeLast(0)', () => {
const e1 = hot('--a--^--b----c---d--|');
const e1subs: string[] = []; // Don't subscribe at all
const expected = '|';
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot('--a--^--b----c---d--|');
const expected = ' |';
const e1subs: string[] = []; // Don't subscribe at all

expectObservable(e1.pipe(takeLast(0))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(0))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should take one value from an observable with one value', () => {
const e1 = hot('---(a|)');
const e1subs = '^ ! ';
const expected = '---(a|)';
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot(' ---(a|)');
const e1subs = ' ^--! ';
const expected = '---(a|)';

expectObservable(e1.pipe(takeLast(1))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(1))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should take one value from an observable with many values', () => {
const e1 = hot('--a--^--b----c---d--| ');
const e1subs = '^ ! ';
const expected = '---------------(d|)';
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot('--a--^--b----c---d--| ');
const e1subs = ' ^--------------! ';
const expected = ' ---------------(d|)';

expectObservable(e1.pipe(takeLast(1))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(1))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should error on empty', () => {
const e1 = hot('--a--^----|');
const e1subs = '^ !';
const expected = '-----|';
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot('--a--^----|');
const e1subs = ' ^----!';
const expected = ' -----|';

expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should propagate error from the source observable', () => {
const e1 = hot('---^---#', undefined, 'too bad');
const e1subs = '^ !';
const expected = '----#';
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot('---^---#', undefined, 'too bad');
const e1subs = ' ^---!';
const expected = ' ----#';

expectObservable(e1.pipe(takeLast(42))).toBe(expected, null, 'too bad');
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(42))).toBe(expected, null, 'too bad');
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should propagate error from an observable with values', () => {
const e1 = hot('---^--a--b--#');
const e1subs = '^ !';
const expected = '---------#';
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot('---^--a--b--#');
const e1subs = ' ^--------!';
const expected = ' ---------#';

expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should allow unsubscribing explicitly and early', () => {
const e1 = hot('---^--a--b-----c--d--e--|');
const unsub = ' ! ';
const e1subs = '^ ! ';
const expected = '---------- ';
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot('---^--a--b-----c--d--e--|');
const unsub = ' ---------! ';
const e1subs = ' ^--------! ';
const expected = ' ----------------------';

expectObservable(e1.pipe(takeLast(42)), unsub).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(42)), unsub).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should work with throw', () => {
const e1 = cold('#');
const e1subs = '(^!)';
const expected = '#';
rxTest.run(({ cold, expectObservable, expectSubscriptions }) => {
const e1 = cold(' #');
const e1subs = ' (^!)';
const expected = '#';

expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
expectObservable(e1.pipe(takeLast(42))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});

it('should throw if total is less than zero', () => {
expect(() => { range(0, 10).pipe(takeLast(-1)); })
.to.throw(ArgumentOutOfRangeError);
expect(() => {
range(0, 10).pipe(takeLast(-1));
}).to.throw(ArgumentOutOfRangeError);
});

it('should not break unsubscription chain when unsubscribed explicitly', () => {
const e1 = hot('---^--a--b-----c--d--e--|');
const unsub = ' ! ';
const e1subs = '^ ! ';
const expected = '---------- ';

const result = e1.pipe(
mergeMap((x: string) => of(x)),
takeLast(42),
mergeMap((x: string) => of(x))
);

expectObservable(result, unsub).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
rxTest.run(({ hot, expectObservable, expectSubscriptions }) => {
const e1 = hot('---^--a--b-----c--d--e--|');
const unsub = ' ---------! ';
const e1subs = ' ^--------! ';
const expected = ' ----------------------';

const result = e1.pipe(
mergeMap((x: string) => of(x)),
takeLast(42),
mergeMap((x: string) => of(x))
);

expectObservable(result, unsub).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});
});
});
Loading

0 comments on commit 5efc474

Please sign in to comment.