feat: allow TestScheduler to accept comparison functions

[tklever]

TestScheduler is an invaluable tool when testing observables, however, in practice, the singular reliance on deepEqual can make for a burdensome and rigid testing experience. This change adds the ability for users to specify their own comparison functions. These user supplied comparisons can open the door to testing conveniences like property-value assertions that developers may be accustomed to.

The first step to make this a reality was to step away from comparing observables as a whole and compare each individual emission. This allowed me to write up some (what I believe to be) more helpful error messages when tests fail, but the downside is test failures no longer provide "whole stream" context.

The Good

• User Supplied Comparison Functions (allow a developer to run propVal, greaterThan, hasMembers, etc. on an observable's values)
• Targeted specific error messages about failures, no more (potentially giant) deep equal monster for test failures

The Bad

• TestScheduler needs to do a handful of comparisons now, which I'm doing with lodash, I could pass this function in, but that would require a BC break on the scheduler constructor, which I didn't want to do unless I got feedback to do so.
• I stole the error stack normalization and stringify functions from testscheduler-ui for use in this class, open to suggestions on how I should appropriately remove that duplication.

[kwonoj]

I am actually more interested in interfaces around taking out Notification values instead of inject comparison into assertion of test scheduler as similar to https://github.com/kwonoj/rx-sandbox#custom-assertion-for-marble-diagram (yes, it's my work), allows scheduler itself becomes test framework / assertion agnostic and anyone can use their own comparison. Core team has been thinking of improving / spliiting test related code from core and do have some additional changes (including breaking changes).

[rxjs-bot]

	Messages
📖

CJS: 1390.9KB, global: 1283.2KB (gzipped: 214.7KB), min: 218.1KB (gzipped: 55.9KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage decreased (-0.2%) to 96.831% when pulling 3c5deda on tklever:test-scheduler-user-comparison-function into c9f69ad on ReactiveX:master.

[tklever]

@kwonoj, Certainly open to going down that road and think that it's probably a more robust long term solution. This change struck me a minimally invasive (for now) and I get assertion agnostic (although wrapped) out of the deal.

If that's what the core team wants to see, it's honestly not an impossible amount of work. The marble parsing would have to refactored out independently and on first glance some kind of "Observable Notifications" result object containing emissions and subscriptions would need to exist (similar concepts in your library)

[kwonoj]

@tklever thanks for creating PR, now core team trying to make actual changes in core repo around test scheduler interfaces. Rough proof of concept can be found at #3266, which resolves most of pain points around current interface - plz check it out. Once we have high level agreement, there'll be following actual PR to migrate it.

[benlesh]

cc @jayphelps is there anything here that you think we can include in the newer test runner work?

[jayphelps]

The first step to make this a reality was to step away from comparing observables as a whole and compare each individual emission.

It's not clear how this works in practice as the same function would be called for every next/error/complete individual wouldn't it? How do you distinguish?

@benlesh appears to be a non-breaking change, so it could be introduced. I'm still a bit confused though on the design.

[tklever]

In theory (and forgive me, it's been a bit since I worked on or thought about this), by being able to specify a variable comparison function, you could (in psudocode):

const obs = hot('a-b-c-d-(e|)', { 
    a: { num: 1 },
    b: { num: 2 },
    c: { num: 3 },
    d: { num: 4 },
    e: { num: 5 },
})

expectObservable(obs).withComparison(propValue.bind({}, 'num'), '1-2-3-4-(5|)');

Basically allowing for testing of pieces of a emission as, in some circumstances, deepEqual in practice can force a significant amount of mocking. I think a change like this (or allowing for variable comparisons through some means, even if this PR is not good) could make testing observables more flexible and in some cases easier to read.

The example above is contrived and small, but imagine for a moment my emission is a large object with many properties, and I want to test that only one of them is changing, or that every emission is a number greater than zero. To do this now, you have to drop back to subscribe.

As you mentioned the function would be called every time, but informed by the provided marble diagram (just as deepEqual is today).

Hope that helps clear up the DESIRE of this PR, even if the implementation isn't worthy of inclusion

[benlesh]

I'm terribly sorry, @tklever. I can see that you put a significant amount of work into this PR, but we're not going to accept it at this time, for the following reasons:

1. The team isn't sure we really understand this PR fully.
2. This was never a proposed feature nor was it discussed
3. We never had a chance to discuss a design and talk about ergonomics and how it fits into the ecosystem.

So sorry, again, and hopefully this isn't too off-putting, but I need to close this PR.

[tklever]

@benlesh No worries. Certainly not off putting in the slightest.

I'll keep an ear to the ground for @jayphelps' upcoming "improved test runner" work to see if that satisfies my needs or allows me a baseline to re-propose an updated and clarified version of this idea.

Sorry I never made an articulate point here, thanks for taking the time to review it.

[jayphelps]

The latest testing update was released as part of v6 https://github.com/ReactiveX/rxjs/blob/master/doc/marble-testing.md