Convert tests to react-testing-library, remove enzyme and test-renderer #996

cellog · 2018-08-12T14:06:42Z

This drastically simplifies testing against future versions, as the only requirement is ReactDOM.render()

This commit fixes reduxjs#965 The essence of the problem is that getDerivedStateFromProps is called when the incoming props OR incoming local state changes. So we cannot store anything in state that is needed in shouldComponentUpdate. This commit splits up the tracking of incoming props, incoming store state changes, and removes getDerivedStateFromProps and the usage of local state to store any information. Instead, local state is used as a flag solely to track whether the incoming store state has changed. Since derived props are needed in shouldComponentUpdate, it is generated there and then compared to the previous version of derived props. If forceUpdate() is called, this bypasses sCU, and so a check in render() compares the props that should have been passed to sCU to those passed to render(). If they are different, it generates them just-in-time. To summarize: 1) shouldComponentUpdate is ONLY used to process changes to incoming props 2) runUpdater (renamed to triggerUpdateOnStoreStateChange) checks to see if the store state has changed, and stores the state, then updates the counter in local state in order to trigger a new sCU call to re-generate derived state. Because of these changes, getDerivedStateFromProps and the polyfill are both removed. All tests pass on my machine, but there is at least 1 side effects to the new design: - Many of the tests pass state unchanged to props, and pass this to child components. With these changes, the two updates are processed separately. Props changes are processed first, and then state changes are processed. I updated the affected tests to show that there are "in-between" states where the state and props are inconsistent and mapStateToProps is called with these changes. If the old behavior is desired, that would require another redesign, I suspect.

merge in testing

DO NOT REMOVE PLEASE TIM

cellog · 2018-08-12T14:50:27Z

(Travis build fails because HEAD doesn't work in React 16.4. The output is expected)

This reverts commit eb960c4, reversing changes made to b255a46.

timdorr

I also wonder if we should drop 16.4 from the test suite. It's not going to get fixed in this version (I really don't want to try), so it's not something we need to support and, therefore, test against.

timdorr · 2018-08-13T15:26:53Z

test/components/Provider.spec.js

@@ -75,47 +81,52 @@ describe('React', () => {

    it('should add the store to the child context', () => {
      const store = createStore(() => ({}))
+      store.mine = 'hi'


I'm not sure this is the best approach for identifying a store. It would be better to provide canary values in the initialState for each store. Setting arbitrary properties is technically possible, but it's not idiomatic Redux and might be confusing to those working with these tests in the future.

I'm fixing this in the 16.x PR I'm making. Fully agree, and an easy fix

I already took care of this in the #998 branch - I put some canary values in the store as suggested.

timdorr · 2018-08-13T15:30:21Z

test/components/Provider.spec.js

@@ -5,22 +5,28 @@ import PropTypes from 'prop-types'
 import semver from 'semver'
 import { createStore } from 'redux'
 import { Provider, createProvider, connect } from '../../src/index.js'
-import { TestRenderer, enzyme } from '../getTestDeps.js'
+import * as rtl from 'react-testing-library'


Can we have explicit imports here (import { render, fireEvent })? I tend to dislike import * as foo syntax out of both habit and as a very implicit signal that this is TS code (as it's a standard over there, I've noticed; a minor nit pick, for sure). Obviously it doesn't matter at all in the testing context, but it's mainly about enforcing good patterns for others in future PRs. Folks tend to copy what you do in their code.

I tend to disagree about this, not in principle, just on whether it's a good pattern. I like to do explicit imports for things that have fewer than 5 items. More than that, it gets confusing (I'm looking at you, redux-saga/effects) as to whether an imported thing is from one place or another (for example, delay is from redux-saga, the effects are not).

For react-testing-library, it is even more confusing, because render and cleanup and a few others are from the main library, but the queries like getByTextId come from the return value of render Making this clear is difficult, and I think it will reduce the learning curve for new contributors to use the * import. But I'll leave the call to you two. If you still feel it's an issue after reading my perspective, I'll change it.

timdorr · 2018-08-13T15:30:52Z

test/components/Provider.spec.js

@@ -5,22 +5,28 @@ import PropTypes from 'prop-types'
 import semver from 'semver'
 import { createStore } from 'redux'
 import { Provider, createProvider, connect } from '../../src/index.js'
-import { TestRenderer, enzyme } from '../getTestDeps.js'
+import * as rtl from 'react-testing-library'
+import 'jest-dom/extend-expect'


This should probably be in our test setup file. (Do we have one? We should, if not.)

although... we only use these files in 2 of 5 test files. so maybe not?

timdorr · 2018-08-13T15:32:38Z

wallaby.js

@@ -0,0 +1,23 @@
+module.exports = function(wallaby) {


I don't want to get into a philosophical debate, but can we please remove this file? I don't want to support proprietary, closed-source software in an OSS project. It's fine that you use it locally, but it's not a requirement to develop for the library nor do I really want to promote them from this project.

I added it to .gitignore in my PR

markerikson · 2018-08-13T15:51:34Z

@timdorr : I think Greg's comment is that our current HEAD is the 5.1.0-test1 code, and that doesn't work right with 16.4. Did you mean that we should actually drop 16.3 from the test matrix?

timdorr · 2018-08-13T16:56:53Z

Well, I consider 16.3 a "broken" release anyways, so yeah, that can go too.

Or we can keep them and try returning to the previous code prior to #919 (which is the last code change anyways).

markerikson · 2018-08-13T17:19:49Z

Yeah. Tonight I plan to revert the actual code on master to where it was for 5.0.7, then create a 5.x branch (similar to our existing 4.x branch). Then I'm going to sort out what's going on with tests.

markerikson · 2018-08-14T00:42:05Z

@timdorr : when you said "It's not going to get fixed in this version", what "it" are you referring to? The <StrictMode> test?

markerikson · 2018-08-14T04:28:18Z

Closed, superseded by #998 .

cellog added 14 commits July 14, 2018 16:47

fix linting errors

41ff984

update dev dependency to React 16.4

9f44bed

Merge pull request #3 from reduxjs/master

865cc82

merge in testing

prepare for react-testing-library

eaa3284

add wallaby.js for debugging using wallaby

499aa63

DO NOT REMOVE PLEASE TIM

begin work of moving from enzyme to rtl

0e2e194

convert more tests to rtl

1e0ad89

fully convert to rtl, remove enzyme and test renderer deps

545f53d

fix error in test

5333b40

convert to rtl

a47b64c

update testing infrastructure

77d4fa3

remove enzyme and react-test-renderer

0f427b5

fix lint errors

b255a46

cellog added 2 commits August 12, 2018 13:30

Merge branch 'pre-6.x' into master

eb960c4

Revert "Merge branch 'pre-6.x' into master"

9578017

This reverts commit eb960c4, reversing changes made to b255a46.

cellog mentioned this pull request Aug 13, 2018

Use new Context, forwardRef for 6.x (alternate implementation to #995) #997

Closed

timdorr requested changes Aug 13, 2018

View reviewed changes

markerikson mentioned this pull request Aug 14, 2018

Convert tests to use react-testing-library instead of Enzyme #998

Merged

markerikson closed this Aug 14, 2018

cellog mentioned this pull request Aug 14, 2018

Use React.createContext() by @cellog #1000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert tests to react-testing-library, remove enzyme and test-renderer #996

Convert tests to react-testing-library, remove enzyme and test-renderer #996

cellog commented Aug 12, 2018

cellog commented Aug 12, 2018

timdorr left a comment

timdorr Aug 13, 2018

cellog Aug 14, 2018

markerikson Aug 14, 2018

timdorr Aug 13, 2018 •

edited

cellog Aug 14, 2018

timdorr Aug 13, 2018

cellog Aug 14, 2018

cellog Aug 14, 2018

timdorr Aug 13, 2018

cellog Aug 14, 2018

markerikson commented Aug 13, 2018

timdorr commented Aug 13, 2018

markerikson commented Aug 13, 2018

markerikson commented Aug 14, 2018

markerikson commented Aug 14, 2018

Convert tests to react-testing-library, remove enzyme and test-renderer #996

Convert tests to react-testing-library, remove enzyme and test-renderer #996

Conversation

cellog commented Aug 12, 2018

cellog commented Aug 12, 2018

timdorr 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

timdorr Aug 13, 2018 • edited

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

markerikson commented Aug 13, 2018

timdorr commented Aug 13, 2018

markerikson commented Aug 13, 2018

markerikson commented Aug 14, 2018

markerikson commented Aug 14, 2018

timdorr Aug 13, 2018 •

edited