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

Add addon jest #2295

Merged
merged 38 commits into from
Nov 23, 2017
Merged

Add addon jest #2295

merged 38 commits into from
Nov 23, 2017

Conversation

renaudtertrais
Copy link
Contributor

Issue:

What I did

Add addon-jest to monorepo

How to test

In storybook example

@Hypnosphi
Copy link
Member

Hypnosphi commented Nov 12, 2017

@shilman is a new addon considered a "small feature"?

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

yarn.lock should be removed as well: we use yarn workspaces feature, so all the dependencies are installed by root yarn bootstrap command

@@ -0,0 +1,2 @@
node_modules
dist
Copy link
Member

Choose a reason for hiding this comment

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

Those should be covered by root .eslintignore file

},
},
};

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to use our root eslint config for this package, and override only the things that really need it, if any

require('../List.story');
}

configure(loadStories, module);
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the stories to example/cra-kitchen-sink

Copy link
Member

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am on it.

},
"scripts": {
"prebuild": "npm run clear",
"build": "cross-env NODE_ENV=production babel -d ./dist ./src",
Copy link
Member

Choose a reason for hiding this comment

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

We need a prepare script instead (see other addons). All the babel-related stuff can be removed from package as we use top-level babel dependencies and setup for transpiling everything

"jest": "^21.2.1",
"react": "^16.1.0",
"react-dom": "^16.1.0",
"style-loader": "^0.19.0"
},
"peerDependencies": {
"@storybook/addons": "^3.2.14",
"prop-types": "^15.6.0",
Copy link
Member

Choose a reason for hiding this comment

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

It should be a simple dependency, not a peer one

Copy link
Member

Choose a reason for hiding this comment

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

I actually meant the prop-types package. It's OK for an end user not to have it

@codecov
Copy link

codecov bot commented Nov 14, 2017

Codecov Report

Merging #2295 into master will decrease coverage by 1.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2295     +/-   ##
=========================================
- Coverage   21.23%   20.14%   -1.1%     
=========================================
  Files         283      293     +10     
  Lines        6155     6489    +334     
  Branches      727      755     +28     
=========================================
  Hits         1307     1307             
- Misses       4298     4595    +297     
- Partials      550      587     +37
Impacted Files Coverage Δ
addons/jest/src/components/Result.js 0% <0%> (ø)
addons/jest/src/register.js 0% <0%> (ø)
addons/jest/src/hoc/provideJestResult.js 0% <0%> (ø)
lib/components/src/theme.js 0% <0%> (ø) ⬆️
addons/jest/src/components/Panel.js 0% <0%> (ø)
addons/jest/register.js 0% <0%> (ø)
addons/jest/src/index.js 0% <0%> (ø)
lib/components/src/index.js 0% <0%> (ø) ⬆️
addons/jest/src/styles.js 0% <0%> (ø)
addons/jest/styles.js 0% <0%> (ø)
... and 44 more

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 37c1645...70d3424. Read the comment docs.

@ndelangen
Copy link
Member

Ready to merge I think? @Hypnosphi ?

@Hypnosphi
Copy link
Member

Please move prop-types to dependencies

@ndelangen
Copy link
Member

Latest screenshot:
screen shot 2017-11-19 at 00 34 56

@Hypnosphi
Copy link
Member

Actually, I still don't get what's happening here =( Why those green and red bars have different length? Why storybook is a place to show test results in a fancy way? Why would anybody want to make empty stories?

```

**Known issue**: if you use a deploy script using for example `gh-pages`, be sure to not put
the `test` script that write the result in part of the script process (in `predeploy` for example).
Copy link
Member

Choose a reason for hiding this comment

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

Why?


Brings Jest results in storybook.

[![Storybook Jest Addon Demo](@storybook/addon-jest.gif)](https://@storybook/addon-jest-example.herokuapp.com/)
Copy link
Member

Choose a reason for hiding this comment

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

The link is broken

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

What's up with the SimpleList CI failures? Looks like some files didn't get checked in?

Aside from that, this looks great. @Hypnosphi this is a new feature so technically it should probably go into 3.3, but it's isolated and shouldn't break anything so I'm also OK with it to go into a patch.

Very excited to get this into peoples' hands in one way or another and look forward to getting a cleaned up version merged soon after the changes have been addressed.

@Hypnosphi
Copy link
Member

it's isolated and shouldn't break anything

Actually isolation is an issue here. Once someone adds this addon for some of the stories, nobody can start-storybook in that project without running those tests locally first.

}
```

Add it the result file to `.gitignore`:
Copy link
Member

@Hypnosphi Hypnosphi Nov 19, 2017

Choose a reason for hiding this comment

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

@renaudtertrais @ndelangen why do we even have this recommendation? Checking this file into VCS makes a lot of sense to me, given that it's used in an import. What's the downside?

Copy link
Member

Choose a reason for hiding this comment

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

Well, it will likely have tons of merge-conflicts over time?

Copy link
Member

Choose a reason for hiding this comment

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

Just like lockfiles and test snapshots. It's easy to regenerate them, so this shouldn't be a problem

Copy link
Member

Choose a reason for hiding this comment

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

true

I will change the text and make it so users are aware of the trade-offs.

@ndelangen
Copy link
Member

@shilman, @Hypnosphi I think I've resolved all issues? Can you accept?

@ndelangen
Copy link
Member

@shilman The thing you mentioned has been resolved, I hope you're OK with me merging this.

@ndelangen ndelangen merged commit 5b86063 into master Nov 23, 2017
@ndelangen ndelangen deleted the add-addon-jest branch November 23, 2017 21:40
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.

5 participants