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

tushardhole · 2017-12-30T09:55:56Z

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

Summary
Implements #41

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

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

Limitations

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.

SimenB · 2018-01-05T10:28:59Z

CI is failing because of non-semantic commit message. See http://karma-runner.github.io/2.0/dev/git-commit-msg.html

SimenB · 2018-01-05T11:38:03Z

rules/__tests__/valid_expect_in_promise.test.js

+      'function() { /*fulfillment*/}, ' +
+      'function() { /*rejection*/ expect(someThing).toEqual(true)})})',
+
+    'it("it1", function() { return somePromise.then()})',


can you add assertions that await is ok without return?

I will work on that.

tushardhole · 2018-01-05T12:21:17Z

I will work on fixing non-semantic commit message.

introduce a lint rule to report error when testing promises. If a expectation is added inside a then or catch block of promise then as per jest docs, it is mandatory to return that promise for succesfull execution of that expectation. This rule will report all such expectations where promise is not returned.

tushardhole · 2018-01-10T13:08:29Z

@SimenB I have done the semantic commit message fix and a new commit to allow await expressions without return statement

SimenB

Needs docs

tushardhole · 2018-01-10T15:27:27Z

@SimenB doc changes done! Please review the same.

Also did some sanity on a project which uses promises and jest. Found one bug with nested then or catch, added unit test & fixed 😊

SimenB

I ran this against Jest's code base, and there are a couple of false positives.

arrow shorthand: https://github.com/facebook/jest/blob/3ffc99e02d4b1ecad9d0f5ec2e228bddc8e7210b/packages/jest-runtime/src/__tests__/runtime_module_directories.test.js#L57-L66
1. There are many cases here where it doesn't warn even though the tests use the same pattern. Why?
A promise which is created, but only returned at a later time: https://github.com/facebook/jest/blob/3ffc99e02d4b1ecad9d0f5ec2e228bddc8e7210b/packages/jest-haste-map/src/crawlers/__tests__/node.test.js#L103-L134

tushardhole · 2018-01-10T16:59:47Z

@SimenB thanks for that feedback! I will work on that.

tushardhole · 2018-01-13T06:23:36Z

@SimenB When i tried to run the rule against jest, the rule was not running at all, I believe following was the cause,

In line no 21 of .eslintrc.js in jest code base, it says to check only .md files, I.e. files: ['*.md'],
when I changed the line to below, the rule started reporting,
files: ['*.js', '*.md'],

To summarize I did following steps to test this rule agains jest code base,

Copied index.js and rules dir from eslint-plugin-jest to node_modules/eslint-plugin-jest/ of jest code base
Changed the block on line 20 in .eslintrc.js to this,

    {
      files: ['*.js', '*.md'],
      rules: {
        'consistent-return': 0,
        'flowtype/require-valid-file-annotation': 0,
        'import/no-extraneous-dependencies': 0,
        'import/no-unresolved': 0,
        "jest/valid-expect-in-promise": 2,
        'jest/no-focused-tests': 0,
        'jest/no-identical-title': 0,
        'no-undef': 0,
        'no-unused-vars': 0,
        'prettier/prettier': 0,
        'react/jsx-no-undef': 0,
        'react/react-in-jsx-scope': 0,
        'sort-keys': 0,
        'unicorn/filename-case': 0,
      },
    }

Here is the output,

/Users/tushard/OpenProjects/jest/packages/jest-haste-map/src/crawlers/__tests__/node.test.js
  103:11  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

/Users/tushard/OpenProjects/jest/packages/jest-runtime/src/__tests__/runtime_module_directories.test.js
  57:48  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

/Users/tushard/OpenProjects/jest/packages/jest-runtime/src/__tests__/runtime_require_mock.test.js
  133:58  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

/Users/tushard/OpenProjects/jest/packages/jest-runtime/src/__tests__/runtime_require_module.test.js
  134:56  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise
  159:54  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

Let me know your thoughts, we may need to add .js in .eslintrc.js of jest

SimenB · 2018-01-13T10:34:27Z

To test the rule in the Jest repo, you have to add the rule here: https://github.com/facebook/jest/blob/c799a0251d84a7e0c04d98e7d921a5a0e707c72f/packages/eslint-config-fb-strict/index.js#L50-L52

tushardhole · 2018-01-13T13:52:29Z

@SimenB Thanks, I will test the rule that way.

On other hand , I don't find the previousaly mentioned false positives in the output posted in my previous comment. Do you find that as the expectated output is or something is missing in that.

SimenB · 2018-01-13T15:40:58Z

On other hand , I don't find the previousaly mentioned false positives in the output posted in my previous comment.

They are in your paste, in particular

/Users/tushard/OpenProjects/jest/packages/jest-haste-map/src/crawlers/__tests__/node.test.js
  103:11  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

and

/Users/tushard/OpenProjects/jest/packages/jest-runtime/src/__tests__/runtime_module_directories.test.js
  57:48  error  Promise should be returned to test its fulfillment or rejection  jest/valid-expect-in-promise

SimenB · 2018-01-14T22:58:32Z

~~I'm having issues creating a test for the case of arrow shorthand, but I pushed up a test for one of the failing cases at least.~~

I was being dumb, two failing test cases which should be valid pushed

SimenB · 2018-01-14T23:03:37Z

rules/__tests__/valid_expect_in_promise.test.js

+    `
+     it('shorthand arrow', () =>
+       something().then(value => {
+         expect(() => {


this passes if I add return before expect (which is not needed)

I will work on this.

There are two more similar cases,

Where expect is inside a block,

test('invalid return', () => { const promise = something().then(value => { { expect(value).toBe('red'); } }); });

Where expect is inside another method,

test('invalid return', () => { const promise = something().then(value => { anotherMethod(); //this method contains the expect stament }); });

I have already added this limitation(#>2) in the PR description. I think these are rare scenarios, the first one may not occur at all. As mentioned earlier in the PR description, I think they are good to have can be done in next enhancement to this rule.
If this scenarios are must to have , then I am happy to implement in this PR itself.

SimenB · 2018-01-14T23:04:49Z

rules/__tests__/valid_expect_in_promise.test.js

+        'it("it1", function() { Promise.resolve().then(' +
+        'function() { /*fulfillment*/ expect(someThing).toEqual(true)}, ' +
+        'function() { /*rejection*/ expect(someThing).toEqual(true)})})',
+      errors: [


why is this 2 errors? It should just be one, there is only a single promise here

I will fix that.

SimenB · 2018-01-14T23:07:52Z

Another false positive is if I use the done callback.

test('callback in handler', done => {
 something().then(value => {
   expect(value).toBe('red');
   
   done();
  });
});

This shouldn't warn, but it's also weird usage of promises, so I think we should just not warn here. (if it's easy to solve, please do so)

tushardhole · 2018-01-15T12:16:23Z

This shouldn't warn, but it's also weird usage of promises, so I think we should just not warn here. (if it's easy to solve, please do so)

Yes I will do that. I think, this rule should just ignore processing if the test function have done parameter. Also i verified, the name of parameter can be anything not necessary to be named as done only.

~~Whether that done function is actually called or not, should not be verified by this rule. We can add a different rule for that like prefer-calling-done, thoughts ?~~

Verification of done getting called is not required, the test will fail if the function signature has done param, and test does not call it.
Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

tushardhole · 2018-01-17T03:52:25Z

@SimenB below feedback are fixed,

short hand arrow function ✅
later promise return ✅
done async callback ✅
duplicate errors for same promise ✅

SimenB

This is great, thank you!

SimenB · 2018-01-17T14:44:14Z

Ran it on the jest code base, and it actually found an error! jestjs/jest#5336

Great job!

tushardhole mentioned this pull request Dec 30, 2017

Lint rule to indicate missing return staement when testing inside promise then or catch block. #41

Closed

thymikee requested a review from SimenB January 2, 2018 21:34

SimenB reviewed Jan 5, 2018

View reviewed changes

tushardhole added 3 commits January 10, 2018 10:21

fix: do not add the rule to recommended

ab0f43c

fix: do not validate await expressions for valid_expect_in_promise

da393e0

tushardhole force-pushed the valid_expect_in_promise branch from 63da944 to da393e0 Compare January 10, 2018 13:05

SimenB requested changes Jan 10, 2018

View reviewed changes

SimenB mentioned this pull request Jan 10, 2018

Introduce a lint rule to report error when testing promises. If a exp… jestjs/jest#4844

Closed

tushardhole added 2 commits January 10, 2018 20:40

fix: scenario with expect in nested then/catch

dbba301

docs: updating docs for valid-expect-in-promise rule

4651b68

SimenB requested changes Jan 10, 2018

View reviewed changes

SimenB force-pushed the valid_expect_in_promise branch from 6f02ae9 to d1416c2 Compare January 14, 2018 23:01

test: add failing test case

5b25a69

SimenB force-pushed the valid_expect_in_promise branch from d1416c2 to 5b25a69 Compare January 14, 2018 23:03

SimenB reviewed Jan 14, 2018

View reviewed changes

fix: scenario where promise is returned later

8b6367d

tushardhole added 8 commits January 15, 2018 13:34

fix: adding more scenarios where promise is returned later

ede8427

test: adding more tests

8614010

fix: adding scenarios for non test functions

9abdd01

fix: refactor to avoid multiple execution of getTestFuncBody

3314dbf

fix: scenario with arrow-short-hand-fn with implicit return statement

00422dd

fix: scenario with multiline function in then block

1242897

fix: scenario with short hand arrow function in then block

0bd4572

fix: do not validate tests with done async param

4237828

tushardhole added 4 commits January 15, 2018 19:01

fix: scenario where expect in then is preceded by return

567b85c

fix: duplicate warning for same promise

5a69337

fix: better naming

7e44766

test: better formatting

e2dc02c

SimenB approved these changes Jan 17, 2018

View reviewed changes

SimenB merged commit eb31a54 into jest-community:master Jan 17, 2018

SimenB mentioned this pull request Jan 17, 2018

test: make sure to return promise from test jestjs/jest#5336

Merged

dependencies bot mentioned this pull request Jan 17, 2018

Update 42 dependencies from npm storybookjs/storybook#2770

Merged

dependencies bot mentioned this pull request Jan 31, 2018

Update eslint-plugin-jest in / from 21.5.0 to 21.7.0 severest/retrobot#11

Merged

SimenB mentioned this pull request Feb 3, 2018

feat(rule): add valid-describe rule #57

Merged

dependabot-preview bot mentioned this pull request Mar 14, 2018

Bump eslint-plugin-jest from 21.5.0 to 21.14.1 holvonix-misc/js-devbox#78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tushardhole commented Dec 30, 2017 •

edited

SimenB commented Jan 5, 2018

SimenB Jan 5, 2018

tushardhole Jan 5, 2018

tushardhole commented Jan 5, 2018

tushardhole commented Jan 10, 2018

SimenB left a comment

tushardhole commented Jan 10, 2018

SimenB left a comment •

edited

tushardhole commented Jan 10, 2018

tushardhole commented Jan 13, 2018 •

edited

SimenB commented Jan 13, 2018

tushardhole commented Jan 13, 2018

SimenB commented Jan 13, 2018

SimenB commented Jan 14, 2018 •

edited

SimenB Jan 14, 2018 •

edited

tushardhole Jan 15, 2018

tushardhole Jan 16, 2018

SimenB Jan 14, 2018

tushardhole Jan 15, 2018

SimenB commented Jan 14, 2018

tushardhole commented Jan 15, 2018 •

edited

tushardhole commented Jan 17, 2018 •

edited

SimenB left a comment

SimenB commented Jan 17, 2018

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

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

Conversation

tushardhole commented Dec 30, 2017 • edited

SimenB commented Jan 5, 2018

SimenB Jan 5, 2018

Choose a reason for hiding this comment

tushardhole Jan 5, 2018

Choose a reason for hiding this comment

tushardhole commented Jan 5, 2018

tushardhole commented Jan 10, 2018

SimenB left a comment

Choose a reason for hiding this comment

tushardhole commented Jan 10, 2018

SimenB left a comment • edited

Choose a reason for hiding this comment

tushardhole commented Jan 10, 2018

tushardhole commented Jan 13, 2018 • edited

SimenB commented Jan 13, 2018

tushardhole commented Jan 13, 2018

SimenB commented Jan 13, 2018

SimenB commented Jan 14, 2018 • edited

SimenB Jan 14, 2018 • edited

Choose a reason for hiding this comment

tushardhole Jan 15, 2018

Choose a reason for hiding this comment

tushardhole Jan 16, 2018

Choose a reason for hiding this comment

SimenB Jan 14, 2018

Choose a reason for hiding this comment

tushardhole Jan 15, 2018

Choose a reason for hiding this comment

SimenB commented Jan 14, 2018

tushardhole commented Jan 15, 2018 • edited

tushardhole commented Jan 17, 2018 • edited

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Jan 17, 2018

tushardhole commented Dec 30, 2017 •

edited

SimenB left a comment •

edited

tushardhole commented Jan 13, 2018 •

edited

SimenB commented Jan 14, 2018 •

edited

SimenB Jan 14, 2018 •

edited

tushardhole commented Jan 15, 2018 •

edited

tushardhole commented Jan 17, 2018 •

edited