Introduce a lint rule to report error when testing promises. If a exp…

[tushardhole]

…ectation is added inside a then or catch block of promise then as per jest docs, it is required to return that promise for successful execution of that expectation. This rule will report all such expectations where promise is not returned.

Summary
Implements #4843

As jest user I work on a project that heavily uses promises and jest is our tool to write tests. We were aware that as per jest docs, it is required to return a promise, if you want to test some thing in its fulfillment or rejection block.

If you do not return that promise and write some exceptions in its then or catch block. Then that test will always be green, as the expectation in then or catch block is never executed. If you return that promise then the expectation will be executed and tests will be red or green depending on the actual result of that expectation.

In this situation it becomes very important to make sure that,
a) You make that test red before making it green for the final push. Which will assure that the exception is getting executed.

As human, we may forget to do that and hence this lint rule will be helpful.

Test plan

1. Added a unit test case with many combinations.
2. Tested on an existing project which uses jest & promises.
3. Ran all tests using 'yarn test' command

Limitations

1. If the expectation inside then or catch is a expect statement then this rule will work correctly. In case, there is call expression to some function, from then or catch block and that function has the expect statement then this rule will not be able to detect that. I will work on that issue but I feel that as 'Good to Have' and should be next step to improve this rule and current PR changes can be very well considered as 'Step 1' which is very useful in its current state.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@272ff84). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #4844   +/-   ##
=========================================
  Coverage          ?   59.34%           
=========================================
  Files             ?      201           
  Lines             ?     6698           
  Branches          ?        4           
=========================================
  Hits              ?     3975           
  Misses            ?     2723           
  Partials          ?        0

Impacted Files	Coverage Δ
packages/eslint-plugin-jest/src/index.js	100% <ø> (ø)
...t-plugin-jest/src/rules/valid_expect_in_promise.js	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 272ff84...20f40dd. Read the comment docs.

[SimenB]

I'm not sure if this is the correct fix for the problem. I think this issue is more generally solved using something like https://www.npmjs.com/package/babel-jest-assertions? It will only fail when running the test instead of telling you inline in the IDE. But it will cover more cases (like if the catch is not actually called as the promised resolved)

A lint rule might be to always expect a expect.assertions(num).

[tushardhole]

@SimenB thanks for letting me know about that super-cool plugin 👍
It looks to be more in general way to solve this problem. I did give it a try and surprisingly, this was not working in this case, below is what happened,

1. There was a misunderstanding on my side that expect statement does not gets executed when you don't return the promise to be tested, that seems to be incorrect,
   example:

    it('should say hi on getting data', () => {
      getData().then((data) => {
      	console.log("The expect will be executed")
        expect(data).toEqual("Hi");
      });
    });

Here I was able to see "The expect will be executed" getting printed on console. And there is no effect on jest reporting, if I change the expectation of 'data' to 'any string' instead of 'Hi'

2. If i change the above example and add a return statement, then,

    it('should say hi on getting data', () => {
      return getData().then((data) => {
      	console.log("The expect will be executed")
        expect(data).toEqual("Hi");
      });
    });

Here I was able to see "The expect will be executed" getting printed on console. And there was effect on jest reporting, if I change the expectation of 'data' to 'any string' instead of 'Hi'

3. I tried an example, similar to one mentioned on that plugins read me page, like below,
   Note: Here I am directly using expect.hasAssertions(), just for simplicity in understanding,

it('works with resolves', () => {
  expect.hasAssertions();
  Promise.reject(10).then(value => {
    expect(value).toBe(1)
  })
});

Here, as the promise is rejected, expect.hasAssertions() will fail the test, since no assertion is executed.
if we resolve the promise as below,

it('works with resolves', () => {
  expect.hasAssertions();
  Promise.resolve(10).then(value => {
    expect(value).tobe(1)
  })
});

Here, ideally test should fail as value is '10'. But as said earlier this assertion gets executed but jest honors that only when we return that Promise. If I add return to above example test will fail for value not be 1

In the end, this lint rule looks to be required even if you use that plugin, as I discovered today that, assertion may or may not get executed but jest will honor result on that assertion only if return statement is added.

@SimenB @cpojer @thymikee please let me know if my understanding is correct.

[tushardhole]

As per above analysis, combination of this lint rule + this plugin would make it more seamless,

1. Plugin will make sure that assertion statement is always executed
2. Lint rule, will make sure that assertion statement result is honored by jest

[mattphillips]

@tushardhole I could be wrong here but I think the issue you are seeing is because you aren't marking the callback (test function) as asynchronous with either async/await or providing a done argument.

describe('async', () => {
  const getData = () => Promise.resolve('Hello');

  it('should fail as hello is returned when getting data (using done callback)', (done) => {
    getData().then((data) => {
      console.log("The expect will be executed")
      expect(data).toEqual("Hi");
      done();
    });
  });

  it('should fail as hello is returned when getting data (using async/await)', async () => {
    await getData().then((data) => {
      console.log("The expect will be executed")
      expect(data).toEqual("Hi");
    });
  });
});

[tushardhole]

@mattphillips that example works.

To conclude this as per my understanding, the plugin will work for this this issue, if the test is written using either async/await or providing a done.

In case a developer do not do this, which is possible as human error, this lint rule would be still helpful.

I do not want to be biased to get this PR merged, I feel still it makes sense to to have this lint rule and I would be interested to hear feedback from the community. 😄

If we all feel this lint rule is not required and the plugin with async/await is the way to go ahead, then no issues to close the PR.

[SimenB]

We've decided to move the linting package out of this repo, and into https://github.com/jest-community/eslint-plugin-jest. Could you please open this PR against that repo instead? Thanks!

[SimenB]

For people following along: jest-community/eslint-plugin-jest#42

[github-actions]

This pull request 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.