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

add todos enzyme tests #1493

Closed
wants to merge 4 commits into from
Closed

Conversation

mwilc0x
Copy link
Contributor

@mwilc0x mwilc0x commented Mar 8, 2016

This PR adds enzyme tests for the todos example components

This is part of #1481

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2016

I specifically didn’t mention todos in #1481 because @jontewks was already working on it in #1471. I should have communicated this better 😥 .

Ultimately we want to use enzyme in all our tests so this probably supersedes @jontewks’s work. I apologize for not handling it better.

Can you please make sure that this includes whatever tests existed in #1471?

This was referenced Mar 8, 2016
@mwilc0x
Copy link
Contributor Author

mwilc0x commented Mar 9, 2016

Yeah this was my fault, I should have checked the open PR's first. I reviewed #1471 and it looks like this includes all the tests from there.

@whoisandy
Copy link

Aren't the component tests for todo-with-undo the same as todos? Trying to figure out if there's any other component test that has to be added. The action and reducer tests are missing though. Since redux-undo is already tested, minor tweaks in action and reducer tests might be sufficient.

"expect": "^1.8.0",
"express": "^4.13.3",
"jsdom": "^5.6.1",
"jsdom": "^8.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn’t we be able to get rid of jsdom now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can safely remove jsdom now. Good catch, thanks. I've updated the PR with 0baa696 to reflect this.

@timdorr
Copy link
Member

timdorr commented May 7, 2016

Can this be reworked to also remove/replace the existing React Test Utils tests?

@mwilc0x
Copy link
Contributor Author

mwilc0x commented May 10, 2016

@timdorr sure, we can do that. If anyone else is interested, please feel free to chime in. I wouldn't be able to start these until tomorrow.

@mwilc0x
Copy link
Contributor Author

mwilc0x commented May 11, 2016

@timdorr I started by converting one file, does this look ok to you?

@timdorr
Copy link
Member

timdorr commented Jan 5, 2017

Thanks for the efforts @mjw56. We're going to go down the route of using react-test-renderer instead.

@timdorr timdorr closed this Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants