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

valid-expect-in-promise doesn't work with Promise.all #219

Closed
realityking opened this issue Jan 28, 2019 · 6 comments · Fixed by #916
Closed

valid-expect-in-promise doesn't work with Promise.all #219

realityking opened this issue Jan 28, 2019 · 6 comments · Fixed by #916

Comments

@realityking
Copy link

realityking commented Jan 28, 2019

I have a test case that relies on multiple promises being tested with Promise.all:

test('Resource loader: client usage', function () {
  expect.assertions(2);

  const quxLoader = ResourceLoader.create(['qux']);
  let promiseResolve;
  fetchResult = new Promise(resolve => promiseResolve = resolve);

  const promise1 = quxLoader.load();
  promise1.then(([x]) => expect(x.test).toBeTruthy());
  const promise2 = quxLoader.load();
  promise2.then(([x])  => expect(x.test).toBeTruthy());
  promiseResolve({test: true});

  return Promise.all([promise1, promise2]);
});

This causes the following eslint errors:

  68:3  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise
  70:3  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

It'd be great if the rule would recognize Promise.all and considered it valid.

@SimenB
Copy link
Member

SimenB commented Jan 28, 2019

You don't wait for them though - when you do .then you return a new promise (from promise1 and promise2) which you don't wait for.

const somePromise = Promise.resolve();

somePromise.then(() => Promise.reject('oh no'));

Promise.all([somePromise]).then(() => {
  console.log('this resolves');
});

// logs
// this resolves
// "Uncaught (in promise) oh no"

@realityking
Copy link
Author

realityking commented Jan 28, 2019

🤦‍♂️ Good catch!

But if I change things to consider the new promises, it still fails:

New code:

test('Resource loader: client usage', function () {
  expect.assertions(2);

  const quxLoader = ResourceLoader.create(['qux']);
  let promiseResolve;
  fetchResult = new Promise(resolve => promiseResolve = resolve);

  const promise1 = quxLoader.load()
    .then(([x]) => expect(x.test).toBeTruthy());
  const promise2 = quxLoader.load()
    .then(([x]) => expect(x.test).toBeTruthy());
  promiseResolve({test: true});

  return Promise.all([promise1, promise2]);
});

eslint report:

  67:9  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise
  69:9  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

@SimenB
Copy link
Member

SimenB commented Jan 28, 2019

Yeah, that's a bug 🙂 Doesn't seem like the rule handles Promise.all at all. PR welcome! 😀

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 9, 2021

For now I've decided that actually Promise.all shouldn't be allowed because it rejects as soon as one of the promises reject, which'll always happen if you've got expect since it throws, meaning at best you're creating a conditional expectation which is something we don't recommend anyway.

Instead, Promise.allSettled should be used to ensure all promises are fulfilled.

@github-actions
Copy link

github-actions bot commented Oct 9, 2021

🎉 This issue has been resolved in version 24.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This issue has been resolved in version 25.0.0-next.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants