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

Provide unit testing devDependencies from wp-scripts #17016

Closed
iandunn opened this issue Aug 12, 2019 · 11 comments · Fixed by #17027
Closed

Provide unit testing devDependencies from wp-scripts #17016

iandunn opened this issue Aug 12, 2019 · 11 comments · Fixed by #17027
Assignees
Labels
[Package] Scripts /packages/scripts [Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling

Comments

@iandunn
Copy link
Member

iandunn commented Aug 12, 2019

Problem

wp-scripts provides some of the framework necessary to unit-test plugins, but not all. One specific piece that's missing is many of the dev-dependencies still need to be added to the plugin's package.json. For example, https://github.com/WordPress/wordcamp.org/pull/211/files.

Proposed Solution

Those dependencies (and any others) should be added to wp-scripts's package.json, so that plugins inherit them automatically.

cc @gziolo , who I think is planning to work on some related issues.

@iandunn iandunn added [Type] Enhancement A suggestion for improvement. [Type] Build Tooling Issues or PRs related to build tooling [Package] Scripts /packages/scripts labels Aug 12, 2019
@gziolo gziolo added [Type] Bug An existing feature does not function as intended and removed [Type] Enhancement A suggestion for improvement. labels Aug 13, 2019
@gziolo
Copy link
Member

gziolo commented Aug 13, 2019

Yes, I opened to WordPress/gutenberg-examples#84 to investigate further and I wasn't happy about the result. There are a few things that might go smoother so overall I consider it as a bug as it might be painful to write a single test with linting enabled :)

Known Issues

When running npm run lint-js I see the following errors:

/Users/gziolo/PhpstormProjects/gutenberg-examples/test/examples.js
  10:1  error  'test' is not defined    no-undef
  11:2  error  'expect' is not defined  no-undef
  14:1  error  'test' is not defined    no-undef
  15:2  error  'expect' is not defined  no-undef

✖ 4 problems (4 errors, 0 warnings)

To solve it I had to add:

+ /* global test, expect */

at the top of the file which should work out of the box. This should work out of the box when using @wordpress/scripts.

I also had to put:

+ /**
+  * WordPress dependencies
+  */
import { Button } from '@wordpress/components';

above the import statement to fix the following issue:

 1:1  error  Expected preceding "WordPress dependencies" comment block  @wordpress/dependency-group

which I don't think we want to enforce outside of Gutenberg.

WordPress/gutenberg-examples#84 (comment)

It looks like react-test-renderer is a very deep dependency of @wordpress/scripts. We should ensure it has tighter integration so it works out of the box in all cases.

WordPress/gutenberg-examples#84 (review)

I think that having @wordpress/components as a dependency brings @wordpress/element as well. Ideally, we bring it as part of @wordpress/babel-preset-default which is the default handler for React regardless.

@gziolo
Copy link
Member

gziolo commented Aug 13, 2019

I started #17027 and I think I will be able to fix all the issue. react-test-renderer is probably something we should handle separately. I will open another issue for it as technically you don't need it for tests but in reality, you will rather want to use :D

@iandunn
Copy link
Member Author

iandunn commented Aug 15, 2019

This might need to be a different issue, but it'd be awesome to also provide all of the @wordpress/* packages as well, so that I don't have to, e.g., add @wordpress/api-fetch to my devDependencies if I want to test a component that uses it.

I tried just adding @wordpress/block-editor, hoping that that would pull in most of the components from its own devDependencies, but that didn't work.

@iandunn
Copy link
Member Author

iandunn commented Aug 15, 2019

Er, api-fetch was a bad example, since that'd probably be better suited for an e2e test, but the same dependency hell comes up when trying to use i18n or anything else. I have to:

  1. npm run test source/blocks/schedule/
  2. scroll back to see the Cannot find module '@wordpress/i18n' ... error
  3. add @wordpress/i18n to my devDependencies
  4. npm i @wordpress/i18n && npm run test source/blocks/schedule/
  5. repeat those steps for every package, even stuff like react-autosize-textarea that I'm not using directly in my component.

@iandunn
Copy link
Member Author

iandunn commented Aug 15, 2019

Er, part of my problem above was that I was accidentally importing../ instead of ../{file}, so it was loading more than it should have.

It does seem like there's still a need to import things like @wordpress/components, @wordpress/element, etc, though.

@gziolo
Copy link
Member

gziolo commented Aug 23, 2019

This might need to be a different issue, but it'd be awesome to also provide all of the @wordpress/* packages as well, so that I don't have to, e.g., add @wordpress/api-fetch to my devDependencies if I want to test a component that uses it.

I tried just adding @wordpress/block-editor, hoping that that would pull in most of the components from its own devDependencies, but that didn't work.

Shouldn’t you put them as regular dependencies? If you are testing them, I would assume that you are using them in your codebase as well.

In fact, I double checked the link you shared and it looks like all dependencies which I see in the components aren’t properly installed in package.json file:
https://github.com/WordPress/wordcamp.org/blob/8faf82a988aa7a0cd120baad7583c813e945a990/public_html/wp-content/mu-plugins/blocks/package.json

This might be confusing a bit but everything you import in the code that gets shipped to the user should be added to regular (aka production) dependencies section. This is expected strategy when working with npm. We convert all the dependencies which WordPress uses to wp globals so but for unit tests we need sources of those packages.

Honestly, I don’t know what would be the best strategy here. If you want to explore it further let’s open new issue and use The weekly Core JS chat to gather more feedback. In the meantime, at least @wordpress/element will get installed by default as a deep dependency of the package when we publish the next version of @wordpress/scripts.

@gziolo
Copy link
Member

gziolo commented Aug 23, 2019

It looks like react-test-renderer is a very deep dependency of @wordpress/scripts. We should ensure it has tighter integration so it works out of the box in all cases.

I’ll open a new issue early next week which will propose some better handling for component testing which installs react-test-renderer behind the scenes.

@iandunn
Copy link
Member Author

iandunn commented Aug 23, 2019

If you are testing them, I would assume that you are using them in your codebase as well.

They're not being used directly in any custom code, but they're dependencies of things the custom code imports. Part of the difference in expectations may be that I'm using mount() to do integration testing, rather than shallow() / unit testing. I should have made that distinction more clear in the title, rather than misusing "unit".

@gziolo
Copy link
Member

gziolo commented Aug 26, 2019

Okay, I see them listed as dev dependencies here:
https://github.com/WordPress/wordcamp.org/blob/217d4f2df01b1e25ba83193542afc7f6773a9903/public_html/wp-content/mu-plugins/blocks/package.json#L27-L33

However, I also see them used in the actual code:
https://github.com/WordPress/wordcamp.org/blob/217d4f2df01b1e25ba83193542afc7f6773a9903/public_html/wp-content/mu-plugins/blocks/source/components/post-list/no-content.js#L6-L10

So my previous statement still holds, they should be listed as regular dependencies even though you technically don't need them to build your plugin when using wp-scripts which treats them as externals because Gutenberg provides them through wp global.

@iandunn
Copy link
Member Author

iandunn commented Aug 27, 2019

Ah, you're right, most of them are being used. I think I've found two that aren't though: react-autosize-textarea and traverse.

@gziolo
Copy link
Member

gziolo commented Aug 28, 2019

Ah, you're right, most of them are being used. I think I've found two that aren't though: react-autosize-textarea and traverse.

I think you had to add them because we had tons of configuration issues in the list of dependencies. I fixed all (hopefully) of them in #16969. I see I added both dependencies you listed in this PR. We need another release of packages and I hope this won't be an issue anymore after you bump versions of WordPress dependencies. I should perform the npm release tomorrow if the next version of Gutenberg is successfully released today.

@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Aug 29, 2019
iandunn added a commit to WordPress/wordcamp.org that referenced this issue Aug 29, 2019
These aren't needed to run the code, because they exist in the `wp` namespace as externals, but they are needed for unit tests, since they don't otherwise exist in that context.

See WordPress/gutenberg#17016 (comment).
iandunn added a commit to WordPress/wordcamp.org that referenced this issue Aug 30, 2019
These aren't needed to run the code, because they exist in the `wp` namespace as externals, but they are needed for unit tests, since they don't otherwise exist in that context.

See WordPress/gutenberg#17016 (comment).
iandunn added a commit to WordPress/wordcamp.org that referenced this issue Sep 3, 2019
These aren't needed to run the code, because they exist in the `wp` namespace as externals, but they are needed for unit tests, since they don't otherwise exist in that context.

See WordPress/gutenberg#17016 (comment).
iandunn added a commit to WordPress/wordcamp.org that referenced this issue Sep 4, 2019
These aren't needed to run the code, because they exist in the `wp` namespace as externals, but they are needed for unit tests, since they don't otherwise exist in that context.

See WordPress/gutenberg#17016 (comment).
iandunn added a commit to WordPress/wordcamp.org that referenced this issue Sep 18, 2019
These aren't needed to run the code, because they exist in the `wp` namespace as externals, but they are needed for unit tests, since they don't otherwise exist in that context.

See WordPress/gutenberg#17016 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Scripts /packages/scripts [Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants