Migrate to Jest for JS unit testing #437

swissspidy · 2019-08-23T12:39:04Z

Summary

While discussing #428 and the exceptions made for QUnit, I thought I'd try a short at migrating these tests over to Jest.

So far it works really well. I only had to skip a few tests because of various reasons:

a) Node is missing some locale data
b) Some tests were inconsistent
c) Tests have weird dependencies on global variables

Addresses #524 and #782.

Relevant technical choices

Sets up new Jest config for unit tests
Removes QUnit dependency and tests
Migrates all tests over to new Jest setup
Uses proper imports instead of accessing globals

Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

googlebot · 2019-10-15T17:06:59Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Unused since #82.

Needed for tests. Webpack will map these back to the globals again.

tofumatt · 2019-11-04T20:28:20Z

I'm seeing three failures setting up Analytics from stories/settings.stories.js but I'm unclear on why the JSON responses there would have changed. @adamsilverstein: any chance I'm missing something needed to load this test data properly? Sorry, but I haven't changed much in that file so I'm not clear what's up.

I wonder if we need to be better mocking @wordpress/api-fetch now that we're importing it rather than relying on the global? 🤷‍♂

felixarntz · 2019-11-05T00:54:13Z

@swissspidy Can you take a look at the additional changes?

aaemnnosttv · 2019-11-05T08:42:34Z

@tofumatt the problem is that it's making an API request using apiFetch which is mocked in the storybook config using the wp global (along with a bunch of other stuff).

Looks like the eslint rule for the wp global is being excluded here.

site-kit-wp/.storybook/config.js

Lines 86 to 91 in 102e534

    
           wp.apiFetch = ( vars ) => { 
        
           	const matches = vars.path.match( '/google-site-kit/v1/modules/(.*)/data/(.*[^/])' ); 
        
           	if ( window.googlesitekit.modules[ matches[ 1 ] ][ matches[ 2 ] ] ) { 
        
           		return Promise.resolve( window.googlesitekit.modules[ matches[ 1 ] ][ matches[ 2 ] ] ); 
        
           	}

Can we mock the response of this module resolution for storybook only?

felixarntz · 2019-11-06T13:07:17Z

@tofumatt Note that there is a conflict with develop.

tofumatt · 2019-11-06T13:35:23Z

Conflict fixed; it was a new QUnit test. I ported it over to Jest so this should be set now!

felixarntz

This is some amazing work.

There are a couple minor issues we still need to resolve though. I found for example some deleted code being re-included, and an incorrect uncapitalized Url. Let's do our best to spot such issues.

For the future, let's keep in mind to use IBs properly. This is a massive PR and could easily have been spread across at least two ones (e.g. the "noise" with all the const --> import changes makes this hard to review). But all in all, looks great!

.eslintrc.json

felixarntz · 2019-11-06T15:20:53Z

assets/js/modules/pagespeed-insights/setup.js

@@ -0,0 +1,198 @@
+/**
+ * TagmanagerSetup component.


This is old code. There shouldn't be a setup component for PSI.

felixarntz · 2019-11-06T15:25:04Z

assets/js/util/test/__snapshots__/showErrorNotification.js.snap

@@ -0,0 +1,9 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


Are these files needed under version control? I'm not sure what this is for.

Yes, that's the whole point of snapshot testing.

Learn more: https://jestjs.io/docs/en/snapshot-testing

felixarntz · 2019-11-06T15:26:28Z

assets/js/components/dashboard-splash/dashboard-splash-app.js

+/**
+ * WordPress dependencies
+ */
+import { Component, Fragment, Suspense as WPSuspense, lazy as WPlazy } from '@wordpress/element';
+import { __ } from '@wordpress/i18n';
+


This still needs to be revisited, it should import from util/react-features.

felixarntz · 2019-11-06T15:27:12Z

assets/js/components/dashboard-splash/dashboard-splash-app.js

 // Check for `Suspense` and `lazy` in `wp.element`; versions before 2.4.0 did
 // not include either, so we need to fallback to the React versions. See:
 // https://github.com/WordPress/gutenberg/blob/master/packages/element/CHANGELOG.md#240-2019-05-21
+let Suspense = WPSuspense;
+let lazy = WPlazy;


See above, this can be removed following the above change.

felixarntz · 2019-11-06T15:29:58Z

assets/js/util/test/getReAuthUrl.js

+		admin: {
+			adminRoot: 'http://sitekit.withgoogle.com/wp-admin/admin.php',
+			apikey: apiKey,
+			connectUrl: 'http://sitekit.withgoogle.com/wp-admin/admin.php?googlesitekit_connect=1&nonce=12345&page=googlesitekit-splash',


This should be connectURL.

felixarntz · 2019-11-06T15:30:52Z

assets/js/util/test/getReAuthUrl.js

+ */
+import { getReAuthURL } from '../';
+
+const createSiteKit = ( apiKey ) => {


A bit unrelated, but let's remove the apiKey references here entirely as they were only necessary when we actually used one.

felixarntz · 2019-11-06T15:31:51Z

assets/js/util/test/getReAuthUrl.js

+
+		expect(
+			getReAuthURL( 'pagespeed-insights', true, googlesitekit )
+		).toStrictEqual(


After removing the API key, we can also remove two of these four checks.

aaemnnosttv

A few minor import grouping tweaks to make here (I wonder why ESLint doesn't catch this?)

assets/js/components/settings/settings-module.js

assets/js/components/settings/settings-modules.js

.../js/modules/analytics/dashboard/dashboard-alltraffic-widget-top-acquisition-sources-table.js

stories/settings.stories.js

aaemnnosttv

Looks good to me. Great work @tofumatt !

felixarntz

Great stuff!

googlebot added the cla: yes label Aug 23, 2019

aaemnnosttv mentioned this pull request Sep 12, 2019

Scaffold foundation for JS unit tests with Jest #524

Closed

tofumatt mentioned this pull request Oct 3, 2019

refactor: Move Adsense output into component file #616

Merged

6 tasks

googlebot added cla: no and removed cla: yes labels Oct 15, 2019

swissspidy and others added 22 commits October 28, 2019 13:52

Remove isFrontendIframeLoaded function

652cbd7

Unused since #82.

Add initial Jest config

c865bb4

Port over some initial tests

1430b66

Setup globals for jest unit tests

c5d6d33

Fix moduleNameMapper

820971c

Use proper module imports

44dbe10

Needed for tests. Webpack will map these back to the globals again.

Make readableLargeNumber always return a string

712f1d5

Update test globals

183457a

Update some tests

8dbfc2a

Remove qunit

d428649

Further update tests

601afd6

Fix import of wp.sanitize

bf58866

Add back babel/polyfill

f2f0cc1

Port data api tests over to jest

d0eaf6e

fix: Fix fillFilterWithComponent test

e0729a6

fix: Test getQuery params with DI

803b34c

fix: Package-lock tweaks

b398da2

fix: Reauth test

b17c7ca

fix: Include Intl to fix numberFormat tests

677acdd

chore: Make tests easier to read

e702b09

fix: Fix toggle test

6ea6c73

chore: fix lint

e6e8898

tofumatt force-pushed the add/jest-for-unit-testing branch from e20d637 to e6e8898 Compare October 28, 2019 21:14

update npm to latest before installing

e53d8c1

try: Use TerserPlugin (#782)

cfffa24

aaemnnosttv mentioned this pull request Nov 6, 2019

Internalize withFilters HOC from Gutenberg #802

Merged

6 tasks

tofumatt added 4 commits November 6, 2019 11:34

fix: Bad rename

ae11dd7

fix: webpack config for storybook

6d32ce9

fix: Maintain translation functions

c53f7e6

fix: Add all i18n functions

22382cb

fix: Move QUnit test to Jest

cd8af25

swissspidy mentioned this pull request Nov 6, 2019

Bundle withFilters HOC from Gutenberg #784

Closed

6 tasks

ThierryA mentioned this pull request Nov 6, 2019

JS translation strings are broken on .org #782

Closed

felixarntz requested changes Nov 6, 2019

View reviewed changes

aaemnnosttv reviewed Nov 6, 2019

View reviewed changes

tofumatt added 4 commits November 6, 2019 16:09

fix: Remove unused setup file

14f7ef0

fix: Use react-features

ccfeee5

fix: Remove API Key usage

78ef379

fix: Use defaultProfileID

76835ca

tofumatt requested review from felixarntz and aaemnnosttv November 6, 2019 16:38

fix: Place lodash in correct import section

9f24287

felixarntz requested changes Nov 6, 2019

View reviewed changes

stories/settings.stories.js Outdated Show resolved Hide resolved

fix: revert defaultProfileId

ab5e45b

tofumatt requested a review from felixarntz November 6, 2019 17:21

aaemnnosttv approved these changes Nov 6, 2019

View reviewed changes

felixarntz approved these changes Nov 6, 2019

View reviewed changes

felixarntz merged commit cb0b1e1 into develop Nov 6, 2019

felixarntz deleted the add/jest-for-unit-testing branch November 6, 2019 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Jest for JS unit testing #437

Migrate to Jest for JS unit testing #437

swissspidy commented Aug 23, 2019 •

edited by tofumatt

googlebot commented Oct 15, 2019

tofumatt commented Nov 4, 2019

felixarntz commented Nov 5, 2019

aaemnnosttv commented Nov 5, 2019

felixarntz commented Nov 6, 2019

tofumatt commented Nov 6, 2019

felixarntz left a comment

felixarntz Nov 6, 2019

felixarntz Nov 6, 2019

swissspidy Nov 6, 2019

felixarntz Nov 6, 2019

felixarntz Nov 6, 2019

felixarntz Nov 6, 2019

felixarntz Nov 6, 2019

felixarntz Nov 6, 2019

aaemnnosttv left a comment

aaemnnosttv left a comment

felixarntz left a comment

Migrate to Jest for JS unit testing #437

Migrate to Jest for JS unit testing #437

Conversation

swissspidy commented Aug 23, 2019 • edited by tofumatt

Summary

Relevant technical choices

Checklist

googlebot commented Oct 15, 2019

tofumatt commented Nov 4, 2019

felixarntz commented Nov 5, 2019

aaemnnosttv commented Nov 5, 2019

felixarntz commented Nov 6, 2019

tofumatt commented Nov 6, 2019

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

swissspidy commented Aug 23, 2019 •

edited by tofumatt