ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.84k stars 3.01k forks source link

Limitation in marble testing use cases due to grouping semantics #5677

Open joshribakoff opened 4 years ago

joshribakoff commented 4 years ago

Feature Request

Is your feature request related to a problem? Please describe.

While it can be unintuitive at first, after all the values have synchronously emitted time will progress a number of frames equal to the number of ASCII characters in the group, including the parentheses. e.g. '(abc)' will emit the values of a, b, and c synchronously in the same frame and then advance virtual time by 5 frames, '(abc)'.length === 5. This is done because it often helps you vertically align your marble diagrams

It is therefore impossible to use marbles to test that pairs of values are emitted in subsequent frames. I would propose making this behavior configurable or making a breaking change to revert it. The rationale, people who want to visually align things can still do so (eg by adding spaces, or writing their test around the visual alignment). Contrast this with people who don't care about visual alignment, but want to be able to test some logic that must work this way (pairs of values on subsequent ticks). I for one disagree its a good tradeoff to make certain use cases impossible for the sake of making visual alignment easier.

Describe the solution you'd like Have an option to change, or make a breaking change so it uses 1 tick when there are groupings. Users can use spaces to create alignment, or the users who insist on visual alignment can contort their tests instead of imposing this "contortion" on the use cases that marbles can possibly test

Describe alternatives you've considered N/a, other than working around by not testing this use case at all

Additional context Screen Shot 2020-08-25 at 11 39 42 AM

benlesh commented 4 years ago

So the desired solution would look like this?

rxTest.run(({ cold, expectObservable }) => {
   const a = cold('           (aaaaa)    ');
   const b = cold('                    (bbbbb) ');
   const c = cold('                             (ccccc)');
   const source = cold('------a      --b      --c      ---|', { a, b, c });
   const expected = '   ------(aaaaa)--(bbbbb)--(ccccc)---|';

   const result = source.pipe(mergeAll());
   expectObservable(result).toBe(expected);
});

It seems reasonable. I'm not totally sure of the use case though. The primary use for marble tests is for testing operators, and the dashes and characters really signify the arbitrary passage of time to ensure order. It's unlikely that any process could consistently get the fidelity of < 1ms... but I suppose that will change as time marches on. In fact, each dash could just as easily represent an hour as a millisecond. In other words "what is a frame?" There are similar limitations with the difference between jobs on the event loop, micro tasks, etc. Animation frames are even more difficult.. N functions executing with the same high res timestamp? Not a lot of great ways to emulated that with marble diagrams at the moment.

It would certainly be a breaking change, so we couldn't just introduce this as a default. It would need to be via a setting.

As a team, we'll have to weigh this against it's usefulness and other priorities. I'll make this an agenda item to discuss.

kwonoj commented 4 years ago

@benlesh those reasoning is actually reason why we did decide to not upstream my rx-sandbox.

kwonoj commented 4 years ago

(Since rx-sandbox is still using frame-based virtual time approach)

joshribakoff commented 4 years ago

Yes that’s correct.

Imagine an operator that runs multiple concurrent intervals internally that can emit on the same tick if they happen to run in sync with each other. Eg. One interval started at 109ms to fire every 200ms, a second interval started at 209ms for every 100ms. This is only an edge case and the consumer desires singular values in the happy path.

Now imagine that operator also emits at the point in time some other event happens (which starts a new interval and has to emit a 0 initial value)

For all we know we may have N emissions at the 100ms mark, and then an event at 101ms requires another emission at that exact ms.

Right now it’s not possible to test

It would certainly be a breaking change

One option is to do with the addition of new syntax and optionally deprecate old syntax such that it’s non breaking, but we’re saying the same thing (it’d need to be optional)

An easy workaround may be to bufferTime(1) (in the test only), and just assert on arrays, this actually wouldn't be too bad, it is how many users assert jest mocks were called by passing in an array of calls, which themselves are arrays of arguments, which in turn also may be arrays. It's not "ideal", but it's also by no means a blocker...

benlesh commented 4 years ago

After discussion about #5760, core team decided to punt on certain TestScheduler ergonomics issues until next major.

RobertDiebels commented 1 year ago

Hi all, I would like to ask if this request can be brought back to life for RX v8. I bumped into this issue recently and aside from it being counter-intuitive it also requires adjusting test-code to match marble-parsing behavior. That last issue to me should mark this not as an FR but as a bug.

I wrote the following UT to illustrate:

 it('should emit twice per frame', () => {
        testScheduler.run(({cold, expectObservable}) => {
            // Assemble
            const input = cold('t---f', {t: true, f: false}); // This input does not represent expected input observable cold('tf',...).

            function doubleEmissions(input: Observable<boolean>): Observable<boolean> {
                return new Observable((subscriber) => {
                    input.subscribe({
                        next(value) {
                            subscriber.next(value);
                            subscriber.next(value);
                        }
                    });
                });
            }

            // Act
            const actual: Observable<boolean> = doubleEmissions(input);
            // Assert
            expectObservable(actual).toBe('(tt)(ff)', {t: true, f: false});
        });
    });

Here I have to alter the input observable to ensure that I get a passing test. I do not think that it is within the expected behavior of similar syntax (regex for instance) that grouping-syntax impacts output in any other way than grouping.