search

shelljs / shx

Portable Shell Commands for Node
MIT License
1.73k stars 45 forks source link

Switch to mocha #31

Closed levithomason closed 8 years ago

levithomason commented 8 years ago

Fixes #22

levithomason commented 8 years ago

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

nfischer commented 8 years ago

@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 commented 8 years ago

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

nfischer commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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

levithomason commented 8 years ago

Hm, checking.

nfischer commented 8 years ago

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

levithomason commented 8 years ago

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
shx@0.0.1 /Users/levithomason/src/shx
`-- babel-cli@6.7.7
  `-- babel-register@6.7.2 

› npm ls babel-polyfill
shx@0.0.1 /Users/levithomason/src/shx
`-- babel-cli@6.7.7
  `-- babel-polyfill@6.7.4
nfischer commented 8 years ago

After running npm install on Linux:

npm WARN package.json shx@0.0.1 No bin file found at lib/cli.js
npm WARN optional dep failed, continuing fsevents@1.0.11
npm WARN deprecated jade@0.26.3: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated graceful-fs@2.0.3: 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
shx@0.0.1 /home/nate/PR_31/node_modules/shx
├─┬ babel-cli@6.7.7
│ └── babel-register@6.7.2
└─┬ babel-preset-es2015@6.6.0
  └─┬ babel-plugin-transform-regenerator@6.6.5
    └─┬ babel-core@6.7.7
      └── babel-register@6.7.2

$ npm ls babel-polyfill
shx@0.0.1 /home/nate/PR_31/node_modules/shx
└─┬ babel-cli@6.7.7
  └── babel-polyfill@6.7.4

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

LGTM. Passes for both node v4 and v5.

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