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

[Fiber] Add unit tests for ReactDOMFiber #8016

Merged
merged 1 commit into from
Oct 21, 2016

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Oct 19, 2016

This is an another PR of #8001, which adds unit tests for ReactTopLevelText in ReactDOMFiber.

#8001 (comment)


beforeEach(() => {
// to supress a warning that ReactDOMFiber is an experimental renderer.
spyOn(console, 'error');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has an unfortunate downside of hiding all (even unintentional) warnings.

An alternative to this would be to import ReactDOM but wrap the whole test suite into if (ReactDOMFeatureFlags.useFiber). I would probably prefer that with existing test infrastructure.

Alternatively we can suppress the warning when process.env.NODE_ENV === 'test'.

@sebmarkbage You have an opinion on this?

Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 20, 2016

Choose a reason for hiding this comment

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

I like the idea of just importing ReactDOM and wrapping it all in a if (ReactDOMFeatureFlags.useFiber).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon @sebmarkbage Thanks! I've updated this with using ReactDOMFeatureFlags.useFiber.

@koba04 koba04 force-pushed the add-tests-for-react-dom-fiber branch from 20967c3 to 3c4aac4 Compare October 20, 2016 16:54
});

it('should render a component returning strings directly from render', () => {
if (ReactDOMFeatureFlags.useFiber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add this if outside the it? That way this test isn't counted into the total.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebmarkbage Thanks! I've updated it. But In case of ReactDOMFeatureFlags.useFiber is false, this test is failed because of Your test suite must contain at least one test..
So I added some tests that can pass regardless the fiber flag.
Does this make sense?

@sebmarkbage
Copy link
Collaborator

That makes sense! Thanks.

@sebmarkbage sebmarkbage merged commit a6728f9 into facebook:master Oct 21, 2016
@koba04
Copy link
Contributor Author

koba04 commented Oct 21, 2016

Thanks!

@koba04 koba04 deleted the add-tests-for-react-dom-fiber branch October 21, 2016 07:12
acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
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.

4 participants