Testing: Add integration test which ensures that npm packages can be used with Node

[gziolo]

This should prevent bugs like #17165 where a naked reference to window will crash if used in a SSR context where there's no window.

See more details in the comment from @jsnajdr #17165 (comment):

In this case, the easiest TDD-style test would be to simply import @wordpress/compose in a Node.js environment without DOM. And it would fail.

Neither compose nor components packages use the sideEffects: false flag (although they would be a good fit IMO), so using anything from compose bundles the whole library.

And the window check is a top-level statement in the module. We don't need to use or instantiate the useReducedMotion hook at all to get a broken build.

[mpark86]

Hi, I'm new to this project and this issue kind of intrigued me. I really would like to try this one but I'm not that familiar with testing. Are there any instructions or guides on how integration testing works for this project?

[gziolo]

We have the Testing Overview document.

You can put tests under:
https://github.com/WordPress/gutenberg/tree/master/test/integration

The file(s) should have .test.js extension.

In this particular case, it's necessary to change the testing environment for the individual test. It can be done with:

By adding a @jest-environment docblock at the top of the file, you can specify another environment to be used for all tests in that file:

/**
 * @jest-environment node
 */

test('use node in this test file', () => {
  // test's body
});

Based on Jest documentation available at https://jestjs.io/docs/en/configuration#testenvironment-string.

[mpark86]

Thank you. I'll start by going through those docs.

[simison]

Would it be possible to lint for unguarded browser globals with Eslint?

Would simply defining env: { node: true, browser: false } as Eslint config for packages work?

[gziolo]

Would it be possible to lint for unguarded browser globals with Eslint?

Would simply defining env: { node: true, browser: false } as Eslint config for packages work?

You can give it a try. I have no idea, to be honest.

How about using node specific APIs with such setup? Wouldn't it make it possible to use path or fs in the code which is intended to run in the browser?

[jsnajdr]

How about using node specific APIs with such setup? Wouldn't it make it possible to use path or fs in the code which is intended to run in the browser?

What we want to do is to check that the code can run both in Node and browser. I.e., use only globals and API that are valid on both platforms. Platform-specific code should be guarded (typeof window !== 'undefined'...)

For React components, there's one additional gotcha: it's ok to work with DOM in "effects":

componentDidMount() {
  window.addEventListener( 'scroll', this.handleScroll );
}

or

useEffect( () => {
  ReactDom.findDomNode( this.cartRef ).scrollIntoView();
} );

When server-side rendering, only constructors, componentWillMount and render are ever called in class components, and effects are never executed.

I don't know how to lint for that -- maybe eslint-plugin-react has some useful rules? @ljharb might know.

[ljharb]

There's not much linting that can do here - typically what's done is that you have two enzyme test runs: one with jsdom, and one without - and all your shallow tests (that each component should have) are evaluated in that context.

mount should be reserved for the special/minority cases that can't be tested with shallow.

[gziolo]

In the announcement post for Jest 26 (https://jestjs.io/blog/2020/05/05/jest-26), they shared the roadmap for upcoming breaking changes including:

Jest 28 will remove jest-jasmine2 and jest-environment-jsdom from the default distribution of Jest. The packages will still be actively maintained as part of the Jest project and be published separately. Users will need to install these packages to use them.

It looks like we should change the default env to node earlier and it should help to catch issues like the one discussed as well.