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 an unit test for React.Fragment with ShallowRenderer #12220

Merged
merged 1 commit into from Feb 15, 2018

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Feb 13, 2018

I think it would be nice to add a test to verity that React.Fragment works fine with ShallowRenderer.

@gaearon
Copy link
Collaborator

gaearon commented Feb 13, 2018

@orta Any ideas why it chose https://github.com/facebook/react/commits/905bfcec60d4aec271d523c4076661bec2a30b60 as (from 2015) as the base commit?

I guess it's because it's the master of @koba04's fork (https://github.com/koba04/react). Shouldn't we be calculating against the target's master?

In fact, shouldn't we use the PR target branch and target fork instead of origin/master for calculating merge base?

@orta
Copy link
Contributor

orta commented Feb 14, 2018

In fact, shouldn't we use the PR target branch and target fork instead of origin/master for calculating merge base?

Yeah, this makes sense, I guess circle is cloning their repo and using their master. Will probably need to add a git remote add fb [git link] and use that for the merge-base 👍 - will make time after tomorrow

@gaearon
Copy link
Collaborator

gaearon commented Feb 14, 2018

I think ideally it shouldn’t even necessarily be tied to master or a specific origin. Like what if you send a PR against another PR, even across forks.

My intuition is that instead of origin/master it should use target/target-branch where target is the fork against which the PR was sent (I assume we know it?) and target-branch is the branch against which the PR was sent. Basically the same things GitHub shows at the top of any PR.

@koba04 koba04 force-pushed the add-an-unit-test-for-react-fragment branch from c2bee97 to 5c568a4 Compare February 15, 2018 13:59
@koba04
Copy link
Contributor Author

koba04 commented Feb 15, 2018

Thanks! CI is passed! 🚀

@gaearon gaearon merged commit 5cd5f63 into facebook:master Feb 15, 2018
@koba04 koba04 deleted the add-an-unit-test-for-react-fragment branch February 15, 2018 14:31
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants