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

don't mistake directories for executables #16

Merged
merged 1 commit into from
Apr 16, 2016

Conversation

boneskull
Copy link
Contributor

  • add assert package and rimraf package for test

I ran into a problem wherein scripty thought a directory was a script. Unfortunately, I'm unable to reproduce this in the context of the integration tests, though I tried.

  • I have a scripts/test/browser/ dir. That dir has an index.sh and a dev.sh.
  • The scripts/test/ dir has an index.sh which invokes test:browser, test:node, test:lint, etc.
  • Somewhere in there, scripty thinks scripts/test/browser/ is a script to be executed:
Error: scripty - failed trying to read "/Volumes/alien/mochajs/mocha-core/scripts/test/browser":

EISDIR: illegal operation on a directory, read
internal/child_process.js:302
    throw errnoException(err, 'spawn');
    ^

Error: spawn EACCES
    at exports._errnoException (util.js:890:11)
    at ChildProcess.spawn (internal/child_process.js:302:11)
    at exports.spawn (child_process.js:367:9)
    at /Volumes/alien/forked/scripty/lib/scripty.js:30:17
    at /Volumes/alien/forked/scripty/node_modules/async/lib/async.js:718:13
    at iterate (/Volumes/alien/forked/scripty/node_modules/async/lib/async.js:262:13)
    at async.forEachOfSeries.async.eachOfSeries (/Volumes/alien/forked/scripty/node_modules/async/lib/async.js:281:9)
    at _parallel (/Volumes/alien/forked/scripty/node_modules/async/lib/async.js:717:9)
    at Object.async.series (/Volumes/alien/forked/scripty/node_modules/async/lib/async.js:739:9)
    at /Volumes/alien/forked/scripty/lib/scripty.js:14:11

This changes solves the issue.

Also, I included userland assert, as using the core assert is not recommended by the Node.js team.

- add `assert` package and `rimraf` package for test
@searls
Copy link
Member

searls commented Apr 16, 2016

Hey @boneskull, sorry you encountered this issue and then struggled to reproduce it in your test.

I wasn't aware of that recommendation about assert. I just checked the latest docs, though, and you're totally right:

The assert module provides a simple set of assertion tests that can be used to test invariants. The module is intended for internal use by Node.js, but can be used in application code via require('assert'). However, assert is not a testing framework, and is not intended to be used as a general purpose assertion library.

FWIW, I was actually planning on switching to power-assert tonight, so the second point may end up being moot, but that's not a big deal.

@searls
Copy link
Member

searls commented Apr 16, 2016

LGTM, thanks! ✨

Sorry about the ugliness of that particular test you had to update, btw. It does silly stuff like creating its own files and cleaning them up, which predates having some local fixture directories to try to avoid that. I'll try to clean that up later.

@searls searls merged commit 9e1eaea into testdouble:master Apr 16, 2016
@searls
Copy link
Member

searls commented Apr 16, 2016

Landed in 1.2.2

@boneskull boneskull deleted the executable-dir branch April 16, 2016 03:42
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