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

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

Open
joshribakoff opened this issue Aug 25, 2020 · 6 comments
Open

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

joshribakoff opened this issue Aug 25, 2020 · 6 comments

Comments

@joshribakoff
Copy link
Contributor

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

@joshribakoff joshribakoff changed the title Limitation is marble testing Limitation is marble testing use cases due to grouping semantics Aug 25, 2020
@benlesh
Copy link
Member

benlesh commented Sep 3, 2020

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.

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Sep 3, 2020
@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2020

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

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2020

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

@joshribakoff
Copy link
Contributor Author

joshribakoff commented Sep 3, 2020

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...

@joshribakoff joshribakoff changed the title Limitation is marble testing use cases due to grouping semantics Limitation in marble testing use cases due to grouping semantics Sep 3, 2020
@benlesh
Copy link
Member

benlesh commented Oct 7, 2020

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

@benlesh benlesh added Needs Monorepo and removed AGENDA ITEM Flagged for discussion at core team meetings labels Oct 7, 2020
@RobertDiebels
Copy link

RobertDiebels commented Mar 17, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants