[jest-jasmine2]: Test throws error when a non promise/undefined value is returned

[mattphillips]

🐛 Bug Report

I'm not sure if this is a bug report or feature request 🤷‍♂️

Currently Jest throws the following error if the return value from a test is not either a Promise or undefined:

Jest: `it` and `test` must return either a Promise or undefined.

I'm not sure what value this behaviour adds to Jest - while it does seem weird to return from inside a test (as what would you be returning to?) I don't think that it should fail the test by throwing an error.

In jest-chain I've had the following issue mattphillips/jest-chain#1 raised for when using arrow functions without a body i.e.

it('returns 4 for [1, 3]', () => expect(sum(1, 3)).toBe(4));

This causes the error because jest-chain binds all of the available matchers together to be chainable, so the expect statement is no longer returning undefined but rather itself (the expect object).

It appears this behaviour was added a while back when adding support for promises in 644a512d and is not something that is supported in Jasmine itself - nor jest-circus from what I could see.

---

Solution

I think a better solution to this would be to remove the error throwing code here and instead add a new rule to eslint-plugin-jest if we think this is something that we should advise against rather than throwing an error.

I'm happy to send a PR with the changes if you guys approve removing this error scenario? 😄

EDIT: It was such a quick change and I wanted to see it working so I've sent a PR see #6517

To Reproduce

it('hello world', () => {
  expect('hello').toBeTruthy();
  return 'world';
});

Expected behavior

I would expect this to not throw an error.

Link to repl or repo (highly encouraged)

https://repl.it/repls/SorrowfulStarkRouter

[aaronabramov]

the reason behind adding this error was that sometimes code under test might return something other than promise when promise is expected.
e.g. if we were to write an async test like this:

test('some async fn', () => {
  return makeHttpRequest('http://jestjestjset');
});

if makeHttpReuqest returns a promise, the test will wait for it to resolve/reject and pass/fail the test based on that.

now if something in the code changes and makeHttpRequest does not return a promise any more and instead takes a callback, the test will just finish as green and exit (and whatever happens with http request will be ignored)

This was a common and dangerous thing :) Things are pretty different now since we mostly use async functions, but i'm still not sure if it's worth removing this invariant.

[mattphillips]

Ah I see why it was added now 😄 While I agree it’s not good to allow tests to give false positives by passing when never running the async code, I think it would be great if we could offer an alternative solution than throwing.

With async/await and expect.hasAssrtions/expect.assertions(n) we are in a great position to stop recommending that when testing async code you should return a promise. I think we already have some nice eslint rules like: prefer-expect-assertion and there is babel-jest-assertions to automate expect checking.

I think overall I agree with the sentiment of the behaviour but it would be better to provide advice on best practises and the tools to check for user error rather than throwing an exception.

@aaronabramov how would you feel about moving this to eslint rules etc or do you think it’s too big of a change? Happy to go with what you think dude :)

[aaronabramov]

i also remember than before/afterEach hooks didn't follow the same pattern, but i'm not sure if i'ts still true

should we disallow explicit return in general? by suggesting to replace

test('111', () => {
  return makeHttpRequest();
});

with:

test('111', async () => {
  await makeHttpRequest();
});

@cpojer do you have any thoughts on that?

[SimenB]

Adding an eslint rule for "never return anything" is pretty easy as a first step, fwiw

[thymikee]

Good candidate for recommended

[stalniy]

there may be another use case. I'm an author of bdd-lazy-var tests interface which makes JS BDD test runners like mocha, jasmine and jest to look more like rspec.

Rspec has very nice shortcuts like:

describe Array do
  subject { [1,2,3] }

  it { is_expected.to include(1) }
end

So, is_expected refers to the test subject defined right after describe statement.

I'm in a process of adding support for it without message, so in JS world it looks like this:

describe('Array', () => {
  subject(() => [1,2,3] })

  it(() => is.expected.toContain(1))
})

As you can see it returns expectation. And jest fails with error Jest: itandtest must return either a Promise or undefined.

In order to overcome this, I can create a wrapper function but then I will create a redundant wrapper function for each it statement what doesn't look efficient from performance point of view.

So, I guess the minium which may satisfy all parties is to allow to return expectation instance, undefined or Promise.

What do you think guys?

[github-actions]

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.