Add eslint rule `valid-expect` #3067

SimenB · 2017-03-05T10:22:20Z

Summary
Adds a new eslint rule to pick up invalid usage of expect.

All credits goes to @tlvince and @alecxe, this code is 100% copy paste, including docs and tests (only change is how context.report is called, but it's called with the same args). Open question is how to credit them in the source code, and what should be done about the license header?
https://github.com/tlvince/eslint-plugin-jasmine/blob/master/lib/rules/valid-expect.js

Motivation: Especially when converting tests from other assertions libs, it's easy to miss something. Jest reports false positives for some misuse of the api.

Test plan
Run yarn lint on some code that uses expect in an invalid way. There actually was a test in the jest code base giving a false positive.

The diff from pretty-format-test.js is a good example of the errors this rule picks up.

SimenB · 2017-03-05T10:36:16Z

/cc @jkimbo as well 😄

cpojer · 2017-03-06T11:31:09Z

packages/pretty-format/src/__tests__/pretty-format-test.js

-    expect(prettyFormat('\"-\"'), '"\\"-\\""');
-    expect(prettyFormat('\\ \\\\'), '"\\\\ \\\\\\\\"');
+    expect(prettyFormat('\"-\"')).toEqual('"\\"-\\""');
+    expect(prettyFormat('\\ \\\\')).toEqual('"\\\\ \\\\\\\\"');


And this is the point where you proved the value of this rule! Thanks.

@tlvince

All credits goes to @tlvince and @alecxe

jkimbo · 2017-03-06T11:46:17Z

packages/eslint-plugin-jest/README.md

@@ -50,6 +51,7 @@ You can also whitelist the environment variables provided by Jest by doing:
 - [no-disabled-tests](/packages/eslint-plugin-jest/docs/rules/no-disabled-tests.md) - disallow disabled tests.
 - [no-focused-tests](/packages/eslint-plugin-jest/docs/rules/no-focused-tests.md) - disallow focused tests.
 - [no-identical-title](/packages/eslint-plugin-jest/docs/rules/no-identical-title.md) - disallow identical titles.
+- [valid-expect](/packages/eslint-plugin-jest/docs/rules/valid-expect.md) - disallow identical titles.


The description here doesn't look right

Indeed! Copy paste error

I'm working on it right now.

SimenB · 2017-03-06T12:04:30Z

packages/eslint-plugin-jest/README.md

@@ -31,6 +31,7 @@ Then configure the rules you want to use under the rules section.
    "jest/no-disabled-tests": "warn",
    "jest/no-focused-tests": "error",
    "jest/no-identical-title": "error",
+    "jest/valid-expect": "error",


This line is a breaking change btw. If the next release is not a major, this should be dropped

cpojer · 2017-03-06T12:05:15Z

@SimenB I adjusted it a little bit, what do you think of that? Concretely I updated the message for expect(foo).toBeDefined. That rule now won't trigger if you don't have a matcher at all (like expect(…)) which already shows a different error, as well as it includes the name of the matcher. Does this make sense to you?

SimenB · 2017-03-06T12:05:23Z

packages/eslint-plugin-jest/src/rules/types.js

@@ -8,31 +8,37 @@
 * @flow
 */

-export type Identifier = {|
+type Node = MemberExpression | CallExpression;


I started doing this, but my flow-fu failed me. Didn't know it'd be as little as this!

SimenB · 2017-03-06T12:08:30Z

Looking at the changed assertions, that makes a lot of sense! Thank you for just blasting through and fixing the errors 😄

cpojer · 2017-03-06T12:56:04Z

Nice! Thanks @SimenB for building this.

SimenB · 2017-03-06T14:50:46Z

docs/SetupAndTeardown.md

@@ -93,11 +93,11 @@ describe('matching cities to foods', () => {
  });

  test('Vienna <3 sausage', () => {
-    expect(isValidCityFoodPair('Vienna', 'Wiener Schnitzel'));
+    expect(isValidCityFoodPair('Vienna', 'Wiener Schnitzel')).toBe(true);


@tlvince

* Add eslint rule `valid-expect` All credits goes to @tlvince and @alecxe * Update valid-expect.md * Update valid-expect-test.js * Update valid-expect-test.js * Update valid-expect.js * Minor fixes and cleanups. * Fix lint errors.

@tlvince

* Add eslint rule `valid-expect` All credits goes to @tlvince and @alecxe * Update valid-expect.md * Update valid-expect-test.js * Update valid-expect-test.js * Update valid-expect.js * Minor fixes and cleanups. * Fix lint errors.

github-actions · 2021-05-13T21:06:56Z

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.

SimenB force-pushed the valid-expect branch from 5aa2492 to dab6c1e Compare March 5, 2017 10:23

facebook-github-bot added the CLA Signed ✔️ label Mar 5, 2017

cpojer reviewed Mar 6, 2017

View reviewed changes

SimenB and others added 5 commits March 6, 2017 11:45

Add eslint rule valid-expect

1cfc707

All credits goes to @tlvince and @alecxe

Update valid-expect.md

d02f97c

Update valid-expect-test.js

294ceda

Update valid-expect-test.js

dec6d5d

Update valid-expect.js

2ec914d

jkimbo reviewed Mar 6, 2017

View reviewed changes

Minor fixes and cleanups.

de36add

cpojer force-pushed the valid-expect branch from 1dbc71c to de36add Compare March 6, 2017 12:04

SimenB commented Mar 6, 2017

View reviewed changes

Fix lint errors.

50f97f8

cpojer merged commit e886064 into jestjs:master Mar 6, 2017

SimenB deleted the valid-expect branch March 6, 2017 14:49

SimenB commented Mar 6, 2017

View reviewed changes

SimenB mentioned this pull request Mar 6, 2017

Support failing a test if no assertions are ran #2209

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eslint rule `valid-expect` #3067

Add eslint rule `valid-expect` #3067

SimenB commented Mar 5, 2017 •

edited

Loading

SimenB commented Mar 5, 2017

cpojer Mar 6, 2017

jkimbo Mar 6, 2017

SimenB Mar 6, 2017

cpojer Mar 6, 2017

SimenB Mar 6, 2017

cpojer commented Mar 6, 2017

SimenB Mar 6, 2017

SimenB commented Mar 6, 2017 •

edited

Loading

cpojer commented Mar 6, 2017

SimenB Mar 6, 2017

github-actions bot commented May 13, 2021

Add eslint rule valid-expect #3067

Add eslint rule valid-expect #3067

Conversation

SimenB commented Mar 5, 2017 • edited Loading

SimenB commented Mar 5, 2017

cpojer Mar 6, 2017

Choose a reason for hiding this comment

jkimbo Mar 6, 2017

Choose a reason for hiding this comment

SimenB Mar 6, 2017

Choose a reason for hiding this comment

cpojer Mar 6, 2017

Choose a reason for hiding this comment

SimenB Mar 6, 2017

Choose a reason for hiding this comment

cpojer commented Mar 6, 2017

SimenB Mar 6, 2017

Choose a reason for hiding this comment

SimenB commented Mar 6, 2017 • edited Loading

cpojer commented Mar 6, 2017

SimenB Mar 6, 2017

Choose a reason for hiding this comment

github-actions bot commented May 13, 2021

Add eslint rule `valid-expect` #3067

Add eslint rule `valid-expect` #3067

SimenB commented Mar 5, 2017 •

edited

Loading

SimenB commented Mar 6, 2017 •

edited

Loading