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

Switch to mocha #31

Merged
merged 3 commits into from
Apr 22, 2016
Merged

Switch to mocha #31

merged 3 commits into from
Apr 22, 2016

Conversation

levithomason
Copy link
Contributor

@levithomason levithomason commented Apr 20, 2016

Fixes #22

"test:watch": "watch 'npm test --silent' src test"
"posttest": "npm run lint --silent",
"test": "mocha",
"test:watch": "concurrently -rk 'npm run test --silent -- -w' 'npm run lint:watch'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved lint to posttest so the lint results show after the tests run. Helps keep linter happy while testing.

@levithomason levithomason mentioned this pull request Apr 20, 2016
@levithomason
Copy link
Contributor Author

@ariporad any ideas why travis / appveyor are not running on this PR?

@nfischer
Copy link
Member

@levithomason

any ideas why travis / appveyor are not running on this PR?

I believe it's configured to only run if .travis.yml is present, which it isn't on this branch. I will verify that this works for node v4 and node v5 for my system.

@levithomason
Copy link
Contributor Author

Oh, right, it isn't merged yet :)

@nfischer
Copy link
Member

@levithomason I got this error on node v4:

Error: Cannot find module 'babel-register'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:289:25)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at /home/nate/PR_31/node_modules/shx/node_modules/mocha/bin/_mocha:302:3
    at Array.forEach (native)
    ...

@levithomason
Copy link
Contributor Author

Did you try installing deps again, or blowing away and reinstalling? NPM has been having issues with that. babel-register is needed by mocha to compile, however, it is included with babel which is already a dep. This should be working.

@nfischer
Copy link
Member

It looks like babel-register and babel-polyfill are both dependencies that should be added explicitly.

@levithomason
Copy link
Contributor Author

Hm, checking.

@nfischer
Copy link
Member

Thanks! I just used nvm use v4 and then used my github-review script to download a fresh copy of the PR.

@levithomason
Copy link
Contributor Author

Cool, no worries. You are right about the separate packages, but since we have babel-cli, we get those deps as well:

› npm ls babel-register
[email protected] /Users/levithomason/src/shx
`-- [email protected]
  `-- [email protected] 

› npm ls babel-polyfill
[email protected] /Users/levithomason/src/shx
`-- [email protected]
  `-- [email protected] 

@nfischer
Copy link
Member

After running npm install on Linux:

npm WARN package.json [email protected] No bin file found at lib/cli.js
npm WARN optional dep failed, continuing [email protected]
npm WARN deprecated [email protected]: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated [email protected]: graceful-fs version 3 and before will fail on newer node releases. Please update to graceful-fs@^4.0.0 as soon as possible.
...
$ npm test # this command fails, as before
$ npm ls babel-register
[email protected] /home/nate/PR_31/node_modules/shx
├─┬ [email protected]
│ └── [email protected]
└─┬ [email protected]
  └─┬ [email protected]
    └─┬ [email protected]
      └── [email protected]

$ npm ls babel-polyfill
[email protected] /home/nate/PR_31/node_modules/shx
└─┬ [email protected]
  └── [email protected]

@levithomason Can you successfully re-clone, install deps, and test, all using node v4.2.4? If so, I think this may be an OS difference between Linux and OS X.

@nfischer
Copy link
Member

Yeah, retried it again. For me, it all works if those packages are direct dependencies. Please let me know if I'm doing something wrong or if you're seeing something else.

@levithomason
Copy link
Contributor Author

levithomason commented Apr 21, 2016

Confirmed and fixed. npm v3 (node 5) installs modules in a flat tree, which allows requiring deps of other deps. npm v2 (node 4) uses nested node_modules so they cannot be required.

Good catch and thanks for testing, I've added the deps explicitly.

EDIT

In hindsight, it seems crazy to propose we rely on our deps' deps 😬

@nfischer
Copy link
Member

LGTM. Passes for both node v4 and v5.

I'll merge this in and rebase the other PR to see if things are working.

@nfischer nfischer merged commit 1eb8f0f into master Apr 22, 2016
@levithomason levithomason deleted the feature/mocha branch April 22, 2016 00:21
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.

3 participants