Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create no-conditional-expect & deprecate no-try-expect #588

Merged
merged 2 commits into from Jun 21, 2020

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented May 17, 2020

Closes #482

I originally was playing around with actually not caring if the expect was in a test, since overall it doesn't really matter where the expect is, it's still best to not have it in a condition, which worked nicely except it would require more logic for handling early returns and early throws.

I think it's doable, but adds more complexity than needed right now, as it started heavily crossing into the territory of other rules like no-test-return-statement.

@G-Rath G-Rath requested a review from SimenB May 17, 2020 08:50
@G-Rath
Copy link
Collaborator Author

G-Rath commented May 17, 2020

@SimenB I'm wondering if no-if should be renamed to no-conditionals-in-tests, or better yet merged into no-conditional-expect as an option: disallowAnyConditionals.

While the rule name wouldn't quite match the behaviour when the option is enabled, implementation wise it should be a case of just doing option || isExpectCall(node) in the if condition.

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 17, 2020

ESLint 5 is apparently not having a good time in CI:

FAIL lint src/rules/utils.ts
  ● Test suite failed to run

    Failed to load plugin 'eslint-plugin' declared in '../../home/runner/work/eslint-plugin-jest/eslint-plugin-jest/.eslintrc.js': Cannot find module 'eslint-plugin-eslint-plugin'
    Require stack:
    - /tmp/eslint-plugin-jest-jkGeOe/__placeholder__.js
    Referenced from: /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/.eslintrc.js

      at Object.resolve (node_modules/jest-runner-eslint/node_modules/eslint/lib/shared/relative-module-resolver.js:44:50)
          at _normalizeObjectConfigDataBody.next (<anonymous>)
          at _normalizeObjectConfigData.next (<anonymous>)

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 17, 2020

Oh and I'm also thinking we should at some point move the assertFunctionNames option from expect-expect to settings, so that other rules (such as this one) can make use of it.

@G-Rath G-Rath force-pushed the create-no-conditional-expect branch 3 times, most recently from 88cdc94 to a8feb64 Compare May 17, 2020 10:01
@G-Rath G-Rath force-pushed the create-no-conditional-expect branch 2 times, most recently from a879820 to d0c6646 Compare June 21, 2020 04:39
@G-Rath G-Rath merged commit 6d07cad into master Jun 21, 2020
@G-Rath G-Rath deleted the create-no-conditional-expect branch June 21, 2020 07:06
@github-actions
Copy link

🎉 This PR is included in version 23.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@janvennemann
Copy link

@G-Rath this breaks on of our builds after updating from 23.13.2 to 23.16.0. Not really a big deal since its easily fixable, I was just wondering if it was intended to add a new rule to the recommended configuration that breaks existing setups with a feature update? Shouldn't this be considered a breaking change?

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jun 23, 2020

@janvennemann yes changing the recommended ruleset is considered a breaking change, which is why this wasn't added to the recommended ruleset.

Would you be able to share your eslint config and more details on your setup, and the errors you got?

@janvennemann
Copy link

@G-Rath Duh, for whatever reason we had both all & recommended in our config, so that explains it. As expected removing all solves our issue. Guess i should have taken a closer look before bothering you, sorry ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rename no-try-expect to no-conditional-expect and add support for if
2 participants