New testScheduler.run() auto-scheduler injection method and time progression syntax

[jayphelps]

Huge thank you to @benlesh for the countless hours helping me bikeshed testing infra in general and future related proposals, and @kwonoj for his input and work on rx-sandbox which provided a lot of the inspiration.

---

This PR adds a new run method on instances of TestScheduler. It accepts a callback which is then synchronously called with a single argument containing the helpers hot (short for createHotObservable), cold (short for createColdObservable), expectObservable, expectSubscription, and flush.

it('should do stuff', () => {
  const testScheduler = new TestScheduler(assertDeepEquals);

  testScheduler.run(({ cold, expectObservable }) => {
    const output = cold('-a-b-c--------|').pipe(
      debounceTime(5) // you don't have to pass testScheduler anymore!
    );
    const expected = '   ----------c---|';
    expectObservable(output).toBe(expected);
  });
});

Inside that callback you can use these helpers similar to how you would with the existing TestScheduler, except this new API allows us to introduce some changes in a non-breaking way. The changes listed below only apply inside the testScheduler.run(callback) callback.

Aside from the hot/cold name changes:

• The testScheduler instance will automatically be used by any operator that uses AsyncScheduler, e.g. delay, debounceTime, etc, so you no longer have to manually inject and pass it around.
• You can advance virtual time with a new time progression syntax e.g. -a 100ms b-|
• Whitespace no longer means 1 frame, instead it is just ignored so it can be used to arbitrarily format/align marbles.
• The testScheduler instance will automatically flush after the callback returns, or you can also explicitly flush it earlier.
• Each frame is now 1 virtual millisecond, instead of 10. i.e. TestScheduler.frameTimeFactor = 1
• There is no maximum frame count i.e. maxFrames = Number.POSITIVE_INFINITY

Remember: these changes do not apply to usage of TestScheduler outside of testScheduler.run(). If you call testScheduler.expectObservable() it will not have this new behavior and features..

Time progression syntax

The new time progression syntax takes inspiration from the CSS duration syntax. It's a number (int or float) immediately followed by a unit; ms (milliseconds), s (seconds), m (minutes). e.g. 100ms, 1.4s, 5.25m.

When it's not the first character of the diagram it must be padded a space before/after to disambiguate it from a series of marbles. e.g. a 1ms b needs the spaces because a1msb will be interpreted as ['a', '1', 'm', 's', 'b'] where each of these characters is a value that will be next()'d as-is.

NOTE: Very often you have to subtract 1 millisecond from the time you want to progress because the alphanumeric marbles (representing an actual emitted value) advance time 1 virtual frame. This can be very unintuitive and frustrating.

const input = ' -a-b-c|';
const output = '-- 9ms a 9ms b 9ms (c|)';

const result = cold(input).pipe(
  concatMap(d => of(d).pipe(
    delay(10)
  ))
);

expectObservable(result).toBe(expected);

We could change it so that value marbles no longer advance time, but this then has the affect of making marble diagrams less likely to align without adding extra whitespaces, similar to why grouping syntax advances the number of frames equal to the character count, e.g. (abc) advances time 5 frames. In previous discussions around making that change it was decided against, but this quirk was not known then so it might be wise to discuss?

Additional questions

• Do we deprecate the old usage?
  • We've found Internal testing of rxjs itself often has conflicting goals with app testing of rxjs code. Do these changes still play well for internal testing?
• Should this be documented? Currently the only official docs on the marble diagram testing is written from the prospective of testing rxjs internals, not app code that uses rxjs. It might seem like an obvious question, that we should document it, but even with these changes the TestScheduler has major shortcomings that we'd like to address.

The future

One of the goals was to change as little of the code as possible (to get this out quicker). The existing TestScheduler infrastructure is badly in need of a complete overhaul (all of the schedulers, to be honest). There has been numerous very involved attempts at doing so over many many months, including completely reimagining marble diagrams and ditching them altogether for a builder-pattern.

I won't spell out here all of the problems with the existing TestScheduler, but some of them are documented in this rough RFC that was based on numerous sessions between Ben Lesh and I. We were originally super optimistic with what we came up with, but as I spent the time and implemented it I kept running into cases we hadn't accounted for. And after revising, and revising, numerous times we unfortunately both came to the conclusion it had become something neither of us were happy with. It may not seem like it, but boy there are a lot of complexities to making a marble syntax that works and is aesthetically beneficial. While we will continue to bikeshed how to solve these problems, it was decided that we should provide this PR as an incremental change to at least solve some of the most common complaints.

[jayphelps]

Done so that I could assert that everything was correctly flushed

[jayphelps]

I'm not a huge fan of using a flag like this, but it seemed to be the easiest. The alternative of having two copies of the function seems like a worse nightmare and increases potential for someone to fix a bug in one but forget about the other.

[jayphelps]

While I'm open to alternatives, it turned out to be rather difficult to injection just for AsyncScheduler because TestScheduler extends VirtualScheduler which also extends AsyncScheduler, so changes made here also apply to TestScheduler and many attempts I had ended in just infinite recursion 😆

[coveralls]

Coverage increased (+0.02%) to 96.501% when pulling 9322b7d on jayphelps:testing2 into 91088da on ReactiveX:master.

[cartant]

Do you have an example of a diagram in which you don't have to subtract 1 millisecond from the time you want to progress?

One way of looking at it would be that in marble diagrams like "a--b" the absence of explicit durations implies a 1ms duration between characters. However, when a duration is explicitly specified, the implicit 1ms duration doesn't apply.

[jayphelps]

@cartant

Do you have an example of a diagram in which you don't have to subtract 1 millisecond from the time you want to progress?

You could absorb the extra needed ms before the duration.

'-a|'
'- 100ms (a|)'
// instead of
'-- 99ms (a|)'

Or if the input diagram also has an extra 1ms

'-a 100ms b|'
'-- 100ms b|'

One way of looking at it would be that in marble diagrams like "a--b" the absence of explicit durations implies a 1ms duration between characters. However, when a duration is explicitly specified, the implicit 1ms duration doesn't apply.

Can you elaborate? Currently 'a--b' means that 4 frames would pass.

[benlesh]

This test is fine, but I feel we should have more (any) unit tests around just the parseMarbles method. Something to assert that the diagrams are being converted to the appropriate object structures. It would make this test more sound, IMO.

[jayphelps]

Done. https://github.com/ReactiveX/rxjs/pull/3596/files#diff-dd8d0ea30be1698896e447581ec91fffR70

[benlesh]

We're missing the time helper, which was designed to convert a marble like -----| to a number in virtual ms.

[jayphelps]

We discussed and we're going to skip it for now as it's not clear if it actually adds enough value in a world with a single frame = 1ms, there's potential debate about what syntax it should support (e.g. next values or errors?), and it's a non-breaking change to include it later.

[benlesh]

Overall it looks good. Just missing one helper and we should probably add some unit tests around parsing marbles.

[cartant]

@jayphelps

Can you elaborate?

Basically, I was trying to think about how the explicitly-specified durations could be related to frames in a manner that would be conceptually easier to understand/explain.

The current behaviour makes sense when you think about the implementation. Consider this diagram:

a 10ms b

a happens and the TestScheduler advances one frame - which is 1 ms with a frameTimeFactor of 1 - then the scheduler's virtual time is progressed by 10 ms and then b occurs at 11 ms. That makes sense.

However, anyone looking at a 10ms b might reasonably expect a to occur and then b to occur 10 ms later. I think a fair amount of confusion could be avoided if that expectation could be met.

One way of achieving this might be to deem that each frame/character has a default/implied duration of 1 ms, but if the character is followed by an explicit time, that explicit time specifies the frame's/character's duration. So this:

ab

would be equivalent to this:

a 1ms b

But what that would mean for synchronous groups, I don't know. Synchronous groups are a pain point. Every single dev I've introduced to marble tests has misunderstood how virtual time is handled for synchronous groups.

---

Anyway, it's too late, now, as 6.0.0 has just been published and any changes along these lines would be breaking.

[jayphelps]

Every single dev I've introduced to marble tests has misunderstood how virtual time is handled for synchronous groups.

100%

Sorry we wanted to get this in ASAP since, based on the massive amount of iteration and bikeshedding that has already happened I’m fairly confident we won’t come up with a solid solution to these other problems in the short term. If you have interest in this though, please do continue to experiment and think of solutions. See the RFC doc in the original post above for inspiration to some of the problems we want to solve eventually. We can chat separately too.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.