passing options instead of cb to spawnCommand #685
Conversation
|
Mind adding unit test to make sure it doesn't break in the future. |
SBoudrias
force-pushed
the
master
branch
2 times, most recently
from
f7728bd
to
9df7c62
Compare
|
Thanks @SBoudrias I am looking and trying to figure out how the install.js unit tests work. Will update when I figure that out! |
|
Changes Unknown when pulling 796c85a on grawk:master into * on yeoman:master*. |
|
Changes Unknown when pulling d635024 on grawk:master into * on yeoman:master*. |
| return asyncStub; | ||
| }.bind(this); | ||
| }); | ||
|
|
||
| describe('#runInstall()', function () { |
There was a problem hiding this comment.
Add an empty line between describe blocks.
|
Changes Unknown when pulling 39bef57 on grawk:master into * on yeoman:master*. |
|
@SBoudrias I converted from using the commandsRun array to using |
|
Changes Unknown when pulling 9fd0d8b on grawk:master into * on yeoman:master*. |
| }; | ||
| //args: installer, paths, options, cb | ||
| this.dummy.runInstall('nestedScript', ['path1', 'path2'], spawnEnv, done); | ||
| assert.ok(this.spawnCommandStub.calledWithExactly('nestedScript', ['install', 'path1', 'path2'], spawnEnv)); |
There was a problem hiding this comment.
Change every assertions to this structure: sinon.assert.calledWithExactly(this.spawnCommandStub, 'nestedScript', ['install', 'path1', 'path2'], spawnEnv)
|
You can see this page about sinon assertions |
|
Changes Unknown when pulling 524bd41 on grawk:master into * on yeoman:master*. |
|
Hey, that's perfect! Just squash you commits (with a useful commit message) and I'll merge! |
|
Changes Unknown when pulling efa7846 on grawk:master into * on yeoman:master*. |
|
Sorry for additional churn on the commits. I'm learning how to "squash" them and causing Travis builds in the process |
…asserts to install unit tests
|
Perfect, thanks a lot! |
passing options instead of cb to spawnCommand
|
Thanks! Can you please let me know what the timeline would be for seeing this in a published release of the generator? |
|
I'd say a couple weeks to a month. We're currently working toward 0.18 so we need to make some new feature more solid before we can consider a release. When we're in patching mode release go out faster, you're just not falling in a good phase for path releases. |
|
OK. That's good to know. Thanks. The reason I took this on to begin with is that my project's generator currently requires a globally installed bower instance in order to work. But we want to instead rely upon an internally require'd instance, which in turn requires modifying the PATH of the child process. In the meantime we will continue to document to our users that they should have globally installed bower. Looking forward to the release to remove that dependency upon our users :) Thanks again. |
|
@SBoudrias upon further thinking on it, do you think it may make sense to eliminate bower as a global dependency at the generator level? I.e. modify the bowerInstall method to add a local bower instance to the PATH environment variable? |
|
Maybe open another issue to discuss it. I'm not sure it's a good idea, but maybe the rest of the team have other opinions. |
I was puzzled by why 'cb' was being passed to the spawnCommand method. I assume this was a typo? With the proposed change, you can now pass in options to the spawned command in order to do things like modify the PATH variable for the spawn'd process, etc.