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

Add eslint-plugin-jest to @wordpress/scripts #17061

Closed
ntwb opened this issue Aug 16, 2019 · 13 comments · Fixed by #17027
Closed

Add eslint-plugin-jest to @wordpress/scripts #17061

ntwb opened this issue Aug 16, 2019 · 13 comments · Fixed by #17027
Assignees
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward [Tool] WP Scripts /packages/scripts

Comments

@ntwb
Copy link
Member

ntwb commented Aug 16, 2019

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

In the process of updating stylelint-config-wordpress to use @wordpress/scripts ESLint reports a bunch of no-undef errors for the test files in the /__tests__/ folder, for example:

/Users/netweb/Code/WordPress/stylelint-config-wordpress/__tests__/index.test.js
   9:1  error  'describe' is not defined    no-undef
  12:2  error  'beforeEach' is not defined  no-undef
  19:2  error  'it' is not defined          no-undef
  21:4  error  'expect' is not defined      no-undef

Describe the solution you'd like
A clear and concise description of what you want to happen.

The @wordpress/scripts package already includes Jest for running unit and e2e tests and as such I would expect that writing tests with Jest should include the ESLint Jest rules also.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

The alternate solution is to install eslint-plugin-jest in the stylelint-config-wordpress repo instead and add an ESLint configuration.


The #17027 PR comes close to solving the above solution by adding a new ESLint ruleset @wordpress/eslint-plugin/test-unit that includes Jest.

The issue with that as it currently stands is it does not inherit the current @wordpress/eslint-plugin/recommended ruleset.

cc @gziolo

@gziolo
Copy link
Member

gziolo commented Aug 16, 2019

The issue with that as it currently stands is it does not inherit the current @wordpress/eslint-plugin/recommended ruleset.

In #17033, there is change to the recommended config included as well. It adds all Jest related rules to all files which Jest would match as test files. So it sort of works the other way around than you propose. I has the same issue when trying to add tests to Gutenberg examples repository here: WordPress/gutenberg-examples#84.

@ntwb
Copy link
Member Author

ntwb commented Aug 16, 2019

In #17033, there is change to the recommended config included as well. It adds all Jest related rules to all files which Jest would match as test files.

Thanks, but I don't see any changes to the "recommended config" in this PR

@gziolo
Copy link
Member

gziolo commented Aug 16, 2019

I meant changes highlighted here: #17027 (comment). I shared a wrong PR 🙃

@gziolo
Copy link
Member

gziolo commented Aug 23, 2019

I hope to release the next major version of @wordpress/scripts next Wednesday/Thursday which should fix it. Let’s reopen only in case when it doesn’t work as expected afterward.

@gziolo
Copy link
Member

gziolo commented Aug 29, 2019

@ntwb - new major version of @wordpress/scripts is out but it doesn't work for test files out of the box and I can't figure out why. If I copy https://github.com/WordPress/gutenberg/blob/master/packages/scripts/config/.eslintrc.js to the root of my project it works perfectly fine. There might be some bug in Eslint with how overrides are resolved inside config files located in node_modules folder. Can you double-check if it works for you?

Related PR where I test it: WordPress/gutenberg-examples#87

@gziolo gziolo reopened this Aug 29, 2019
@gziolo gziolo self-assigned this Aug 29, 2019
@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Aug 29, 2019
@gziolo
Copy link
Member

gziolo commented Sep 17, 2019

I filed a new issue in ESLint: eslint/eslint#12278.

@gziolo gziolo added [Status] Blocked Used to indicate that a current effort isn't able to move forward and removed [Status] In Progress Tracking issues with work in progress labels Sep 17, 2019
@gziolo
Copy link
Member

gziolo commented Sep 19, 2019

It looks like the fix in ESLint is going to be breaking change and would eventually land in 7.0: eslint/rfcs#37. We will need a temporary fix. I think it's fine to always enable configs for unit and e2e tests in @wordpress/scripts for the time being.

@ntwb
Copy link
Member Author

ntwb commented Sep 19, 2019

Not sure on the timeline but there is broad support for a rework of the ESLint configuration as outlined in eslint/rfcs#9

That, if agreed to would also land in a major release of ESLint, possibly 7.0

@gziolo
Copy link
Member

gziolo commented Sep 19, 2019

It only confirms my assumption that we need to have a working solution in the meantime as it might take months before we see 7.0 :)

@gziolo
Copy link
Member

gziolo commented Oct 3, 2019

A temporary workaround is ready with #17744. I don't think it makes sense to wait until ESLint 7.0 is out as it might take some time.

@gziolo
Copy link
Member

gziolo commented Oct 3, 2019

Should we close it now that #17744 was merged? We will be able to revisit it after eslint/rfcs#37 is implemented but it might take months.

@ntwb
Copy link
Member Author

ntwb commented Oct 3, 2019

Yup, closing 👍🏼

@ntwb ntwb closed this as completed Oct 3, 2019
@gziolo
Copy link
Member

gziolo commented Mar 23, 2020

The upstream issue was fixed eslint/eslint#12887 in ESLint but we need until v7.0.0 is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Blocked Used to indicate that a current effort isn't able to move forward [Tool] WP Scripts /packages/scripts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants