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

Addon-contexts: component tests and readability improvements #6716

Merged
merged 3 commits into from
May 22, 2019

Conversation

leoyli
Copy link
Contributor

@leoyli leoyli commented May 4, 2019

Issue: #6559

What I did

  • Added unit tests for manager components
  • Improved tests readability and typing

How to test

$ yarn test.

@leoyli leoyli added maintenance User-facing maintenance tasks addon: contexts labels May 4, 2019
@leoyli leoyli added this to the 5.1.0 milestone May 4, 2019
@leoyli leoyli requested review from shilman and CodeByAlex May 4, 2019 23:05
@leoyli leoyli self-assigned this May 4, 2019
@vercel
Copy link

vercel bot commented May 4, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-add-addon-contextscomponent-tests.storybook.now.sh

@leoyli leoyli changed the title Addon-contexts: component tests and readability improvements WIP: Addon-contexts: component tests and readability improvements May 4, 2019
@leoyli
Copy link
Contributor Author

leoyli commented May 4, 2019

For some reason the build was failed but it was okay on my local machine... WIP

@leoyli leoyli force-pushed the add/addon-contexts/component-tests branch from 2579d85 to 191d294 Compare May 5, 2019 00:16
@leoyli leoyli changed the title WIP: Addon-contexts: component tests and readability improvements Addon-contexts: component tests and readability improvements May 5, 2019
@codecov
Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #6716 into next will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6716      +/-   ##
==========================================
+ Coverage   39.92%   40.11%   +0.18%     
==========================================
  Files         645      645              
  Lines        8884     8885       +1     
  Branches      666      667       +1     
==========================================
+ Hits         3547     3564      +17     
+ Misses       5238     5221      -17     
- Partials       99      100       +1
Impacted Files Coverage Δ
...contexts/src/manager/components/ToolBarControl.tsx 63.63% <100%> (ø)
addons/contexts/src/manager/components/ToolBar.tsx 100% <100%> (+100%) ⬆️
addons/contexts/src/shared/serializers.ts 100% <100%> (ø) ⬆️
...ns/contexts/src/manager/components/ToolBarMenu.tsx 100% <0%> (+100%) ⬆️
...exts/src/manager/components/ToolBarMenuOptions.tsx 100% <0%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4fa9c2...e62d251. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented May 5, 2019

Codecov Report

Merging #6716 into next will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6716      +/-   ##
==========================================
+ Coverage   39.92%   40.11%   +0.18%     
==========================================
  Files         645      645              
  Lines        8884     8885       +1     
  Branches      666      667       +1     
==========================================
+ Hits         3547     3564      +17     
+ Misses       5238     5221      -17     
- Partials       99      100       +1
Impacted Files Coverage Δ
...contexts/src/manager/components/ToolBarControl.tsx 63.63% <100%> (ø)
addons/contexts/src/manager/components/ToolBar.tsx 100% <100%> (+100%) ⬆️
addons/contexts/src/shared/serializers.ts 100% <100%> (ø) ⬆️
...ns/contexts/src/manager/components/ToolBarMenu.tsx 100% <0%> (+100%) ⬆️
...exts/src/manager/components/ToolBarMenuOptions.tsx 100% <0%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4fa9c2...e62d251. Read the comment docs.

describe('Tests on addon-contexts component: ToolBar', () => {
it('should render nothing if receive an empty contextNodes', () => {
// when
const result = shallow(<ToolBar nodes={[]} state={{}} setSelected={jest.fn} />);
Copy link
Member

Choose a reason for hiding this comment

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

Are these better expressed as storybook stories & tested with storyshots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shilman, I was thinking about this previously, and even try to use react-testing-library at very beginning... But later I found if I really want to test them, I have to find its ThemeProvider first; otherwise, I will get some object node missed and React get crashed.

That is why I roll it back to more traditional shallow testing approach (to stay inside the addon itself). Right now everything is in the monorepo and user will have to use @storybook/theme implicitly so I guess it should be fine?! But I somehow think the @storybook/components should have some components out-of-the-box for the addon creator.

To be more specific on the scope here, the only thing really need to get tested is the ToolBarControl component. This component is responsible for validating the selected name (and some states as you may see there). I added other testing for parity.

There are some part I want to test but not get it tested. Anyway I will need to rethink about testing here. Thanks for suggestion though 🙏.

Copy link
Member

Choose a reason for hiding this comment

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

Check out my addon improvements PR @leoyli

#6759

It adds a component for addon creation, If you have idea on what components would be useful for addon creators, let's chat about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woo nice, I will take a look, I will also migrate @storybook/components first, and reach you out 👍

@ndelangen ndelangen merged commit bac26d1 into next May 22, 2019
@ndelangen ndelangen deleted the add/addon-contexts/component-tests branch May 22, 2019 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: contexts maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants