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.

Sign up for GitHub

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

Jump to bottom

test(shareReplay): don't restart when refCount if the source has completed #5457

Merged
merged 2 commits into from May 27, 2020
Merged

test(shareReplay): don't restart when refCount if the source has completed #5457

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce1dc54
test(shareReplay): don't restart when refCount if the source has comp…
josepot May 26, 2020
f771c8b
test(shareReplay): improve tests by checking on source subscriptions
josepot May 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions spec/operators/shareReplay-spec.ts
View file
Open in desktop
Expand Up @@ -175,6 +175,7 @@ describe('shareReplay operator', () => {

it('should not restart due to unsubscriptions if refCount is false', () => {
const source = cold('a-b-c-d-e-f-g-h-i-j');
const sourceSubs = '^------------------';
const sub1 = '^------!';
const expected1 = 'a-b-c-d-';
const sub2 = '-----------^-------';
Expand All @@ -184,10 +185,14 @@ describe('shareReplay operator', () => {

expectObservable(shared, sub1).toBe(expected1);
expectObservable(shared, sub2).toBe(expected2);
expectSubscriptions(source.subscriptions).toBe(sourceSubs);
});

it('should restart due to unsubscriptions if refCount is true', () => {
const sourceSubs = [];
const source = cold('a-b-c-d-e-f-g-h-i-j');
sourceSubs.push( '^------!----------------------');
sourceSubs.push( '-----------^------------------');
const sub1 = '^------!';
const expected1 = 'a-b-c-d-';
const sub2 = '-----------^------------------';
Expand All @@ -197,10 +202,27 @@ describe('shareReplay operator', () => {

expectObservable(shared, sub1).toBe(expected1);
expectObservable(shared, sub2).toBe(expected2);
expectSubscriptions(source.subscriptions).toBe(sourceSubs);
});

it('should not restart due to unsubscriptions if refCount is true when the source has completed', () => {
const source = cold('a-(b|) ');
const sourceSubs = '^-! ';
const sub1 = '^------! ';
const expected1 = 'a-(b|) ';
const sub2 = '-----------^! ';
const expected2 = '-----------(b|)';

const shared = source.pipe(shareReplay({ bufferSize: 1, refCount: true }));

expectObservable(shared, sub1).toBe(expected1);
expectObservable(shared, sub2).toBe(expected2);
Copy link
Collaborator

@cartant cartant May 26, 2020 •
edited

Choose a reason for hiding this comment

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

We also need to test that there is only one subscription to the source. That can be done with expectSubscriptions which takes an expected subscription marble diagram - or an array of diagrams if multiple subscriptions are expected.

Something like this:

    const source     = cold('a-(b|)         ');
    const sourceSubs =      '^-!            ';
    /* ... */
    expectSubscriptions(source.subscriptions).toBe(sourceSubs);

If multiple subscriptions to the source are made, this expectation would fail, as sourceSubs would have to be an array of subscriptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. That makes a lot of sense. I've amended that commit with this expectation, thanks!

I've also added another commit that does the same thing on other tests that were not doing that. I do understand that for those tests that expectation may not be as important, because the current expectations are "implicitly" checking for that already. So, I don't mind dropping this commit from the PR. I thought that maybe for consistency it could be worth it to have those expectations?

expectSubscriptions(source.subscriptions).toBe(sourceSubs);
});

it('should default to refCount being false', () => {
const source = cold('a-b-c-d-e-f-g-h-i-j');
const sourceSubs = '^------------------';
const sub1 = '^------!';
const expected1 = 'a-b-c-d-';
const sub2 = '-----------^-------';
Expand All @@ -210,6 +232,7 @@ describe('shareReplay operator', () => {

expectObservable(shared, sub1).toBe(expected1);
expectObservable(shared, sub2).toBe(expected2);
expectSubscriptions(source.subscriptions).toBe(sourceSubs);
});

it('should not break lift() composability', (done: MochaDone) => {
Expand Down