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

Update dependencies #399

Merged
merged 1 commit into from
Aug 18, 2016
Merged

Update dependencies #399

merged 1 commit into from
Aug 18, 2016

Conversation

KyleAMathews
Copy link
Contributor

No description provided.

@KyleAMathews
Copy link
Contributor Author

Man... the tests time out on Macs like everytime. If someone wants to debug them lemme know but for now I'm going to turn them off.

@KyleAMathews KyleAMathews merged commit 39b9e8e into master Aug 18, 2016
@benstepp
Copy link
Contributor

Installing the starter has too many outside dependencies to be a really good measure. With our massive dependency tree it probably has at least 1000+ http requests, each with a chance of failure. As well as a lower perf of running the command inside of a spawned child_process with babel-node inside of a container.

A better test would focus on the CLI's ability to download the starter, and not test the integrity of the containerized filesystem or the response time of npm. Maybe a --no-install flag should be introduced for the new command to be used for tests, or those who intend on developing through docker, where the npm install is usually a part of the Dockerfile and not the mounted volumes.

Ideally though, code should be concise enough that it can be unit tested to keep the need for integration testing to a minimum.

@KyleAMathews
Copy link
Contributor Author

@benstepp 👍 seems very reasonable. Will be revisiting tests soon once 1.0-alphas start getting released.

@0x80 0x80 deleted the updep2 branch April 19, 2017 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants