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

Passing a done callback #43

Closed Hetty82 closed 6 years ago

Hetty82 commented 6 years ago

I am using rxjs-marbles with Jest (and very thankful for it). However I would like to use the done() callback, as I can do with test that don't use the marbles. Is there a way to do that? Or another recommended way to test if the subscribe has fired? Thanks in advance!

cartant commented 6 years ago

If you are testing an observable without using a marble diagram, you might be looking for the observe helper. I've been using it internally in a number of projects and I've recently added it to this project (so that it's just in one place) but I've not documented it. It performs the subscribe and wires the the done callback, etc. See this test for an example.

If you are using a marble diagram and still want to use the done callback, you should be able to do so like this:

test("some test", marbles((m, done: DoneCallback) => {
    /* ... */
    setTimeout(() => {
        m.flush();
        done();
    }, 1000);
});

However, I'm close to certain that this will not work unless { run: false } is specified, as the TestScheduler's new run method assumes a synchronous test. That is, if you need to call any of the context methods - e.g. m.flush() - asynchronously, it probably won't work.

cartant commented 6 years ago

Actually, what do you mean by "test if the subscribe has fired?" If you can give me an example of what it is you are trying to test, I might be able to be of more help. Generally, you should do your utmost to avoid any asynchrony with marble tests.

Hetty82 commented 6 years ago

Hey thanks for the response. This is our case, we're using ngrx effects library. This is the effect (which doesn't return an action):

@Effect({ dispatch: false })
clearToken$: Observable<Action> = this.actions$
  .ofType(tokenActions.CLEAR_TOKEN)
  .do(() => {
    this.tokenService.clear();
  });

And here is the spec:

describe('clearToken$', () => {
  test('should call the tokenService', marbles((m) => {
    // Arrange
    const action = new tokenActions.ClearToken();

    // Act
    actions = m.hot('--a-', { a: action });

    // Assert
    effects.clearToken$.subscribe(() => {
      expect(tokenService.clear).toHaveBeenCalled();
    });
  }));
});

What I would like to do is something like this:

// Assert
effects.clearToken$.subscribe(() => {
   expect(tokenService.clear).toHaveBeenCalled();
   done(); // <---
});

We could achieve the desired result like this:

// Arrange
const action = new tokenActions.ClearToken();
const done = new Subject()

// Act
actions = m.hot('--a-', { a: action });

// Assert
effects.clearToken$.subscribe(() => {
  expect(tokenService.clear).toHaveBeenCalled();
  done.next(true);
});
m.expect(done).toBeObservable(m.cold('--b', { b: true }));

But it would be nicer to have a simple done() 😃 Thanks!

cartant commented 6 years ago

Yep, you should be able to do that. The marble test can use the test framework's done callback that's used for asynchronous tests, it's just the marbles-related calls that cannot be asynchronous.

This should do what you want, as the argument passed by the test framework is passed to the callback after the context - that is, after the m parameter:

describe('clearToken$', () => {
  test('should call the tokenService', marbles((m, done: DoneCallback) => {
    // Arrange
    const action = new tokenActions.ClearToken();

    // Act
    actions = m.hot('--a-', { a: action });

    // Assert
    effects.clearToken$.subscribe(() => {
      expect(tokenService.clear).toHaveBeenCalled();
      done();
    });
  }));
});

If it doesn't, please let me know. Although, without seeing the rest of the test infrastructure, I'm not sure how actions is wired up to the effects, etc.

Hetty82 commented 6 years ago

I get this: TypeError: done is not a function

cartant commented 6 years ago

I've added a test that shows how it works. It uses the DoneFunction declaration from rxjs-marbles, but you can use DoneCallback from the Jest typings. They should be the same. DoneFunction is included so that a hard dependency on jest is not required by the package.

Note, however, that if that test snippet you included is the entirety of the your test, I'm not sure it will work. My understanding of the TestScheduler is that a marbles expectation is required for everything to be wired up and I don't see one of those in your test.

If you are just trying to use a hot, marbles observable as a source, I don't think it will work. In which case, you should probably use one of the more conventional NgRx effects testing strategies.

Hetty82 commented 6 years ago

Allright thanks. We'll use our own done.next() then.

Halt001 commented 6 years ago

It seems the done callback is remove inside marbles. When I bind it back it all seems to work (with Jest) but is this a safe/good aproach?

const MarblesWithDone = (testFn: any) => (done) => {
  const testFnWithBoundDone = testFn.bind(null, done);

  return marbles(testFnWithBoundDone)();
};

describe('Testing with Marbles and Done', () => {

  test('should work as long as done is called', MarblesWithDone((done, m) => {

    const source =  m.hot('--^-a-b-c-|');
    const subs =            '^-------!';
    const expected =        '--b-c-d-|';

    const destination = source.pipe(
        map((value: string) => String.fromCharCode(value.charCodeAt(0) + 1))
    );
    m.expect(destination).toBeObservable(expected);
    m.expect(source).toHaveSubscriptions(subs);

    done();
  }));
});
cartant commented 6 years ago

@Halt001 There is a working fixture in the Jest tests that uses done callback. However, using marble tests with asynchrony should be avoided and from the snippet you've included, it's not clear why it's even necessary.

As stated above, the tests run using the TestScheduler's run method have to be synchronous, as it 'cleans up' its changes to the testing infrastructure synchronously.

cartant commented 6 years ago

Closing this because asynchronous marble tests are not something I wish to encourage.

Halt001 commented 6 years ago

You are right about done() not being necessary in my snippet. It was just for illustration purposes.

cartant commented 6 years ago

You should consider why you need an asynchronous marbles test and look for alternatives. The TestScheduler is simply not intended to be used with an asynchronous test. And with the introduction of the run function, it simply won't work.

Note that the re-instating of the non-run configuration happens synchronously.