Skip to content

test(getComponentName): Increase test coverage #18149

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.

Sign up for GitHub

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

Jump to bottom
Merged
bvaughn merged 16 commits into from
Mar 17, 2020
Merged

test(getComponentName): Increase test coverage #18149

bvaughn merged 16 commits into from
Mar 17, 2020

Conversation

@eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Feb 27, 2020 •
edited
Loading

Summary

Increases coverage of getComponentName to increase likely-hood of #15638 being accepted.

The usages of getComponentName that are still uncovered are included in 2979a97. They either affect react-native (don't know how this is tested), affect messages for possibly internal bugs ("file an issue") or aren't tested to begin with.

It seems like

react/packages/react-dom/src/server/ReactPartialRenderer.js

Line 1321 in abc8fa9

info += '\n\nCheck the render method of `' + ownerName + '`.';
is actually not reachable. Looking at

react/packages/react-dom/src/server/ReactPartialRenderer.js

Line 124 in abc8fa9

const ownerName = null;
it seems like the owner name is forcefully set to null in other cases and I can't find any references to owners in react-dom/server. Looks like owners aren't avaiable in the server renderer and therefore

react/packages/react-dom/src/server/ReactPartialRenderer.js

Line 1321 in abc8fa9

info += '\n\nCheck the render method of `' + ownerName + '`.';
is dead code?

Test Plan

  1. Replace getComponentName(**) with null
  2. If tests pass then mark as fixme and push to verify with full CI
  3. If tests fail then this line is covered. Everything is fine

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 27, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1fae59a:

Sandbox Source
upbeat-tereshkova-p78ez Configuration

@sizebot
Copy link

sizebot commented Feb 27, 2020 •
edited
Loading

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 1fae59a

@sizebot
Copy link

sizebot commented Feb 27, 2020 •
edited
Loading

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 1fae59a

@eps1lon eps1lon force-pushed the test/getComponentName branch 3 times, most recently from febda60 to 3b1dfd2 Compare March 4, 2020 00:08
@eps1lon eps1lon marked this pull request as ready for review March 4, 2020 00:10
@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 4, 2020

Hey @bvaughn is this ok if I ping you to review this? Seems like this concerns more of your work (dev tooling).

@eps1lon eps1lon force-pushed the test/getComponentName branch from 3b1dfd2 to 4852efd Compare March 13, 2020 14:44
bvaughn
bvaughn reviewed Mar 17, 2020
View reviewed changes
packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js Outdated
expect(() => ReactDOMServer.renderToString(<MyComponent />)).toWarnDev(
'componentWillMount has been renamed, and is not recommended for use. See https://fb.me/react-unsafe-component-lifecycles for details.\n\n' +
'* Move code from componentWillMount to componentDidMount (preferred in most cases) or the constructor.\n\n' +
'Please update the following components: MyComponent',
Copy link
Contributor

@bvaughn bvaughn Mar 17, 2020

Choose a reason for hiding this comment

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

I'm not sure I understand why we updated this test. This test isn't about verifying warnings. It's testing behavior, and the toWarnDev() assertions are just there to suppress the (expected) warnings.

Copy link
Collaborator Author

@eps1lon eps1lon Mar 17, 2020

Choose a reason for hiding this comment

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

Hm, let me check the git blame for when this warning was added. Might be I got confused or the warning message wasn't checked in the first place.

Copy link
Contributor

@bvaughn bvaughn Mar 17, 2020

Choose a reason for hiding this comment

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

No worries. I think maybe just revert this file and I'll merge the rest?

Copy link
Collaborator Author

@eps1lon eps1lon Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok so the reason I chose this test was that it was the first one I found that hit

react/packages/react-dom/src/server/ReactPartialRenderer.js

Line 577 in 4b5cb6b

if (!didWarnAboutDeprecatedWillMount[componentName]) {
. But I agree that this is the wrong place. There's already a better place. 1fae59a should resolve this issue.

Copy link
Contributor

@bvaughn bvaughn Mar 17, 2020

Choose a reason for hiding this comment

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

Agreed, the new location seems better.

@eps1lon eps1lon force-pushed the test/getComponentName branch from 0ed6d5d to 1fae59a Compare March 17, 2020 21:02
@bvaughn bvaughn merged commit 22cab1c into facebook:master Mar 17, 2020
@eps1lon eps1lon deleted the test/getComponentName branch March 17, 2020 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bvaughn bvaughn bvaughn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eps1lon @sizebot @bvaughn @facebook-github-bot