-
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
17492: Increase SideNavMenu unit test coverage #17842
base: main
Are you sure you want to change the base?
17492: Increase SideNavMenu unit test coverage #17842
Conversation
Added testcases for SideNavMenu Component that increased the test coverage BREAKING CHANGE: The SideNavMenu now has a test coverage greater than 80%
✅ 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!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks for taking this on! I have a couple points of feedback below, let me know what you think.
it('should return true if the child is a valid React element, and instance of Array and has isActive and aria-current props', () => { | ||
const child = [ | ||
<SideNavMenuItem isActive={true} aria-current="page"> | ||
<SideNavMenuItem isActive={false} aria-current="page"> | ||
a | ||
</SideNavMenuItem> | ||
</SideNavMenuItem>, | ||
<SideNavMenuItem isActive={true} aria-current="page"> | ||
b | ||
</SideNavMenuItem>, | ||
]; | ||
expect(hasActiveDescendant(child)).toBe(true); | ||
}); |
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.
Rather than test the internal implementation like this - how could we reframe the tests from the perspective of someone using the component?
hasActiveDescendant is only called in one place under certain circumstances
carbon/packages/react/src/components/UIShell/SideNavMenu.tsx
Lines 91 to 92 in 0619707
[`${prefix}--side-nav__item--active`]: | |
isActive || (hasActiveDescendant(children) && !isExpanded), |
I think this would mean that to test this from a user-interaction viewpoint, each test could render
the component with a different type of children which would get hasActiveDescendant
to be called, and then the assertion would be toHaveClass('${prefix}--side-nav__item--active')
.
it('should return true if the child is a invalid React element and has isActive props set to true', () => { | ||
const child = <div isActive={true} />; | ||
expect(hasActiveDescendant(child)).toBe(true); | ||
}); |
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.
Same kind of idea here. I don't think we need to directly assert that the function call returns true. Instead it's the end result of the function call we want to ensure happens correctly.
Again I think in nearly all cases this would be an assertion of toHaveClass
or .not.toHaveClass
. The classes are admittedly still an implementation detail, but they're the closest thing to what the user perceives without having to actually assert that certain styles are applied (background, etc. or whatever is in selectors with ${prefix}--side-nav__item--active
it('should return false if child is an invalid React element', () => { | ||
const child = ['abc', 'xyz']; | ||
render( | ||
<SideNavMenu title="test-title"> | ||
<SideNavMenuItem>a</SideNavMenuItem> | ||
<SideNavMenuItem>b</SideNavMenuItem> | ||
<SideNavMenuItem>c</SideNavMenuItem> | ||
</SideNavMenu> | ||
); | ||
expect(hasActiveDescendant(child)).toBe(false); | ||
}); |
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.
This one doesn't use anything that's being rendered.
@@ -200,7 +200,9 @@ SideNavMenu.propTypes = { | |||
Defining the children parameter with the type ReactNode | ReactNode[]. This allows for various possibilities: | |||
a single element, an array of elements, or null or undefined. | |||
**/ | |||
function hasActiveDescendant(children: ReactNode | ReactNode[]): boolean { | |||
export function hasActiveDescendant( |
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.
If the tests are refactored to be user centered this won't need to be exported.
packages/react/src/components/UIShell/__tests__/SideNavMenu-test.js
Outdated
Show resolved
Hide resolved
Thanks for your feedback. Will work on them. |
Added testcases for SideNavMenu Component that increased the test coverage BREAKING CHANGE: The SideNavMenu now has a test coverage greater than 90% - fixes based on feedback
Removed unused import fireEvent from the test file BREAKING CHANGE: Removed unused imports
@tay1orjones , I have updated the PR based on your feedback. Let me know if any other modifications are needed. Thank you. |
Added testcases for SideNavMenu Component that increased the test coverage
BREAKING CHANGE: The SideNavMenu now has a test coverage greater than 80%
Closes #17492
Changelog
Changed
Testing / Reviewing