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

feat(TestScheduler): Add subscription schedule to expectObservable #3997

Merged
merged 1 commit into from
Aug 20, 2018

Conversation

gstamac
Copy link
Contributor

@gstamac gstamac commented Aug 8, 2018

Description:
I made this PR (#3429) already for the previous version but was rejected because of API changes. Now the new API is here and I'm making the same PR.

Related issue (if exists): #3429

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.948% when pulling 01faee6 on gstamac:master into 37c5c09 on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Aug 8, 2018

Could you please describe the use cases and motivation for the change? This isn't anything that has been considered or discussed on the team, so my immediate concern is that there isn't a need for it. Giving us concrete examples of where it's a useful feature will help us decide what we should do here.

@gstamac
Copy link
Contributor Author

gstamac commented Aug 9, 2018

An example use would be to test shareReplay.
The test

it('should multicast the same values to multiple observers, bufferSize=1', () => {
    const source =     cold('-1-2-3----4-|'); const shared = source.pipe(shareReplay(1));
    const sourceSubs =      '^           !';
    const subscriber1 = hot('a|           ').pipe(mergeMapTo(shared));
    const expected1   =     '-1-2-3----4-|';
    const subscriber2 = hot('    b|       ').pipe(mergeMapTo(shared));
    const expected2   =     '    23----4-|';
    const subscriber3 = hot('        c|   ').pipe(mergeMapTo(shared));
    const expected3   =     '        3-4-|';

    expectObservable(subscriber1).toBe(expected1);
    expectObservable(subscriber2).toBe(expected2);
    expectObservable(subscriber3).toBe(expected3);
    expectSubscriptions(source.subscriptions).toBe(sourceSubs);
});

would be rewritten to something like this

it('should multicast the same values to multiple observers, bufferSize=1', () => {
    const source =      cold('-1-2-3----4-|'); const shared = source.pipe(shareReplay(1));
    const sourceSubs =       '^           !';
    const expected1   =      '-1-2-3----4-|';
    const expected2   =      '    23----4-|';
    const expected3   =      '        3-4-|';

    expectObservable(shared, '^           !').toBe(expected1);
    expectObservable(shared, '    ^       !').toBe(expected2);
    expectObservable(shared, '        ^   !').toBe(expected3);
    expectSubscriptions(source.subscriptions).toBe(sourceSubs);
});

@benlesh
Copy link
Member

benlesh commented Aug 10, 2018

@gstamac That's a great example! I'll add this to our core team agenda so we can discuss it. Thanks!

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Aug 10, 2018
@benlesh
Copy link
Member

benlesh commented Aug 10, 2018

#4006 fixes the travis issue.

@benlesh benlesh merged commit 0d20255 into ReactiveX:master Aug 20, 2018
@benlesh
Copy link
Member

benlesh commented Aug 20, 2018

Thanks, @gstamac! Good addition!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 19, 2018
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.

4 participants