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 example tests #1471

Closed
wants to merge 1 commit into from
Closed

Conversation

jontewks
Copy link
Contributor

@jontewks jontewks commented Mar 3, 2016

Per issue #1397

@jontewks
Copy link
Contributor Author

jontewks commented Mar 3, 2016

@gaearon wanted to get this opened up for initial review. Originally @urbanvikingr and I were going to pair on these, but my son was born a tad early so I wasn't able to stick to a schedule so we decided to split them. I last heard from @urbanvikingr that he was dealing with some issues and didn't have a lot of spare time about a week ago, so I didn't want this to appear forgotten about. I completed tests for each component, but wanted to leave the option open for @urbanvikingr to contribute some additional test cases, or tests around the container elements if he finds the time.

@badnorseman
Copy link
Contributor

@jontewks Thank you. I suggest that we merge your additional tests. Then, the todos example will have tests for actions, reducers and components.

@weicheng113
Copy link

Hi Guys,

How these tests are better than the ones in #1463? Have you looked? is it more reasonable to make app easier to test first?

Cheers, Cheng

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2016

This looks like a good start to me, please continue!

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2016

Hey @jontewks!

We started porting tests to Enzyme (#1481), and @mjw56 submitted a PR handling todos as well in #1493. I want to apologize for not handling it better because you already put in some work on this, and I should have notified you about our decision to use Enzyme, and suggest you to work on it first. I’m sorry!

Can you please review #1493 and leave your thoughts on it? Is there anything from here that is missing there?

@jontewks
Copy link
Contributor Author

jontewks commented Mar 8, 2016

Thanks @gaearon, and no worries. Sorry this happened so slowly, my wife and I just had a child a tad early so I didn't have nearly the amount of time to put into this as I was hoping. The new tests look good, and I learned from reading through them, like expect has a createSpy method, which I could have made use of!

Things have calmed down for us now, so if there is anything else that I can contribute to, I have the spare time this week. Thanks again.

@jontewks jontewks closed this Mar 8, 2016
@mwilc0x
Copy link
Contributor

mwilc0x commented Mar 9, 2016

I apologize, I should have gone through the open PR's first before working on this. I'll make sure this does not happen again. Thanks for the feedback @jontewks. I also just reviewed yours as well to make sure we are all on the same page. Again, sorry for the mixup. Congrats on your new child! 😃

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.

None yet

6 participants