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.

Sign up for GitHub

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

Jump to bottom

add tests for NpmUtilities.getExecOpts() #663

Merged
merged 3 commits into from Mar 14, 2017
Merged

add tests for NpmUtilities.getExecOpts() #663

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c73e40
registry flag fix with additional tests
noherczeg Mar 11, 2017
87558bc
removed commented code
noherczeg Mar 12, 2017
6d8c0fd
Merge branch 'master' into registry-flag-handling-fix
evocateur Mar 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/NpmUtilities.js
View file
Open in desktop
Expand Up @@ -23,7 +23,9 @@ export default class NpmUtilities {
};

if (registry) {
opts.env = {npm_config_registry: registry};
opts.env = Object.assign({}, process.env, {
npm_config_registry: registry,
});
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do in the evening!

}

const packageJson = path.join(directory, "package.json");
Expand Down
16 changes: 15 additions & 1 deletion test/BootstrapCommand.js
View file
Open in desktop
Expand Up @@ -765,20 +765,34 @@ describe("BootstrapCommand", () => {

describe("registries", () => {
let testDir;
const originalEnv = Object.assign({}, process.env);
const mockEnv = {
mock_value: 1,
NODE_ENV: "lerna-test",
};

beforeEach((done) => {
testDir = initFixture("BootstrapCommand/registries", done);
});

afterEach(() => {
process.env = originalEnv;
});

it("should use config property", (done) => {
// mock out the ENV to a simpler version for testing
process.env = mockEnv;
const bootstrapCommand = new BootstrapCommand([], {});
const env = Object.assign({}, mockEnv, {
npm_config_registry: "https://my-secure-registry/npm",
});

bootstrapCommand.runValidations();
bootstrapCommand.runPreparations();

assertStubbedCalls([
[ChildProcessUtilities, "spawn", { nodeCallback: true }, [
{ args: ["npm", ["install"], { cwd: path.join(testDir, "packages", "package-1"), stdio: STDIO_OPT, env: { npm_config_registry: "https://my-secure-registry/npm" } }] }
{ args: ["npm", ["install"], { cwd: path.join(testDir, "packages", "package-1"), stdio: STDIO_OPT, env }] }
]]
]);

Expand Down
24 changes: 24 additions & 0 deletions test/NpmUtilities.js
View file
Open in desktop
Expand Up @@ -21,4 +21,28 @@ describe("NpmUtilities", () => {
});
});
});

describe(".getExecOpts()", () => {
const originalEnv = Object.assign({}, process.env);
const mockEnv = {
mock_value: 1,
NODE_ENV: "lerna-test",
};

afterEach(() => {
process.env = originalEnv;
});

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"})};
Copy link
Member

Choose a reason for hiding this comment

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

Let's spread this across multiple lines for readability, please.

assert.deepEqual(NpmUtilities.getExecOpts("test_dir", "https://my-secure-registry/npm"), want);
});

it("should handle missing environment variables", () => {
process.env = mockEnv;
const want = {cwd: "test_dir"};
assert.deepEqual(NpmUtilities.getExecOpts("test_dir"), want);
});
});
});