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.

Sign up for GitHub

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

Jump to bottom

Tests: Improve configuration and mocking strategy #8188

Merged
merged 4 commits into from
Jul 26, 2018
Merged

Tests: Improve configuration and mocking strategy #8188

merged 4 commits into from
Jul 26, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jul 25, 2018 •
edited
Loading

Description

I encountered some issues with our current test setup when trying to debug failing tests in #8189.

This PR tries to improve our test configuration by:

  • Removing global mocks which are enabled by default for every test suite. Instead, we enable all mocks explicitly using jest.mock calls.
  • Fixing moduleNameMapper to always use source code rather than build files.
  • Removing no longer needed wp global aliasing in the test setup.
  • Removes __mocks__ folder from the build output for all packages.

We should tackle #7005 next. It should be easier to identify tests which need to be updated since we can easily identify tests that depend on Button by searching for jest.mock call.

How has this been tested?

  • npm run prebuild:packages - to ensure there are no build files created for production code.
  • npm test - to make sure all tests pass.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jul 25, 2018
@gziolo gziolo added this to the 3.4 milestone Jul 25, 2018
@gziolo gziolo self-assigned this Jul 25, 2018
@youknowriad
Copy link
Contributor

Kind of unrelated, but can I ask why we need to mock the "Button" component, it's not really clear to me?

@gziolo gziolo changed the title Tests: Improve configuration to use mocks explicitly Tests: Improve configuration and mocking strategy Jul 25, 2018
@gziolo
Copy link
Member Author

gziolo commented Jul 25, 2018

Kind of unrelated, but can I ask why we need to mock the "Button" component, it's not really clear to me?

See #7005 and #6527. In the latter, we wrapped Button with forwardRef which doesn't work with enzyme so we had to introduce such workaround. I hope to tackle it next week.

@gziolo gziolo mentioned this pull request Jul 25, 2018
Merged
4 tasks
@gziolo gziolo requested a review from tofumatt July 25, 2018 10:44
tofumatt
tofumatt approved these changes Jul 25, 2018
View reviewed changes
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I dig it and this is nicer, but those imports are quite something.

I filed #8190 to deal with them on a broader scale, but I'd really like it if the imports in here were absolute. Right now it's really tough to figure out where those files actually are located.

Code looks good though, just minor things to fix and then merge!

edit-post/components/header/more-menu/test/index.js
@@ -8,6 +8,8 @@ import { mount } from 'enzyme';
*/
import MoreMenu from '../index';

jest.mock( '../../../../../packages/components/src/button' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a nigh-unusable import path? I know there's discussion around just using absolute paths everywhere (I brought it up in a chat and should really file an issue... so: #8190).

But this should be absolute if possible. It's really unusable to try to figure out what this is importing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it took me some time to build them properly 😅

In the first place, I want to get rid of those mocks completely with #7005 which will sort of resolve this issue :D

packages/components/src/button/__mocks__/index.js
@@ -0,0 +1,17 @@
// [TEMPORARY]: Button uses React.forwardRef, added in [email protected] but not yet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the docs! 👍

__mocks__/@wordpress/api-fetch/index.js Outdated
const apiFetch = jest.fn( () => {
return apiFetch.mockReturnValue;
} );
apiFetch.mockReturnValue = 'mock this value by overriding apiFetch.mockReturnValue';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make this a full sentence? 'Mock this value by overriding apiFetch.mockReturnValue.'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it over - can tweak 👍

@gziolo gziolo merged commit 39a04fa into master Jul 26, 2018
@gziolo gziolo deleted the update/improve-unit-tests branch July 26, 2018 10:03
aduth
aduth reviewed Jul 28, 2018
View reviewed changes
package.json
@@ -144,8 +144,8 @@
"scripts": {
"prebuild": "npm run check-engines",
"clean:packages": "rimraf ./packages/*/build ./packages/*/build-module ./packages/*/build-style",
"prebuild:packages": "npm run clean:packages && lerna run build && cross-env INCLUDE_PACKAGES=babel-plugin-import-jsx-pragma,postcss-themes SKIP_JSX_PRAGMA_TRANSFORM=1 node ./bin/packages/build.js",
"build:packages": "cross-env EXCLUDE_PACKAGES=babel-plugin-import-jsx-pragma,postcss-themes node ./bin/packages/build.js",
"prebuild:packages": "npm run clean:packages && lerna run build && cross-env INCLUDE_PACKAGES=babel-plugin-import-jsx-pragma,postcss-themes,jest-console SKIP_JSX_PRAGMA_TRANSFORM=1 node ./bin/packages/build.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we including these other packages in the prebuild?

Copy link
Member Author

@gziolo gziolo Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • postcss-themes is necessary to build CSS styles for packages, so we need its build version upfront
  • jest-console doesn't strictly need to be there; however, I put it there to be able to run npm run prebuild:package && npm test and verify that packages can be tested without transpiled code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@tofumatt tofumatt tofumatt approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

@ntwb ntwb Awaiting requested review from ntwb

@nerrad nerrad Awaiting requested review from nerrad

Assignees

@gziolo gziolo

Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
4 participants
@gziolo @youknowriad @tofumatt @aduth