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

Friendlier waitFor errors #32

Closed
haroldtreen opened this issue Sep 21, 2017 · 12 comments
Closed

Friendlier waitFor errors #32

haroldtreen opened this issue Sep 21, 2017 · 12 comments

Comments

@haroldtreen
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

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.

@ghost
Copy link

ghost 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
Copy link
Contributor Author

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
Copy link

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
Copy link

Ping @3LOK.

@haroldtreen
Copy link
Contributor Author

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

@haroldtreen
Copy link
Contributor Author

PR opened @3LOK 👍

#43

@mzedeler
Copy link

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

@3LOK
Copy link
Contributor

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
Copy link
Contributor Author

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

@tuzmusic
Copy link

@peterox I'm getting funny behavior with your monkeypatch:

it("returns an error for an invalid username", async () => {
    sagaStore.dispatch(login(creds.badUser));
    await sagaStore.myWaitFor(2000, "LOGIN_FAILURE");
    expect(sagaStore.getCalledActions()).toEqual([
      actions.startSuccess,
      actions.loginBadUser
    ]);
  });

I don't get an error message. Of any kind! Just Test run was interrupted.

  integration  returns an error for an invalid username

      233 |   });
      234 | 
    > 235 |   it("returns an error for an invalid username", async () => {
          |   ^
      236 |     sagaStore.dispatch(login(creds.badUser));
      237 |     await sagaStore.wait(2000, "LOGIN_FAILURE");

      at Env.it (node_modules/@jest/core/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:91:24)
      at Suite.it (tests/userLogin-saga.test.js:235:3)
      at Object.fdescribe (tests/userLogin-saga.test.js:220:1)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 18 skipped, 1 passed, 20 total
Snapshots:   0 total
Time:        9.723s
Test run was interrupted.

This is a test that should fail (I've yet to implement login failure correctly), and I've also implemented your function in a passing test, so it is working. Am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants