Skip to content

[Fiber] Add unit tests for ReactDOMFiber#8016

Merged
sebmarkbage merged 1 commit intofacebook:masterfrom
koba04:add-tests-for-react-dom-fiber
Oct 21, 2016
Merged

[Fiber] Add unit tests for ReactDOMFiber#8016
sebmarkbage merged 1 commit intofacebook:masterfrom
koba04:add-tests-for-react-dom-fiber

Conversation

@koba04
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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?

@koba04 koba04 force-pushed the add-tests-for-react-dom-fiber branch from 3c4aac4 to 8686573 Compare October 21, 2016 00:52
@sebmarkbage
Copy link
Copy Markdown
Contributor

That makes sense! Thanks.

@sebmarkbage sebmarkbage merged commit a6728f9 into facebook:master Oct 21, 2016
@koba04
Copy link
Copy Markdown
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