Implement testing in React 0.14, 15, 16, add wallaby.js, use enzyme

[cellog]

This pull request uses enzyme to equalize most tests across all React versions.

Tests that need special handling:

• should enforce a single child - the error text changed from 0.14 to 15 [Provider.spec.js]
• works in <StrictMode> without warnings (React 16.3+) - the test is only run in newer React, and uses the test renderer (enzyme does not support React 16.3+ yet) [both Provider.spec.js and connect.spec.js]

All other tests that used the test renderer have been moved to enzyme with minor syntax changes, and no change to functionality.

There are two significant differences with enzyme:

1. Any change to the tree requires a manual .update() on the root element, or tests won't be able to detect changes
2. Tests that used stub to save a deep part of the rendered tree failed, because enzyme creates new object wrappers around the internal React elements when update() is called, so those had to be replaced by the original call in order to work.

Otherwise, it was simply find/replacing TestRenderer.create with enzyme.mount and then testRenderer.root.findByType with testRenderer.find and all checks of blah.props.name with blah.prop('name')

Travis is set up to run a matrix of tests, using React 0.14, 15, 15.4, latest 15 (15.6), 16.2, 16.3, 16.4 and I added local testing for each version with commands like npm run test:15 and npm run test:14

This is based on the code from enzyme-adapter-react-helper but modified because I don't think they ever actually really tested it with React 0.14 :). This code actually works.

CAVEAT: 12 tests fail in React 16.4. #980 is designed to fix them, so if the two requests are merged all tests should pass again. The important thing will be to re-base #980 on this pull request to see if the changes do break earlier React versions, once this pull request is accepted.

CC: @markerikson @timdorr

[timdorr]

I dunno, this seems like a lot of complexity for something that's not likely to break any time soon.

4.x/5.x will continue to support React 0.14-16.3, and those versions of React aren't going to change (outside of some huge security thing). The plan is to make whatever we release next dependent on React 16.4, so testing on old versions is irrelevant. It's nice to confirm our guarantees, but it seems pretty expensive to do so.

[cellog]

Perhaps we view expense differently. We probably want 5.x to continue to work in new versions of React, and if we need to make changes as dramatic as the gDSFP bugfix, we will want to make sure that our users with massive legacy apps continue to work. I think it would be very expensive if a new 5.x version is released that breaks on react 15, and there is no test suite that can be used to verify a fix. No?

It took me about 20 minutes to convert the tests, and about 40 minutes to tweak getting 0.14 test environment to make it work. But it's not actually that complex.

What are you afraid will happen? The PR doesn't actually introduce any complexity to the tests, it just makes it possible to run them in different environments.

[markerikson]

I'm all in favor of better testing infrastructure :)

The whole uninstall/reinstall dance is a bit goofy, but I can understand why it's necessary. Other than that, LGTM! Tim, unless you've got some serious objections, let's get this in and see how #980 looks.

[timdorr]

I do have serious objections. The "expense" I'm referring to is with the tooling setup, not the tests themselves. I don't really care about enzyme vs react-test-renderer APIs. That's just bikeshedding.

5.0 doesn't have to support newer versions of React. 6.0 will be for that. Basically, we have this planned/implemented:

• 6.0 <=> React >=16.4
• 5.0 <=> React 0.14, 15, <=16.4
• 4.x <=> React 0.14 - 16.4+ (maintained as a favor to Dan/Facebook for its components that still use this version)

We're done with any feature development on 5.0. There will only be bugfixes from here on out. Since those React peer deps are immutable, there is no risk of breakage.

But the main point here is I don't consider this a better testing infrastructure. It's far more complex, which is a barrier for entry for contributors. Just running npm test will change our dependencies (btw, @cellog, you appear to be running an old version of npm):

⚡️  ~/forks/react-redux (⭠ multi-test|✔)
 ⇶ npm test

... output ...

⚡️  ~/forks/react-redux (⭠ multi-test|✚2)
 ⇶ git status
On branch multi-test
Your branch is up to date with 'cellog/multi-test'.

Changes not staged for commit:
	modified:   package-lock.json
	modified:   package.json

Contribution becomes harder, which I'm very much against. We need and want help. The code is already really complex, let's not make the tooling too.

[cellog]

Ok. How do YOU intend to test any changes to react-redux against react 14 and 15?

[cellog]

I'm going to be blunt. Tim, you're not being helpful. Stop it. It is obvious to you and me and everyone paying attention that this test suite will simply make it possible to test bug fixes against the actual react versions that react-redux claims to support. Are you suggesting that faith is the proper testing approach?

Once master goes to react 16.4+, the complexity can be stripped and left in a 5.x branch for any necessary bug fixes. It won't scare away users because npm test actually does work.

It modifies dependencies because npm is stupid and uninstalls anything not in package.json that it finds installed. The bug still exists in latest npm.

[timdorr]

OK, I'm going to bow out of this PR. To be clear: I'm not critiquing you, the human being behind the keyboard, I'm attempting to critique the code. But it seems like I've struck a nerve here on a personal level, so I'd rather not continue to stoke that fire.

Edit: Also, that's not an npm bug. It does --save by default now (same as yarn). You can disable it with --no-save and removing the -D (shorthand for --save-dev) flag.

[cellog]

Oh come on, you can accept a critique if you can give it :).

Here is my challenge: I clicked the checkbox next to "allow maintainers to make edits" and if you would try it out to fix the issues you saw, or say why they can't be fixed, I will work with it.

When your response is "it can't work" that is... difficult to work with. I mean, what do you want? Delete the whole thing and go away? How can I productively respond to that? It's nothing to do with personal reactions. It's just not helpful.

[cellog]

This: npm/npm#16839 (comment) is what was happening when I used --no-save. However, I was able to fix the issue because npm will not uninstall deps if it is within a single install command. So by merging the 2 calls to npm install into 1, we can again use --no-save.

In addition, the tests were changing our node_modules environment (by necessity) and now using the new run-tests.js file, we can properly restore them by running "npm i" after jest concludes, regardless of whether jest fails or not.

This does NOT address the question of whether this is too complex for contributing users to run tests. I updated the docs and added back in test:watch to match the docs. If this is too complex for contributors, that is a philosophical question I leave to you.

The trade-off is simple: do you want to make certain that react-redux works in all supported React versions with CI, or make it simpler to run tests?

In both cases, the tests will run with:

npm run tests

and this default command does NOT touch node_modules OR package.json/lock any longer.

Contributors dealing with issues that affect all versions can use that command (or test:watch) to do their work. Contributors dealing with regressions that affect older versions can use the test:* commands to run tests against the older versions.

Now that the tests don't touch package.json, it will always be possible to restore to equilibrium in node_modules with npm i even if Ctrl-C stops tests mid-run, and there is 0 chance of accidentally changing dev dependencies in a PR or commit.

[markerikson]

I won't have time to look at this further until Sunday.

I see Dan commented over in #980 advising that we not switch to Enzyme, as it's been lagging behind React.

How feasible would this multi-version testing approach be without Enzyme?

[cellog]

We only use one feature of either, but the documentation for test renderer versions prior to current is not online so we would need to reverse engineer it. Difficult but not impossible

[cellog]

Here's the thing. react-test-renderer did not implement findByType until React 16, so we would need to implement it.

In other words, to support multiple versions of React, the only way to do it is either to use enzyme ...or to write enzyme. Fortunately, this wouldn't affect the 6.x or 7.x branch. What I'd recommend is that the current master be branched off so we can preserve the tests, before this PR is merged (if it is merged)

For version 6.x+, we can write all new tests using the test renderer, and then do a copy/paste of the tests from that saved branch to get the old tests back that use the 16+ test renderer.

We really only need enzyme for testing in React 14/15/16 with the same tests.

[markerikson]

FYI, Dan pointed out that Brian set up per-React-version testing for react-lifecycles-compat here: https://github.com/reactjs/react-lifecycles-compat

Looks like it's done by writing a bunch of package.json files in subfolders, each importing the correct React package versions. There's a install.js file that runs Yarn for each React version folder, and then the test iterates over folders/versions and dynamically imports the right React version.

Also looks like https://github.com/gaearon/react-hot-loader does 15 and 16 as well.

inspectpack does something similar for Webpack 1-4 (see package installation and run scripts ).

[cellog]

Success! The tests build in their own sandboxed directories, with zero interference with the source or our package.json or node_modules. The "test" command runs only against the React version in package.json (and same with the wallaby.js), but continuous integration can test against all versions and combines the lcov.info that jest generates so we can verify full coverage across all versions.

This pull request is ready for another review @timdorr @markerikson

Basically, I made directories

test/
 react/
  0.14/
  15/
  16.2/
  16.3/
  16.4/

Each directory contains a private package.json, and on test runs, the test files AND source are copied just-in-time to these folders, packages installed in node_modules, and then the tests run from their own sandbox, generate coverage, then return back.

I have not figured out a way to make Travis run them in parallel yet, so the CI build will be 5 times slower than it needs to be until that is worked out.

As always, the travis build fails because 5.1.0 is broken on 16.4. Also it looks like I forgot to check for linting errors, so these known issues should be addressed prior to merge.

[cellog]

latest commit fixes travis parallel tests and linting errors, it's ready for merge unless your review finds other issues

[cellog]

just FYI, I made a branch on my setup that merges this with #980 in order to verify the correctness of the patch in all supported React versions (spoiler alert: it works)

I had to fix .babelrc and 1 test is wrong

https://github.com/cellog/react-redux/tree/multi-test-fix

If the PR and the testing config are acceptable, I can make a PR from this branch.

[markerikson]

Cool. I'll try to take a look at this tomorrow evening. Thanks!

[cellog]

I'm about to push a bunch of commits, but wanted to say first for @timdorr that I wrote a short guide for how to add a new React version for testing in CONTRIBUTING.md, and I'll paste that section here to let you know how it works:

Adding a new React version for testing

To add a new version of React to test react-redux against, create a directory structure
in this format for React version XX:

test/
 react/
  XX/
   package.json
   test/
    getTestDeps.js

So, for example, to test against React 15.4:

test/
 react/
  15.4/
   package.json
   test/
    getTestDeps.js

The package.json must include the correct versions of react, react-dom,
react-test-renderer and the correct enzyme adapter for the React version
being used, as well as the needed create-react-class, jest, enzyme versions
and the jest and scripts sections copied verbatim like this:

{
  "private": true,
  "devDependencies": {
    "create-react-class": "^15.6.3",
    "enzyme": "^3.3.0",
    "enzyme-adapter-react-15.4": "^1.0.6",
    "jest": "^23.4.2",
    "react": "15.4",
    "react-dom": "15.4",
    "react-test-renderer": "15.4"
  },
  "jest": {
    "testURL": "http://localhost",
    "collectCoverage": true,
    "coverageDirectory": "./coverage"
  },
  "scripts": {
    "test": "jest"
  }
}

getTestDeps.js should load the version-specific enzyme adapter and
test renderer (all versions newer than 0.14 use react-test-renderer,
0.14 uses react-addons-test-utils):

import enzyme from 'enzyme'
import TestRenderer from 'react-test-renderer'
import Adapter from 'enzyme-adapter-react-15.4'

enzyme.configure({ adapter: new Adapter() })

export { TestRenderer, enzyme }

Then you can run tests against this version with:

REACT=15.4 npm run test

and the new version will also be automatically included in

REACT=all npm run test

I don't think a generic package for running tests against react versions is absolutely necessary now, because it would require taking some of the specific stuff and making it more generic, which necessitates more sanity tests, configuration (for example, we do NOT want to overwrite the getTestDeps.js file in each directory with the global one) that is not needed. I could extract it out later, and I'm happy to do that, but I want to focus on getting it working here first.

Once things are cooking in react-redux, the PR is merged, I can extract it to make the codebase more focused on react-redux. Same with the coverage merging.

Also, it turns out that codecov already automatically merges all lcov.info files it finds, so I can remove that code

[cellog]

@timdorr, I think I implemented all the suggestions, but it was quite a few, so you might want to verify that I didn't miss any. Thanks for the tips, it's a much leaner PR and more extensible for adding new versions.

I do think it can get leaner by removing enzyme, as we won't need adapters any more. react-testing-library will be much easier to work with long-term. With this change, the getTestDeps.js file becomes unnecessary, which will simplify adding new versions (or old ones) tremendously

[markerikson]

Trying this out locally. I'm on Windows, and it looks like the line that tries to spawn NPM to install a given React version is failing because of cross-platform mismatches with starting stuff. I saw mentions of cross-spawn at https://stackoverflow.com/questions/37459717/error-spawn-enoent-on-windows, and switching out spawnSync that seems to be working for me.

[markerikson]

Awright, I'm ready to merge this in. I know Dan's not keen on the Enzyme thing, but sounds like Greg wants to switch this over to react-testing-library anyway.

I want to go ahead and get this in now so that we can turn our attention to getting #980 done. We can always tweak the test setup further later.

[cellog]

Yes, I am much more comfortable with rtl than enzyme for react-redux's testing needs