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

Friendlier waitFor errors #32

Closed
haroldtreen opened this Issue Sep 21, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@haroldtreen
Contributor

haroldtreen commented Sep 21, 2017

First off - this library has been a huge help, so thanks for sharing!

Current Behavior:

waitFor is not very user friendly in failure cases.

It you waitFor an event that is never fired, the test suite will just hang until a timeout kicks in.

image

This is a slow way to detect a failure.

Expected Behavior:

It would be awesome if redux-saga-tester could detect that a saga had run to completion without emitting the desired action.

await sagaTester.waitFor(actions.FAKE_ACTION);

// Fail: actions.FAKE_ACTION was never dispatched

I haven't dug into the saga internals too much, so not sure what the implementation would look like. If you have an idea and can point me in the direction, I'd be happy to open a PR.

Thanks!

@peterox

This comment has been minimized.

peterox commented Sep 22, 2017

I monkey patch SagaTester to do exactly that

SagaTester.prototype.myWaitFor = function(timeout, actionType, futureOnly) {
  function doTimeout(timeout) {
    return new Promise(function(resolve, reject) {
      setTimeout(reject, timeout - 100, `Timed out waiting for action ${actionType} to be dispatched from saga`);
    });
  }

  return Promise.race([doTimeout(timeout), this.waitFor(actionType, futureOnly)]);
};

You need to pass in the timeout value but it works well.
Example:

it('failed - form error', async function () {
      sagaTester.dispatch(sendPasswordResetEmail('test@example.com'));
      await sagaTester.myWaitFor(2000, sendPasswordResetEmailFailed().type);
      expect(sagaTester.getCalledActions()).to.deep.include(...);
    });
});
@haroldtreen

This comment has been minimized.

Contributor

haroldtreen commented Sep 22, 2017

The error message is definitely nicer that way 👍 . It's still time based though.

It would be awesome if you could detect the saga had run to completion and never dispatched that action - then explode.

@peterdivvito

This comment has been minimized.

peterdivvito commented Sep 22, 2017

I'm not sure if that would be possible as the original error you saw was from the testing framework (in your case Jasmine), not SagaTester. Once jasmine times out there is no way to continue with the test.

The caveat to my patch is that the timeout value must be <= to the testing frameworks timeout.

When I was using mocha I was calling timeout() to get that, but as the is no equivalent in jasmine I now just hard code a value of 2 seconds

@haroldtreen

This comment has been minimized.

Contributor

haroldtreen commented Sep 22, 2017

The test isn't failing because code is taking too long to run, so the timeout message is irrelevant.
The test is failing because waitFor can accept non-existent actions and will never return.

I would like waitFor to detect that the saga is complete and return an error stating that the action was never detected.

@mzedeler

This comment has been minimized.

mzedeler commented Dec 16, 2017

I have a similar problem - I am looking for two different actions, let's call them openDone and openError. If I receive openDone, the test succeds and if I get openError, the test fails.

I propose that we extend the api of the saga tester like this:

  • waitFor accepts multiple actions to wait for.
  • We add a more generic wait that just returns the first available action.

I don't mind adding the above if someone with access can approve a PR.

@mzedeler

This comment has been minimized.

mzedeler commented Dec 16, 2017

Ping @3LOK.

@haroldtreen

This comment has been minimized.

Contributor

haroldtreen commented Dec 16, 2017

@mzedeler Not sure how much our problems align, but I'm going to open a PR for the fix I was proposing 👍 .

@haroldtreen

This comment has been minimized.

Contributor

haroldtreen commented Dec 16, 2017

PR opened @3LOK 👍

#43

@mzedeler

This comment has been minimized.

mzedeler commented Dec 17, 2017

I'm sure it is a different thing, but do go ahead, @haroldtreen :-)

@3LOK

This comment has been minimized.

Member

3LOK commented Dec 27, 2017

@haroldtreen's PR merged.
@mzedeler - since waitFor returns a promise, can't you use Promise.race or Promise.all for your use case? or am I missing something?

@haroldtreen

This comment has been minimized.

Contributor

haroldtreen commented Dec 27, 2017

Yay! Well my PR addressed the original reason I opened this issue. So fixed!

I'll close and let others open another if there's any lingering concerns. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment