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.

Sign up for GitHub

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

Jump to bottom

A CLI for running specific tests suites, like bootstrap CLI #1752

Merged
merged 8 commits into from
Aug 28, 2017
Merged

A CLI for running specific tests suites, like bootstrap CLI #1752

merged 8 commits into from
Aug 28, 2017

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Aug 28, 2017 •
edited
Loading

Issue: #1734

What I did

I improved the 'running-tests'-experience by creating a inquirer script for running the test-suites.

How to test

  1. Install
  2. bootstrap
  3. run yarn test >> should open an inquirer
  4. run yarn test -- --watch >> should start default suites in watchmode
  5. run yarn test -- --core --watch >> should start only core suite in watchmode
  6. run yarn test -- --core --reactnative >> should start core & reactnative suites to completion

It's also possible to run yarn test -- -h to get a list of all options available.

Why

I like this because it makes it discoverable. It also allows contributors to focus on a subset of tests. This is useful in our codebase because changes to for example the react, vue or angular apps can not have any effect on their tests. So if your changes are solely on those parts of the codebase it makes little sense to have to bootstrap the react-native example(s) just to be able to run the unit tests.

I've set ALL the suites as default = true, so that includes the react-native suite! So it's an OPT-OUT to NOT run the tests for react-native. Feedback on this would be appreciated very much!

TODO

  • update documentation
  • change .circleci.yml

Is this testable with jest or storyshots?
Possibly, I'm hoping the reviewers will let me know if they think that would be sensible

Does this need an update to the documentation?
YES, I'll do that after I hear positive reactions on the feature itself

Merging this would fix: #1734

@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Aug 28, 2017
@ndelangen ndelangen self-assigned this Aug 28, 2017
@ndelangen ndelangen requested review from freeman14, Hypnosphi, usulpro and a team August 28, 2017 15:18
danielduan
danielduan approved these changes Aug 28, 2017
View reviewed changes
Copy link
Member

@danielduan danielduan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I tested both cra and react-native. Looks good, really appreciate this!

Can we perhaps have an option to run it with the -u to update snapshots?

@codecov
Copy link

codecov bot commented Aug 28, 2017 •
edited
Loading

Codecov Report

Merging #1752 into release/3.3 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/3.3    #1752   +/-   ##
============================================
  Coverage        23.12%   23.12%           
============================================
  Files              253      253           
  Lines             5756     5756           
  Branches           686      689    +3     
============================================
  Hits              1331     1331           
+ Misses            3920     3916    -4     
- Partials           505      509    +4
Impacted Files Coverage Δ
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 21% <0%> (ø) ⬆️
...es__/update-addon-info/update-addon-info.output.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 30.98% <0%> (ø) ⬆️
addons/info/src/components/markdown/htags.js 30% <0%> (ø) ⬆️
addons/knobs/src/KnobManager.js 32% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/down_panel.js 23.52% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 8.51% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
... and 10 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 44d16bf...a8f7942. Read the comment docs.

@ndelangen
WIP
e7660ac
@ndelangen
ADD CLI for running tests, supports jest-projects and passing params …
45476de 
…to jest

Could be expanded later to run a lots of test, and allows us to pick what tests we want to run
via command line flags or a GUI.
@ndelangen
ADD some jest arguments
6190aca
@ndelangen
CHANGE docs (CONTRIBUTIONS) to reflect changed to test script && CHAN…
b64af7b 
…GE to use yarn
danielduan
danielduan approved these changes Aug 28, 2017
View reviewed changes
CONTRIBUTING.md
```sh
git clone https://github.com/storybooks/storybook.git
cd storybook
yarn install
Copy link
Member

Choose a reason for hiding this comment

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

I think @Hypnosphi opted to use the shorthand yarn notation. We should keep it consistent

.circleci/config.yml Outdated
@@ -147,8 +147,8 @@ jobs:
- run:
name: "Unit testing"
command: |
yarn test -- --coverage -i
yarn coverage
npm run test -- --all --coverage --runInBand
Copy link
Member

Choose a reason for hiding this comment

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

If yarn works on circleci, I think we should keep it consistent there as well.

Hypnosphi
Hypnosphi requested changes Aug 28, 2017
View reviewed changes
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.

Same concerns as @danielduan's, otherwise LGTM

usulpro
usulpro approved these changes Aug 28, 2017
View reviewed changes
Copy link
Member

@usulpro usulpro left a comment

Choose a reason for hiding this comment

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

Works fine!
Maybe it worth to add an option for testing a certain package? And do same for test coverage option

@ndelangen
Copy link
Member Author

@Hypnosphi should be fixed!

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 28, 2017 •
edited
Loading

And yarn shorthand (for yarn install), please

@ndelangen
Copy link
Member Author

ndelangen commented Aug 28, 2017 •
edited
Loading

I like the explicitness of yarn install actually..

I don't see a reason to make use of the shorthand when not typing by hand.

@Hypnosphi
Copy link
Member

Change it to this everywhere then =) Let's be consistent with it

@ndelangen ndelangen changed the title Ndelangen/testing cli A CLI for running specific tests suites, like bootstrap CLI Aug 28, 2017
@Hypnosphi Hypnosphi merged commit c9852d0 into release/3.3 Aug 28, 2017
@Hypnosphi Hypnosphi deleted the ndelangen/testing-cli branch August 28, 2017 22:14
@shilman shilman mentioned this pull request Aug 31, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielduan danielduan danielduan approved these changes

@Hypnosphi Hypnosphi Hypnosphi approved these changes

@usulpro usulpro usulpro approved these changes

@freeman14 freeman14 Awaiting requested review from freeman14

Assignees

@ndelangen ndelangen

Labels
maintenance User-facing maintenance tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ndelangen @Hypnosphi @danielduan @usulpro