feat(TestScheduler): add sync-within-subscribe marker

[cartant]

Description:

This PR now adds a sync-within-subscribe marker as described in this comment.

Previous Description:

This PR changes the ColdObservable implementation so that notifications scheduled at frame zero are emitted within subscribe instead of being scheduled for emission at zero milliseconds. See #5523 for a more detailed explanation.

After making this change, there were a whole bunch of tests - a total of 21, IIRC - that needed to be updated, as they depended upon the notifications at frame zero being emitted after subscribe returned. For example, consider this test:

    const a = cold(  '|');
    const asubs =    '(^!)';
    const b = cold(  '-');
    const bsubs =    '(^!)';
    const expected = '|';
    expectObservable(zip(a, b)).toBe(expected);

If the complete notification in a occurs within the subscribe, the subscription to b - asserted by the test - will never occur.

So, in order for the test to pass - and to effect the behaviour it purports to test - the notification needs to be moved off the zero frame, like this:

    const a = cold(  '-|');
    const asubs =    '^!';
    const b = cold(  '--');
    const bsubs =    '^!';
    const expected = '-|';
    expectObservable(zip(a, b)).toBe(expected);

Pretty much all of the changes to the test are like this. The only exception was the test for throttle - and, TBH, I'm not altogether sure what's going on with that one which will pass once the fix in #5749 is merged.

As I mentioned in the issue, there are existing tests that appear to use cold in a manner that expects sync-within-subscribe notifications and there are others that explicitly use of instead of cold.

Whether or not we should make this change, IDK. As I mentioned in the issue, this behaviour could be seen as a quirk. It is, however, pretty confusing as it is at the moment.

Also, this would be a breaking change for anyone dependent upon the cold-observables-are-never-synchronous-within-subscribe behaviour.

Related issue (if exists): #5523

[benlesh]

With regards to our tests, I suppose we need to test the "scheduled(0)" and "unscheduled" routes that things can take. But I'm still thinking about this.

Considering that a - is truly an arbitrary measurement of time (even if we call it "1ms" in some tests, think about it, if we're contracting 1ms, or 100ms to happen "synchronously" in our tests, then what's the difference if a - represents a millisecond or an eon?) I suppose that one might argue that testing any delay at all beyond a synchronous emission is really apples to apples -- meaning that if everything in a desired test is scheduler(fn, 0), the test isn't any different if everything is schedule(fn, 100).

So I think this is okay, but it's a bit odd to me. that the first dash is just different now.

I think that we might consider doing this:

expectObservable(of('a', 'b', 'c')).toBe( '(^abc|)'); // PASS
expectObservable(of('a', 'b', 'c')).toBe('(abc|)'); // FAIL
expectObservable(scheduled('abc', asyncScheduler)).toBe('(abc|)'); // PASS

Where a grouping with a '^' at the lead means subscription occurred at that frame, and those values are synchronous with that subscription.

I think this even works with situations where we have a subscription defined and emissions are expected to happen synchronously with the subscription:

const source = hot('-----a---b----------c---d---e--');
const shared = source.pipe(shareReplay());
const sub1 = '      ----^-----------!    ';
const exp1 = '      -----a---b-------    ';
const sub2 = '      -----------^------------------!';
const exp2 = '      -----------(^ab)----c---d---e--'; // NOTE THIS LINE

expectObservable(shared, sub1).toBe(exp1);
expectObservable(shared, sub2).toBe(exp2);

What are your thoughts?

[cartant]

What are your thoughts?

Yeah, I like your (^abc) solution. And the fact that it's not breaking - that was my primary concern.

I'll have a look at implementing it later in the week. It'll change this PR from a fix to a feat. After that, I'll have a look through the tests to see if I can find sources that should be synchronous within subscribe. I'm pretty sure there are a few.

With regards to our tests, I suppose we need to test the "scheduled(0)" and "unscheduled" routes that things can take ...

I think it's worth adding tests to verify that subscriptions to sources that complete or error synchronously within subscribe short-circuit the subscription to further sources.

And, yeah, adding the leading - to the tests makes no difference, IMO. They still test the same thing.

BTW, the test for throttle that's skipped in this PR will pass when #5749 is merged - it includes a fix for throttle when it's used with a synchronous notifier.

[cartant]

@benlesh The (^abc) thing was pretty easy to implement, but it's still going to be a breaking change. There are about 150 tests in the repo that fail because they have expectations that need to be updated to include the synchronous-within-subscribe indicator - e.g.:

const a = cold('  -');
const asubs = '   (^!)';
const expected = '|';
const b: string[] = [];

expectObservable(a.pipe(zipWith(b))).toBe(expected);
expectSubscriptions(a.subscriptions).toBe(asubs);

Needs to become:

const a = cold('  -');
const asubs = '   (^!)';
const expected = '(^|)'; // UPDATED EXPECTATION
const b: string[] = [];

expectObservable(a.pipe(zipWith(b))).toBe(expected);
expectSubscriptions(a.subscriptions).toBe(asubs);

Updating them is no big deal, but this feature will be a breaking change.

[cartant]

I'm going to leave this for the moment. The breaking nature of this feature means that some tests require several changes - e.g.:

it('should handle a throw closing Observable', () => {
  const e1 = hot('--a--^---b---c---d---e---f---g---h------|');
  const e1subs =      '(^!)                                ';
  const e2 =  cold(   '#');
  const e2subs =      '(^!)                                ';
  const expected =    '(x#)                                ';
  const x = cold(     '#                                   ');
  const values = { x: x };

  const result = e1.pipe(windowWhen(() => e2));

  expectObservable(result).toBe(expected, values);
  expectSubscriptions(e1.subscriptions).toBe(e1subs);
  expectSubscriptions(e2.subscriptions).toBe(e2subs);
});

• The x is emitted within subscribe, so it needs the marker.
• The # is emitted outside of subscribe, so it needs to be moved out of the group`.
• That means the source needs to emit the # on a different frame.
• That changes the subscriptions.
• The frame upon which # ends up coincides with the frame upon which b is emitted.
• So b has to be moved.

The updated/worked-around test then looks like this:

it('should handle a throw closing Observable', () => {
  const e1 = hot('--a--^----b---c---d---e---f---g---h------|');
  const e1subs =      '^---!                                ';
  const e2 =  cold(   '----#');
  const e2subs =      '^---!                                ';
  const expected =    '(^x)#                                ';
  const x = cold(     '----#                                ');
  const values = { x: x };

  const result = e1.pipe(windowWhen(() => e2));

  expectObservable(result).toBe(expected, values);
  expectSubscriptions(e1.subscriptions).toBe(e1subs);
  expectSubscriptions(e2.subscriptions).toBe(e2subs);
});

What are your thoughts on this feature now? How well do you think library users would cope with a breakage like this in one of their tests?

[benlesh]

It seems like this change is going to have a large impact. While it fixes the TestScheduler, I'm not sure what impact it will have on the rest of the world, on other people's tests, or on other marble testing libraries. We might consider putting this behind some configuration on the TestScheduler itself or holding off for another major. I'm not sure. Perhaps we should discuss it at the next core team meeting?

[cartant]

Yeah, definitely hold off. Also, I have other changes that I'd like to make to the TestScheduler - both of which would make working with this feature a little easier and both would also be breaking. I agree that discussing it at the next meeting would be best. I'll write up my ideas in a HackMD document before the meeting.

[benlesh]

Core Team Meeting: Decided to pass on this work for v7... Look at all-new test scheduler in v8.