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
feat(no-export): new rule for no-export #307
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an awesome rule, thanks for working on it! 🎉
I'm a bit worried about it forcing consumers to only apply it to test files, as it'll report false positives on production code. While not a huge problem, and it keeps the complexity of the rule implementation down, would it be a lot of work to make sure there's actually tests in the file? Might not be worth it, but if so I think it should be noted in its docs :)
(I've been thinking about adding some way of accessing jest config in rules, then we could check if the test would be run as such by Jest. Nothing concrete, though)
@SimenB actually not hard at all to check for a test and a great idea! I'll add that |
README.md
Outdated
@@ -114,6 +114,7 @@ installations requiring long-term consistency. | |||
| [no-commented-out-tests][] | Disallow commented out tests | | | | |||
| [no-duplicate-hooks][] | Disallow duplicate hooks within a `describe` block | | | | |||
| [no-empty-title][] | Disallow empty titles | | | | |||
| [no-export][] | Disallow export from test files | ![recommended][] | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not recommended (changing recommended rules is a breaking change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the process of getting it to be a recommended change? This rule prevents very hard to track down problems....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remembering when we release a new major :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll remove it for now, but I would like to move this to recommended in another PR, if you don't mind providing guidance on that process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Just open up a PR updating the readme and add it to the config exported from index
and I'll make sure to merge it before making a major release
Sometimes you need to add an empty Because of that, I think it would be helpful to let |
I'm down with an option ( |
can you merge @SimenB ? or give me permission to merge? :) |
I can do the allowEmpty option in another PR |
@SimenB any guidance you can provide on what it takes to get a rule to be 'recommended'? The bugs that come about are quite nefarious and time consuming to track down, especially for junior devs... |
@SimenB looks like the build failed on master. :( https://travis-ci.org/jest-community/eslint-plugin-jest/jobs/560608618 |
🎉 This PR is included in version 22.11.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Not much guidance needed, just add the badge to the readme and add the rule here: eslint-plugin-jest/src/index.js Lines 43 to 55 in b3a360d
I'll make sure to merge the PR for the major 🙂 |
I ran into this issue when we had a set of helper functions in 1 test file that
got exported directly from a
-test.js
file. The test that was importing thisfile then executes both it's own tests as well as the tests in the imported
file. We started seeing phantom snapshots show up that we weren't able to find
the origin of until we finally tracked it down and realized that a test file was
exporting shared functions. We fixed this by moving those functions out into
seperate files, but that was the inspiration of this rule.
This rule aims to eliminate duplicate runs of tests by exporting things from
test files. If you import from a test file, then all the tests in that file will
be run in each imported instance, so bottom line, don't export from a test, but
instead move helper functions into a seperate file when they need to be shared
across tests.