cartant / rxjs-marbles

An RxJS marble testing library for any test framework
https://cartant.github.io/rxjs-marbles/
MIT License
300 stars 18 forks source link

Behavior wrt white space in marble diagrams not conforming to rxjs's documentation #40

Closed huy-nguyen closed 6 years ago

huy-nguyen commented 6 years ago

rxjs's documentation on marble syntax says that

' ' whitespace: horizontal whitespace is ignored, and can be used to help vertically align multiple marble diagrams.

These are the unit tests inside RxJS itself that show in details how white spaces should be treated (tests named “should ignore whitespace when runMode=true”).

However, it looks like the wayrxjs-marbles parses white spaces in marble diagrams doesn't obey that rule.

I made a fork with a few small changes to demonstrate the problem. On the whitespace-bug-demo branch, I first created a test for white space behavior without actually using those whitespace (commit 6828f98) . The tests for jest pass at this point. The test command I used is the same the one you used to test jest:

Then I add white spaces into a marble diagram (commit 1a36cc4). The white space test failed, indicating that the added white spaces changed how the marble diagrams are parsed when they really shouldn't. This is the diff between those two commits for your convenience.

Am I misunderstanding the rxjs's marble syntax explanation or is there a bug in your library?

cartant commented 6 years ago

rxjs-marbles uses RxJS's TestScheduler and the test scheduler's behaviour depends upon whether or not the tests are performed using the test scheduler's run function. The behaviour is radically different and rxjs-marbles does not yet support using run.

That is, it behaves as described here. In particular:

Each space equals 1 frame, same as a hyphen -

Support for using run will eventually be added. However, as the run-related changes were made mere days before RxJS v6 was officially released (the run-related changes were not included in the beta or release candidates) it may be some time before rxjs-marbles supports run.

huy-nguyen commented 6 years ago

How easy or hard is it to support run mode in your library, given that the usage of TestScheduler outside of run mode is now deprecated?

cartant commented 6 years ago

It's not super hard; I just want to do it in a flexible manner. Using the old behaviour should be an option whilst it's deprecated and v6 only just came out, so it needs to be supported for a while, yet.

I'll have a think about it and there should be a new release in the next week or so. It'll likely be a major version bump - as it'll be a breaking change - with the behaviour changing to default to using run and an option to not use it - i.e. to stick with the old behaviour.

huy-nguyen commented 6 years ago

I’ll keep an eye out for the new version. Thanks for working on it.

cartant commented 6 years ago

No worries. If the run-related changes had been made during the release candidate, I would have incorporated them into rxjs-marbles. But they weren't. So I didn't. They were something of a surprise.

I'll notify you via this issue as soon as the next version is available.

cartant commented 6 years ago

I've just published 4.0.0, which defaults to using the new run behaviour. The old behaviour is accessible via the configure function - which has changed. See the CHANGELOG for details.

huy-nguyen commented 6 years ago

Do you want me to make a PR with my test to prevent regressions?

cartant commented 6 years ago

Sure. Open a PR; I'd be happy to merge it.