add tests for NpmUtilities.getExecOpts() #663
Conversation
src/NpmUtilities.js
Outdated
| opts.env = {npm_config_registry: registry}; | ||
| opts.env = Object.assign({}, process.env, { | ||
| npm_config_registry: registry, | ||
| }); |
There was a problem hiding this comment.
I was playing with NpmUtilities.getExecOpts() locally during the contentious rebasing process yesterday, and I could have sworn I added it here, too. (technically ChildProcessUtilities.spawn() in this method, but same diff as far as the opts go). I think we should take the opportunity to reuse the new getExecOpts() method:
const opts = NpmUtilities.getExecOpts(directory, registry);
opts.stdio = ["ignore", "pipe", "pipe"];
Like all good refactorings, the rest of your (much appreciated!) tests won't need to be further modified. :)
(we can leave the other exec opts cwd and env in this file alone for now, they're not aware of the registry for the moment)
There was a problem hiding this comment.
Sure, will do in the evening!
test/NpmUtilities.js
Outdated
|
|
||
| it("should handle environment variables properly", () => { | ||
| process.env = mockEnv; | ||
| const want = {cwd: "test_dir", env: Object.assign({}, mockEnv, {npm_config_registry: "https://my-secure-registry/npm"})}; |
There was a problem hiding this comment.
Let's spread this across multiple lines for readability, please.
evocateur
changed the title
|
This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
No description provided.