-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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 tests for CLI #1767
Add tests for CLI #1767
Conversation
Speaking of node 4/6 testing on CI, the trick is that we should bootstrap with node 8 (as it's the version we support for contributing), and then run tests with an older version. I tried to use some docker images with |
Codecov Report
@@ Coverage Diff @@
## release/3.3 #1767 +/- ##
===============================================
- Coverage 23.32% 22.46% -0.86%
===============================================
Files 281 321 +40
Lines 6045 6276 +231
Branches 706 789 +83
===============================================
Hits 1410 1410
- Misses 4127 4269 +142
- Partials 508 597 +89
Continue to review full report at Codecov.
|
"regenerator": 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.
I believe this file was added by mistake: https://github.com/storybooks/storybook/pull/911/files#diff-c2e6a73df4341abfc1cce840a8008e9e
Why do we have to test on older node versions? |
How about because node 4 is maintained until April 2018? |
# Conflicts: # package.json # yarn.lock
lib/cli/package.json
Outdated
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/storybooks/storybook.git" | ||
}, | ||
"scripts": { | ||
"pretest": "cd test && ./update_fixtures.sh", |
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.
Can we set this up so this is a jest project, like how the examples/react-native-vanilla is set up?
This would allow running the tests along with all the other unit tests!
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.
I'll take a look
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.
all the other unit tests!
These tests are not unit, they're end-to-end =)
}, | ||
"devDependencies": { | ||
"check-node-version": "2.1.0", | ||
"npx": "9.6.0" |
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.
Is this used anywhere within our code? I can't find it in this diff.
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.
Yes, for running fresh CR(N)A without installing globally (yarn create
unfortunately does the global install): https://github.com/storybooks/storybook/blob/cac45b496463180d957780d5ca0a142321658626/lib/cli/test/update_fixtures.sh#L14
@@ -0,0 +1,41 @@ | |||
#!/bin/bash |
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 runs tests?
I don't understand what's going on here.
Can't we write jest tests and run them in jest?
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 this needs to be this way, we need a good documentation on what this code does, either in a readme.md
or above code.
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.
Yes we can, but it's much easier to do those manipulations in bash then in js, plus it's straightforward to reproduce particular steps: you just copypaste them to your console. My inspiration were CRA's e2e tests: https://github.com/facebookincubator/create-react-app/tree/master/tasks
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.
I have added a section to CONTRIBUTING.md
, and some inline comments here
I think the rest can be addressed in separate PRs |
Fixes #1746
How to test
start-storybook
(depends on Add smoke-test option to start-storybook commands #1790)