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

Productizing our test suite. #1775

Closed
benlesh opened this issue Jun 16, 2016 · 20 comments
Closed

Productizing our test suite. #1775

benlesh opened this issue Jun 16, 2016 · 20 comments

Comments

@benlesh
Copy link
Member

benlesh commented Jun 16, 2016

It's of paramount importance that we create some sort of test suite out of our tools that is easy for people to consume. Right now we're doing a lot of "magic" in our tests (like patching it to flush the test scheduler and setting a global test scheduler).

We really need to productize what we have so others can benefit from it more easily. That might mean we need to change the API a little. Perhaps something like:

import { rxSyncTest } from 'rxjs-test';

describe('some feature with observables', () => {
  it('should do things', () => {
    rxSyncTest((expect, { scheduler, hot, cold, time, etc }) => {
        // set up any tests in here, it will execute _immediately_ and when this block is done,
        // it will flush the scheduler

        let s = hot('---a---b---c---|');
        let e =     '---a---b---c---|');
        expect(s).toBe(e);
    });
  });
});

basically, the API I'm thinking of is:

interface Context {
  scheduler: TestScheduler;
  hot: (marbles: string) => HotObservable;
  cold: (marbles: string) => ColdObservable;
  time: (marbles: string) => number;
}

const rxSyncTest = ((expect: expectObservableFunc, context: Context) => void) => void;

This testing suite needs to:

  1. Be easy to use
  2. Be extensible
  3. Work with most major testing libraries (Jasmine, Mocha, QUnit, etc)

The names, of course, can be bikeshedded somewhat, but I'd like to get this out. I went with rxSyncTest because I want people to realize that this test is to be run in a totally synchronous way, and that they shouldn't be adding async code inside of it.

@kwonoj
Copy link
Member

kwonoj commented Jun 17, 2016

What about sandbox style api? Not exactly same interface though...

import { rxSandbox } from '...'

describe('suite',  () => {
    let sandBox: Context;
    let scheduler, hot, cold;

    beforeEach(() => {
        sandBox = rxSandbox.setup();
        { scheduler, hot, cold } = sandBox;
    });

    afterEach(() => {
        rxSandbox.tearDown(); // if global teardown required in implementation
    });

    it('test', () => {
        let s = hot('---a---b---c---|');
        let e =     '---a---(b|)';

        Observable.of(1).getMarble(); //throws, you can't get marble from real observable        

        //getMarble() implicitly flushes testscheduler for first time
        //it's called in single test scheduler context,
        //returns text marble from hot / cold observable
        const marble = s.take(2).getMarble();
        expect(marble).toBe(e);
    })
});

Cons :

  • requires scheduler flush either manually or implicitly (via first getMarble()), can be looks like some dark magic
  • when forget to fixture tearDown or scheduler flush, can cause disaster in whole test case (maybe, or maybe not)

@staltz
Copy link
Member

staltz commented Jun 17, 2016

I sort of prefer @kwonoj's proposal.

@kwonoj
Copy link
Member

kwonoj commented Jun 23, 2016

I can take this up once initial api design reaches agreement.

@erykpiast
Copy link

Is there a plan to provide API similar to one from Rx 4? I mean onNext, onCompleted and onError functions accepting value and time (ones from ReactiveTest)? Are marbles considered as the only way to test Rx 5 code? I'm thinking about eventual migration from v4 to v5. Changes in code seems trivial, but I can't find out how to adapt my tests without much effort.

@kwonoj
Copy link
Member

kwonoj commented Jun 24, 2016

Are marbles considered as the only way to test Rx 5 code?

Since marble test was introduced with v5, it hasn't explicitly excluded but implicitly didn't support any kind of backward compatibility to different version. I think testScheduler would be piece would require most of interop to support v4 as well, not sure how much it'll require at this moment.

Current plan is make migration to v5 perfectly first before consider expanding support.

@erykpiast
Copy link

I'm not sure I was understood correctly. I would to ask if I can test Rx 5 code similarly to Rx 4, using onNext, onCompleted and onError functions of ReactiveTest. I migrated my Rx 4 code to Rx 5 without any problems, but I can't really migrate my tests easily. I don't see any other solution than creating marbles from scratch. Seems to be easy, but also very boring and repetitive work, so I would like to avoid it.

@kwonoj
Copy link
Member

kwonoj commented Jun 26, 2016

Ah sorry, I understood in opposite way. No, I don't think there's immediate plan for those, migrating rx4 test to support rx5. It's just matter of priority, there are things to be wrapped first for public release of rx5. First goal is make rx5 test assertion suite first.

@erykpiast
Copy link

Well, I understand there are priorities. Thank you for your answer :)

@kwonoj kwonoj self-assigned this Jul 15, 2016
@paulpdaniels
Copy link
Contributor

Any movement on this? I started writing the chapter on testing in RxJS in Action and it would be great to have this ironed out.

@kwonoj
Copy link
Member

kwonoj commented Aug 9, 2016

I did some experiment based on my proposal, and seems it takes some time with those way. Haven't decided to go further down with current approach yet, may need some more time.

@kwonoj
Copy link
Member

kwonoj commented Oct 3, 2016

@erykpiast I also hit this issue recently and came to need v4 interface as well, wrote small package for those purpose. It is not 100% compliant and may possibly have debatable changes, so isn't submitted upstream PR.

@benlesh
Copy link
Member Author

benlesh commented Oct 3, 2016

FWIW @jayphelps has been doing some work in this area. Perhaps he can comment

@Zalastax
Copy link

Zalastax commented Nov 15, 2016

I've put some work into cleaning up the rendering of marble tests. Mainly reworking img2png/painter.js so that calculating the shapes and drawing them is done separately. This allowed me to add a new renderer that outputs svg files and has no dependencies, enabling rendering images directly in the browser if so wanted. I've made the changes in a separate repository but there shouldn't be any problem migrating it back. https://github.com/Zalastax/rxjs-marble-testing

I'd love to help with cleaning up the rest but would need direction from someone more involved before I get started. Ping @Zalastax on gitter so we can have a chat!

@kwonoj kwonoj removed their assignment Nov 16, 2016
@kwonoj
Copy link
Member

kwonoj commented Nov 16, 2016

I'm bit inactive in this issue at this moment, unassigning myself and /cc @jayphelps driving primary effort for revisiting api surface. @Zalastax afaik there possibly can be breaking changes in api surface, so might need align with those as well.

@Zalastax
Copy link

We also need to accommodate libraries built on top of Rx. E.g. redux-observable want a TestScheduler but starts things of with their own ActionsObservable and does some preparation before calling testScheduler.expectObservable. There's a pretty good demo at http://jsbin.com/pufima/edit?js,output. I'd guess that @jayphelps already knows about this but I just hit into this problem in my own project.

Zalastax added a commit to Zalastax/openschedule that referenced this issue Nov 24, 2016
I moved most unofficial dependencies from file to github.
Testing can be sort of done but needs more work before ActionObservables (the interesting thing to test) can be tested. Waiting for ReactiveX/rxjs#1775
@avocadowastaken
Copy link

Here how I made it work with jest.

Personally I hate globals, so I used TestScheduler explicitly.
That also allowed me override it's expectObservable method for my personal needs.

@kwonoj
Copy link
Member

kwonoj commented Aug 19, 2017

I have published separate module https://www.npmjs.com/package/rx-sandbox for this effort. It indeed have breaking api changes, so I've published it as user-level module to gather opinions around. Major difference is it doesn't depends on any framework specific setups or using its own assertion but exposes metadata directly, so any preferred assertion can be done.

@staltz
Copy link
Member

staltz commented Sep 11, 2017

Just as a reminder of the challenges for testing that Ben raised during contributor days, and where we stand right now:

Question mark means it's an open problem, green check means there's a relatively straight-forward solution.

  1. APIs that default schedulers ❓
  2. Really long running schedulers ✅
  3. Fine-tuned tests ✅
  4. Comprehensive strategy docs ❓

Default schedulers are going to be hard to invade and change, because currently they are (e.g. for delay) passed as default function arguments. Does anyone have an idea for this? @kwonoj does rx-sanbox do that? (as far I can see, it doesn't)

Challenge (2) is basically solved with the idea of ...n... already in use in rx-sandbox.

Challenge (3) seems solved with frameTimeFactor in rxSandbox.create(autoFlush?: boolean, frameTimeFactor?: number): RxSandboxInstance.

Challenge (4) is about many things, the one I'm thinking most about is testing cases when multiple other schedulers are used, like changing between requestAnimationFrame scheduler, breadth-first scheduler, and depth-first scheduler. These choices actually affect how code behaves, and expected outputs, not just the idea of "threads" like e.g. RxJava has. Should we have multiple TestSchedulers?

@kwonoj
Copy link
Member

kwonoj commented Sep 11, 2017

does rx-sanbox do that? (as far I can see, it doesn't)

You're correct, currently it doesn't touch that at all, aimed to create feature-parity to testscheduler in Rx first. need to think about this as well as (4).

@benlesh
Copy link
Member Author

benlesh commented Oct 11, 2018

This is done with TestScheduler.prototype.run

@benlesh benlesh closed this as completed Oct 11, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants