Skip to content

Commit

Permalink
feat(valid-expect-in-promise): re-implement rule (#916)
Browse files Browse the repository at this point in the history
* feat(valid-expect-in-promise): re-write implementation

* fix(valid-expect-in-promise): check number of arguments being passed

* fix(valid-expect-in-promise): bailout if `done` callback is present

* fix(valid-expect-in-promise): allow awaited promises

* fix(valid-expect-in-promise): handle multi-chained promises properly

Method call chains are represented in AST in a nested fashion, meaning
it's not enough to just separately track if we're both in a promise
chain call and then if we've found an `expect` call, because when we
exit another CallExpression in the same chain it'll look like that has
an `expect` call.

Instead, we need to track our depth as we enter CallExpression nodes so
that when we exit those nodes we can check if we encountered an `expect`
 call at that same depth.

* fix(valid-expect-in-promise): handle promises assigned to variables

* refactor(valid-expect-in-promise): adjust conditions to dedent code

* fix(valid-expect-in-promise): allow variables assigned awaited promises

* feat(valid-expect-in-promise): track promise vars across reassignments

* feat(valid-expect-in-promise): support blocks and multi-variable assigns

* feat(valid-expect-in-promise): support re-assignment promise chaining

* refactor(valid-expect-in-promise): remove unneeded condition

* refactor(valid-expect-in-promise): inline reporting function

We only call it in one place, and so it lets us also remove some minor
types that can be otherwise inferred

* fix(valid-expect-in-promise): rewrite rule message to be clearer

* fix(valid-expect-in-promise): rewrite rule description to be clearer

* fix(valid-expect-in-promise): ignore unreachable code after return

* fix(valid-expect-in-promise): support `Promise.all`

* fix(valid-expect-in-promise): support `resolve` & `reject` methods

* refactor(valid-expect-in-promise): remove unneeded optional chain

* refactor(valid-expect-in-promise): adjust conditions slightly

* chore(valid-expect-in-promise): add a bunch of comments

* feat(valid-expect-in-promise): support `Promise.allSettled`

* docs(valid-expect-in-promise): add more examples and reword
  • Loading branch information
G-Rath authored Oct 9, 2021
1 parent a4f66f6 commit 7a49c58
Show file tree
Hide file tree
Showing 4 changed files with 1,335 additions and 179 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ installations requiring long-term consistency.
| [require-top-level-describe](docs/rules/require-top-level-describe.md) | Require test cases and hooks to be inside a `describe` block | | |
| [valid-describe](docs/rules/valid-describe.md) | Enforce valid `describe()` callback | ![recommended][] | |
| [valid-expect](docs/rules/valid-expect.md) | Enforce valid `expect()` usage | ![recommended][] | |
| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Enforce having return statement when testing with promises | ![recommended][] | |
| [valid-expect-in-promise](docs/rules/valid-expect-in-promise.md) | Ensure promises that have expectations in their chain are valid | ![recommended][] | |
| [valid-title](docs/rules/valid-title.md) | Enforce valid titles | ![recommended][] | ![fixable][] |

<!-- end base rules list -->
Expand Down
69 changes: 55 additions & 14 deletions docs/rules/valid-expect-in-promise.md
Original file line number Diff line number Diff line change
@@ -1,31 +1,72 @@
# Enforce having return statement when testing with promises (`valid-expect-in-promise`)
# Ensure promises that have expectations in their chain are valid (`valid-expect-in-promise`)

Ensure to return promise when having assertions in `then` or `catch` block of
promise
Ensure promises that include expectations are returned or awaited.

## Rule details

This rule looks for tests that have assertions in `then` and `catch` methods on
promises that are not returned by the test.
This rule flags any promises within the body of a test that include expectations
that have either not been returned or awaited.

### Default configuration

The following pattern is considered warning:
The following patterns is considered warning:

```js
it('promise test', () => {
somePromise.then(data => {
expect(data).toEqual('foo');
it('promises a person', () => {
api.getPersonByName('bob').then(person => {
expect(person).toHaveProperty('name', 'Bob');
});
});

it('promises a counted person', () => {
const promise = api.getPersonByName('bob').then(person => {
expect(person).toHaveProperty('name', 'Bob');
});

promise.then(() => {
expect(analytics.gottenPeopleCount).toBe(1);
});
});

it('promises multiple people', () => {
const firstPromise = api.getPersonByName('bob').then(person => {
expect(person).toHaveProperty('name', 'Bob');
});
const secondPromise = api.getPersonByName('alice').then(person => {
expect(person).toHaveProperty('name', 'Alice');
});

return Promise.any([firstPromise, secondPromise]);
});
```

The following pattern is not warning:

```js
it('promise test', () => {
return somePromise.then(data => {
expect(data).toEqual('foo');
it('promises a person', async () => {
await api.getPersonByName('bob').then(person => {
expect(person).toHaveProperty('name', 'Bob');
});
});

it('promises a counted person', () => {
let promise = api.getPersonByName('bob').then(person => {
expect(person).toHaveProperty('name', 'Bob');
});

promise = promise.then(() => {
expect(analytics.gottenPeopleCount).toBe(1);
});

return promise;
});

it('promises multiple people', () => {
const firstPromise = api.getPersonByName('bob').then(person => {
expect(person).toHaveProperty('name', 'Bob');
});
const secondPromise = api.getPersonByName('alice').then(person => {
expect(person).toHaveProperty('name', 'Alice');
});

return Promise.allSettled([firstPromise, secondPromise]);
});
```
Loading

0 comments on commit 7a49c58

Please sign in to comment.