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.

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

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

Closed
wants to merge 16 commits into from

Conversation

cellog
Copy link
Contributor

@cellog cellog commented Aug 12, 2018

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

@markerikson

cellog added 14 commits July 14, 2018 16:47
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.
@cellog
Copy link
Contributor Author

cellog commented Aug 12, 2018

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

Copy link
Member

@timdorr timdorr left a comment

Choose a reason for hiding this comment

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

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.

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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'
Copy link
Member

@timdorr timdorr Aug 13, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to .gitignore in my PR

@markerikson
Copy link
Contributor

@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
Copy link
Member

timdorr commented Aug 13, 2018

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
Copy link
Contributor

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
Copy link
Contributor

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

@markerikson
Copy link
Contributor

Closed, superseded by #998 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants