-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Increase code covergae for OverFlowMenuV2 #17884
Increase code covergae for OverFlowMenuV2 #17884
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17884 +/- ##
==========================================
+ Coverage 84.06% 84.16% +0.09%
==========================================
Files 404 404
Lines 14350 14350
Branches 4599 4599
==========================================
+ Hits 12064 12077 +13
+ Misses 2124 2112 -12
+ Partials 162 161 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks for taking a look at this! Rather than mocking the various components, these tests would be more valuable if they ensured the real code works as expected. Meaning if you render()
something like what's shown/documented in storybook:
<OverflowMenuV2>
<CustomMenuItem />
<MenuItem label="Stop app" />
<MenuItemDivider />
<MenuItem label="Delete app" kind="danger" />
</OverflowMenuV2>
We'd want to assert that
console.warn
is called as expected with the deprecation warninguseFeatureFlag('enable-v12-overflowmenu')
returnstrue
from withinCustomMenuItem
- Everything is passed through and spread onto the new implementation of OverflowMenu, meaning new OverflowMenu props such as
autoAlign
are able to be set and have the proper result (auto align class is present) and the children MenuItem are rendered okay likeexpect(getByText('Stop app')).toBeInTheDocument();
This way the only thing you'll need to mock is the implementation of console.warn.
Hi @tay1orjones apologies for the delayed response on this PR. Could you kindly verify if my tests align with your suggestions? Thanks in advance! |
Hey @soumyaraju this looks great, thank you so much for contributing , just two minor things needs attention |
Hey @2nikhiltom, thank you for reviewing it, could you let me know what changes require? |
packages/react/src/components/OverflowMenuV2/OverFlowMenuV2-test.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/OverflowMenuV2/OverFlowMenuV2-test.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/OverflowMenuV2/OverFlowMenuV2-test.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/OverflowMenuV2/OverFlowMenuV2-test.js
Outdated
Show resolved
Hide resolved
packages/react/src/components/OverflowMenuV2/OverFlowMenuV2-test.js
Outdated
Show resolved
Hide resolved
Thanks @2nikhiltom for the quick review, i have made the changes, please verify it. |
8502d7a
Closes #17507
Increase code coverage for overflowmenuv2