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

[CLI] Adds Testing to CLI #727

Merged
merged 3 commits into from Jun 11, 2015
Merged

[CLI] Adds Testing to CLI #727

merged 3 commits into from Jun 11, 2015

Conversation

tommymchugh
Copy link

This PR adds the necessary testing to the install command for the cli.

Testing checks for: Podfile existing, Containing React-Native Tags, and for React Pod to be included.

@tommymchugh tommymchugh closed this Apr 7, 2015
@frantic
Copy link
Contributor

frantic commented Apr 7, 2015

Why did you close this? I don't know if we have this in the docs, but to run the tests do

$ npm test

we use Jest for testing, but unfortunately it doesn't work on node version above 0.10. I use n to switch node binaries, but it's up to you.

Also I believe in order for Jest to find your test it has to be named moduleYouTesting-test.js (at least that's our convention)

@@ -83,6 +83,20 @@ module.exports = {
throw e;
}
}

return {
podfileExists: function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@tommymchugh
Copy link
Author

@frantic Ahh that makes sense, some of it was failing in Travis CI, but working when doing npm test, so I'll figure that out and commit.

@tommymchugh tommymchugh reopened this Apr 7, 2015
@frantic
Copy link
Contributor

frantic commented Apr 7, 2015

@tommchugh I think the problem is in using current dir in both tests and code. cwd() is super unreliable and often leads to weird bugs. Prefer passing around absolute paths and using path.resolve and friends with __dirname, __filename.

For example, your install function can take root as param. In your tests, you can create fake files to simulate different layouts (with or without Podfile, with or without node_modules, etc.). But cli.js will invoke install function with real root dir (either by passing cwd() or __dirname, etc.)

@tommymchugh
Copy link
Author

@frantic Yep that looks like it. All is well with Travis.

@brentvatne
Copy link
Collaborator

@frantic - is this still relevant? seems like a clean merge still if not 😄

@brentvatne brentvatne changed the title Adds Testing to CLI [CLI] Adds Testing to CLI Jun 1, 2015
@sahrens
Copy link
Contributor

sahrens commented Jun 11, 2015

We have a nice end-to-end test, so not sure if still relevant - @frantic: can you either close out or review if you want it?

@sahrens sahrens assigned frantic and tadeuzagallo and unassigned frantic Jun 11, 2015
@sahrens
Copy link
Contributor

sahrens commented Jun 11, 2015

Actually Tadeu's been looking at this stuff more lately and frantic is on vacation...

@tadeuzagallo
Copy link
Contributor

yaytests

Tests are always relevant! 😄
Just re-triggering travis since a lot has changed in the mean time.

tadeuzagallo added a commit that referenced this pull request Jun 11, 2015
[CLI] Adds Testing to CLI
@tadeuzagallo tadeuzagallo merged commit 7c49d29 into facebook:master Jun 11, 2015
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.

None yet

6 participants