test(take): refactor test use rxsandbox

[kwonoj]

Description:

DONOTMERGE (Test will fail anyway, so can't be merged)
related to #3254, #3231 and #3265.

This PR is poc attempt to move our test assertion to use rx-sandbox instead, provides strong type safety around interfaces, as well as fixes some of known issues in current test assertion.

Unfortunately there is one nasty issue - since rx-sandbox has deps to Rx, there are 2 observable object (from source we'd like to assert, and one rx-sandbox uses internally) and it creates some conflict around scheduling, so couple of subscription tests are failing in here.

Not entirely sure how this can be resolved in quick manner - probably best thing is setting up monorepo and use linked pkg to source allows to have same object over, otherwise we may need to port changes of sandbox into core / or try to expose override rxsandbox to import source directly. Opening up as discussion, since @david-driscoll created PR around current test assertion - we may need to decide which way before make actions.

Related issue (if exists):

[kwonoj]

@david-driscoll plz check this out too.

[rxjs-bot]

	Fails
🚫	missing type definition import in tests (take-spec.ts) (1)

	Messages
📖

(1) : It seems updated test cases uses test scheduler interface hot, cold but miss to import type signature for those.

CJS: 1363.6KB, global: 731.1KB (gzipped: 118.0KB), min: 141.3KB (gzipped: 30.8KB)

Generated by 🚫 dangerJS

[benlesh]

So far it looks okay to me... @jayphelps, I know you have keen interest in this, what do you think?

@kwonoj: can we get the changes that exist in rx-sandbox implemented in core? What documentation exists for this? Did you happen to write down any design rationale?

[kwonoj]

@benlesh

1. porting changes over to core is possible, while it requires some of refactorings of course (mainly due to difference between current testscheduler)

2. I didn't write specific rationonals, most of api surface are documented in https://github.com/kwonoj/rx-sandbox though. Primary differences are

• scheduler flush is on its own instead of hookup test assertion (to make test framework agnostic)
• marble is 1 frame (configurable, no big deal)
• few additional features on marbles (as we've discussed before, like long-frame support, advanceTo support, etcs)

Mostly rx-sandbox is feature parity to TetsScheduler in core.

[jayphelps]

@benlesh

So far it looks okay to me... @jayphelps, I know you have keen interest in this, what do you think?

I think we should pull them into core, rather than depend on rx-sandbox. If you go with this new API I'd prefer some bikesheddding around the name getMessages though off hand I'm not sure what to call it either haha. I really dig the intent of his changes though, so the dev can just do deep array comparisons.

It does feel a bit weird that it's mutable array that changes as virtual time advances. e.g. this example in the docs

const e1 = hot('    --a--b--|');
const subs = s(`    ^       !`);
const messages = getMessages(e1.mapTo('x'));

//at this moment, messages are empty!
expect(messages).to.be.empty;

advanceTo(3);
const expected = e('--x------'); // we're flushing to frame 3 only, so rest of marbles are not constructed

//now values are available
expect(messages).to.deep.equal(expected);
//subscriptions are also available too
expect(e1.subscriptions).to.deep.equal(subs);

I'm not sure if this is weird enough for me to object, just was my initial reaction. It's probably fine or even ideal.

[kwonoj]

I think we should pull them into core, rather than depend on rx-sandbox.

Agreed, consider this if POC only. I don't think it's viable to make rx-sandbox as dep, first we'd go for core and eventually create separate pkg once we have core side monorepo.

I'm not sure if this is weird enough for me to object, just was my initial reaction. It's probably fine or even ideal.

debatable, obviously. For me it's kinda-ok api, as advanceTo and flush is orthogonal that you can't advance against scheduler already flushed (opposite is allowed if I remember correctly, that you advance first then flush out rest). This is sort of similar to v4 scheduler test, while v4 sets assertion inside of callbacks to get updated values over, while mine just mutates array directly. Mostly it's based on assumption mutation occurs synchronously due to nature of virtualtimescheduler - so user can access values imperatively over virtual time passes.

This design is based on intention of you're getting notification metadata array directly so you can assert as you want - while given array is mutable so over virtual time passes you get updated values. If there's better ergonomics, this can be discussed over via first PR migrating sandbox impl to core.

/cc @jayphelps ☝️

[kwonoj]

closing as same reason for #3314 .

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