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 documentation for shallow testing #2963

Merged
merged 2 commits into from
Mar 10, 2015
Merged

Add documentation for shallow testing #2963

merged 2 commits into from
Mar 10, 2015

Conversation

graue
Copy link
Contributor

@graue graue commented Jan 27, 2015

See #2393 for the original issue, and #2497 for the implementation.

See #2393 for the original issue, and #2497 for the implementation.
@jimfb
Copy link
Contributor

jimfb commented Jan 27, 2015

Are we using this internally yet? We might want to hold off on merging this documentation until we've started using this and validated that the API makes sense and is production-ready.

@graue
Copy link
Contributor Author

graue commented Jan 28, 2015

I had similar concerns, especially since it doesn't support refs (or context?). @sebmarkbage points out it's a better testing story than anything we have currently, though.

@zpao
Copy link
Member

zpao commented Jan 28, 2015

I think we should probably start talking about it and getting people to use it. We're a slower moving ship so it's harder to steer but getting feedback from people who can move a bit faster would be worthwhile and will perhaps get us to the best shape possible.

We should probably enumerate any limitations though (and perhaps label it as "beta" or something just so people have an idea of what they're getting into)

Also, move it to the bottom of the test utils documentation and mention that it's experimental.
@jareware
Copy link

jareware commented Feb 9, 2015

This looks very useful! I'm curious as to why you'd want to limit this to one level of components, though..? Seems to me you'd be dealing with the same limitations (e.g. ref's missing) regardless of depth, and being able to test the whole composition of the UI could be valuable.

@graue
Copy link
Contributor Author

graue commented Feb 17, 2015

@jareware: the motivation is to be able to test just one thing at a time (i.e., unit testing). Say Foo's render method returns <div class="foo"><Bar /></div>. It's useful to test in isolation without worrying about how Bar behaves. You couldn't do this before. If, however, you want to do more of an integration test, you've always been able to render all the way down and assert properties of the DOM.

@jareware
Copy link

@graue, the value in testing one-level-deep is not lost on me, and I think I see the possible misunderstanding now. So am I correct in assuming whereas previously we've been able to either render a complete component hierarchy using DOM emulation (e.g. jsdom) OR rendering directly as a string, this PR would introduce a third method, where there's no need for DOM emulation (but limited to the depth of a single component)?

@sebmarkbage
Copy link
Collaborator

@jareware Correct.

@jareware
Copy link

Ok, now that that's cleared up, is there any reason why this couldn't be done for an entire component hierarchy instead of a single, "shallow" component? Anything like this.getDOMNode() would obviously be out of the question, but besides that.

@graue graue added this to the 0.13 milestone Mar 3, 2015
@zpao zpao mentioned this pull request Mar 9, 2015
29 tasks
@zpao
Copy link
Member

zpao commented Mar 9, 2015

@graue let me know if you have any updates you want to make before this gets merged. Otherwise I'm happy with it.

@graue
Copy link
Contributor Author

graue commented Mar 10, 2015

:shipit:

zpao added a commit that referenced this pull request Mar 10, 2015
Add documentation for shallow testing
@zpao zpao merged commit f41904c into facebook:master Mar 10, 2015
@zpao
Copy link
Member

zpao commented Mar 10, 2015

Thanks :)

@graue
Copy link
Contributor Author

graue commented Mar 14, 2015

Aww, this doesn't seem to have made it into the changelog...

@sophiebits
Copy link
Collaborator

Oops, our mistake…

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.

6 participants