State: Remove subtree Makefiles in favor of single test runner #3773

aduth · 2016-03-04T00:45:14Z

Part of #3822.

This pull request seeks to eliminate subtree-specific Makefiles in the client/state directory, used always for testing, with a single test runner for the entirety of the client/state directory.

The changes include 22 eliminated Makefiles.

Benefits include:

Improved performance by decreasing number of Mocha/Babel reinitializations
Common before / after helpers (specifically chai middleware and nock lifecycle)
Fewer files to maintain
Assurance of proper test teardown for eventual inclusion in global test runner

Testing instructions:

Ensure tests pass by running make test from the project root directory.

aduth · 2016-03-04T00:51:39Z

~~For the moment, this does not include plugins/wporg, as there are more customized mocked functions there. I'll see if it's simple enough to include in these changes.~~

#3773 (comment)

blowery · 2016-03-04T00:56:48Z

That's amazing. How much faster is it?

aduth · 2016-03-04T01:18:47Z

@blowery : Looks pretty significant, about 92% faster (3.7412s average now, 48.1172s average previously).

https://gist.github.com/aduth/389940df7aebc0805167

blowery · 2016-03-04T13:17:04Z

And it appears that most all of that time is spent compiling the JS. Big pause, then all of the tests spit out pretty quickly. This will likely scale up quite well.

mtias · 2016-03-04T13:21:41Z

This is great.

blowery · 2016-03-04T13:24:45Z

Messing around with it a little bit, I'm not sure we're using the babel cache. Running this from client/state:

time env BABEL_CACHE_PATH=/tmp/babel/cache-new.json REPORTER=min make test

I get a timing of about 5s cold, 2s hot.

time env REPORTER=min make test

I get ~3s hot. Cold is back around 5s.

aduth · 2016-03-04T13:24:45Z

It wasn't much trouble to include plugins in the single test runner, so there's now only a single Makefile for the entire client/state directory with the changes here.

blowery · 2016-03-04T13:34:05Z

Just confirmed with BABEL_DISABLE_CACHE=1.

Cold = 5s
Default babel caching = 3.3s
Specified babel cache = 2.2s

So finding the cache location takes babel a full second? Weird.

aduth · 2016-03-04T14:07:09Z

@blowery : Looks like CircleCI would let us specify custom directories for cached files, so might be an interesting way to keep tests hot between builds.

Perhaps out of scope of this pull request though? 😄

gwwar · 2016-03-04T15:51:28Z

Do we have instructions for testing a single test file?

blowery · 2016-03-04T16:47:52Z

@aduth #3782

aduth · 2016-03-04T17:19:21Z

Do we have instructions for testing a single test file?

Spent some time wrestling with Makefile, and came up with a solution in 5f0307c which lets a developer run make test-posts from client/state to run only the posts-related tests. This can be even more granular if they wish, e.g. make test-"sites plans".

gwwar · 2016-03-04T17:45:46Z

👍 I gave it a spin, and it works as described. I think I'd still want the granularity of being able to test a specific file, instead of relying on the grep flag, but I'll spin up a separate PR for that. 🚢

gziolo · 2016-03-04T17:56:41Z

client/state/test/index.js


 describe( 'state', () => {
+	before( () => {
+		Chai.use( sinonChai );


I really like that we are doing setup and tear down on the top level, especially I'm happy about disabling all network requests 👍

gziolo · 2016-03-04T18:04:23Z

@aduth: Great work on that PR. In my opinion this is a really simple approach that scales well. We can repeat all steps taken here in other subfolders. In parallel we cane figure out how to optimise the way we load single files. There is also very high probability that if tests loaded for given subfolder execute successfully they are going to work also when combined at top level with other subfolders. :)

🚢

gwwar · 2016-03-04T21:16:49Z

client/state/Makefile

@@ -5,6 +5,9 @@ BASE_DIR := $(NODE_BIN)/../..
 NODE_PATH := $(BASE_DIR)/client

 test:
-	@NODE_ENV=test NODE_PATH=$(NODE_PATH) $(MOCHA) --compilers jsx:babel/register,js:babel/register --reporter $(REPORTER)
+	@NODE_ENV=test NODE_PATH=$(NODE_PATH) $(MOCHA) --compilers jsx:babel/register,js:babel/register --reporter $(REPORTER) $(addprefix --grep ,$(GREP))


I played around with this a bit too, and came up with:

REPORTER ?= spec NODE_BIN := $(shell npm bin) MOCHA ?= $(NODE_BIN)/mocha BASE_DIR := $(NODE_BIN)/../.. NODE_PATH := $(BASE_DIR)/client FILE = "./test" test: @NODE_ENV=test NODE_PATH=$(NODE_PATH) $(MOCHA) --compilers jsx:babel/register,js:babel/register --reporter $(REPORTER) $(FILE) .PHONY: test

With single test usage:

make test FILE=/Users/kerryliu/checkouts/wp-calypso/client/state/test/initial-state.js

The only downside is we don't get the setup/cleanup from the index.js file.

@gwwar: I think we might be able to do something with package.json scripts, as npm will automatically traverse up to the closest package.json. The idea would be to specify the test as running Mocha with all of our environment variables. This could be in a package.json in client/state, or in the root package.json, so long as we detect whether the current working directory is outside the project root.

Then, running cd client/state/sites/plans && npm test would only run the site plans tests.

The only downside is we don't get the setup/cleanup from the index.js file.

That seems like a big downside?

Maybe something to bake into a custom runner at the root? Only add files that match a glob, but always add the root, which has setup / teardown.

State: Remove subtree Makefiles in favor of single test runner

aduth · 2016-03-04T22:28:00Z

Thanks for the reviews, all! Going to go through any related open pull requests that might be affected and ping the authors.

aduth · 2016-03-04T22:44:16Z

client/state/test/index.js

+	} );
+
+	describe( 'ui', () => {
+		require( '../users/test/actions' );


Gah, these should be ../ui 😧 Will put together a follow-up PR.

aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 4, 2016

blowery mentioned this pull request Mar 4, 2016

Framework: Be explicit about which cache we use for babel #3782

Closed

gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 4, 2016

gziolo reviewed Mar 4, 2016
View reviewed changes

gziolo mentioned this pull request Mar 4, 2016

Domains: Move all lib/domains tests to local single test runner #3784

Merged

gwwar reviewed Mar 4, 2016
View reviewed changes

aduth added 6 commits March 4, 2016 17:13

Testing: Add cleanup method to DOM setup module

0a42327

State: Improve tear-down of initial state testing

b08b1d5

State: Move state tests to single test runner

ece40d7

Docs: Update data recommendations to encourage single test runner

12c0e8e

State: Include plugins tests in state test runner

e358ef8

State: Add support for running subtree tests

bc0576c

aduth force-pushed the update/redux-single-test-runner branch from 5f0307c to bc0576c Compare March 4, 2016 22:14

aduth added a commit that referenced this pull request Mar 4, 2016

Merge pull request #3773 from Automattic/update/redux-single-test-runner

e28400e

State: Remove subtree Makefiles in favor of single test runner

aduth merged commit e28400e into master Mar 4, 2016

aduth deleted the update/redux-single-test-runner branch March 4, 2016 22:27

This was referenced Mar 4, 2016

Add stats to the global redux store #2310

Closed

Redux: Add utils actions and store-based uniqueId replacement #1793

Closed

Editor: Support VideoPress post-editor preview #2267

Closed

aduth reviewed Mar 4, 2016
View reviewed changes

aduth mentioned this pull request Mar 4, 2016

State: Global test runner cleanup #3789

Merged

This was referenced Mar 6, 2016

Framework: introduce config file with all test names to be included in the single tests runner #3801

Merged

Framework: Speeding Up The Tests #3822

Closed

gwwar mentioned this pull request Mar 7, 2016

Google Apps: Show Current User #3584

Merged

gziolo added this to the Framework: Single test runner milestone Mar 9, 2016

gziolo mentioned this pull request Mar 10, 2016

Framework: Migrate tests from Makefile to single test runner #3942

Closed

gziolo removed the [Status] Ready to Merge label Mar 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State: Remove subtree Makefiles in favor of single test runner #3773

State: Remove subtree Makefiles in favor of single test runner #3773

aduth commented Mar 4, 2016

aduth commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

blowery commented Mar 4, 2016

mtias commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

gwwar commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

gwwar commented Mar 4, 2016

gziolo Mar 4, 2016

gziolo commented Mar 4, 2016

gwwar Mar 4, 2016

aduth Mar 4, 2016

blowery Mar 4, 2016

aduth commented Mar 4, 2016

aduth Mar 4, 2016

State: Remove subtree Makefiles in favor of single test runner #3773

State: Remove subtree Makefiles in favor of single test runner #3773

Conversation

aduth commented Mar 4, 2016

aduth commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

blowery commented Mar 4, 2016

mtias commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

gwwar commented Mar 4, 2016

blowery commented Mar 4, 2016

aduth commented Mar 4, 2016

gwwar commented Mar 4, 2016

gziolo Mar 4, 2016

Choose a reason for hiding this comment

gziolo commented Mar 4, 2016

gwwar Mar 4, 2016

Choose a reason for hiding this comment

aduth Mar 4, 2016

Choose a reason for hiding this comment

blowery Mar 4, 2016

Choose a reason for hiding this comment

aduth commented Mar 4, 2016

aduth Mar 4, 2016

Choose a reason for hiding this comment