Fix plugin install error not displayed correctly

CHANGELOG.md

@@ -2,8 +2,13 @@
 
 All notable changes to the Imperative package will be documented in this file.
 
+## Recent Changes
+
+- BugFix: Fixed plugin install error not displayed correctly. [#954](https://github.com/zowe/imperative/issues/954)
+
 ## `5.9.1`
-- BugFix: Fix environment file not applying to daemon client environment variables
+
+- BugFix: Fixed environment file not applying to daemon client environment variables
 
 ## `5.9.0`
 

packages/imperative/__tests__/config/cmd/report-env/EnvQuery.unit.test.ts

@@ -391,7 +391,7 @@ describe("Tests for EnvQuery module", () => {
             });
 
             const cmdOutput = await EnvQuery["getCmdOutput"]("bogusCmd", ["bogusArg"]);
-            expect(cmdOutput).toContain("Failed to run commmand = bogusCmd");
+            expect(cmdOutput).toContain("Failed to run command = bogusCmd");
             expect(cmdOutput).toContain("Pretend this was thrown by spawnSync");
         });
     }); // end getCmdOutput function

packages/imperative/__tests__/plugins/cmd/install/install.handler.test.ts

@@ -49,6 +49,7 @@ import { readFileSync, writeFileSync } from "jsonfile";
 import { PMFConstants } from "../../../../src/plugins/utilities/PMFConstants";
 import { TextUtils } from "../../../../../utilities";
 import { getRegistry, npmLogin } from "../../../../src/plugins/utilities/NpmFunctions";
+import * as childProcess from "child_process";
 
 let expectedVal;
 let returnedVal;
@@ -72,7 +73,7 @@ describe("Plugin Management Facility install handler", () => {
 
     const packageName2 = "b";
     const packageVersion2 = "13.1.2";
-    const packageRegistry2 = "http://isl-dsdc.ca.com/artifactory/api/npm/npm-repo/";
+    const packageRegistry2 = "https://zowe.jfrog.io/zowe/api/npm/npm-release/";
 
     const finalValidationMsg = "The final message from runPluginValidation";
 
@@ -276,7 +277,7 @@ describe("Plugin Management Facility install handler", () => {
 
         const params = getIHandlerParametersObject();
         params.arguments.plugin = ["sample1"];
-        params.arguments.registry = "http://isl-dsdc.ca.com";
+        params.arguments.registry = "http://localhost:4873/";
 
         await handler.process(params as IHandlerParameters);
 
@@ -347,7 +348,30 @@ describe("Plugin Management Facility install handler", () => {
             expectedError = e;
         }
 
+        expect(expectedError).toBeInstanceOf(ImperativeError);
         expect(expectedError.message).toBe("Install Failed");
     });
+
+    it("should handle an error in spawned process", async () => {
+        const handler = new InstallHandler();
+
+        const params = getIHandlerParametersObject();
+        params.arguments.plugin = ["sample1"];
+
+        let expectedError: ImperativeError;
+
+        jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({ status: 1 } as any);
+        mocks.getRegistry.mockImplementationOnce(jest.requireActual("../../../../src/plugins/utilities/NpmFunctions").getRegistry);
+
+        try {
+            await handler.process(params);
+        } catch (e) {
+            expectedError = e;
+        }
+
+        expect(expectedError).toBeInstanceOf(ImperativeError);
+        expect(expectedError.additionalDetails).toContain("Command failed");
+        expect(expectedError.additionalDetails).toContain("npm");
+    });
 });
 

packages/imperative/__tests__/plugins/utilities/NpmFunctions.test.ts

@@ -9,16 +9,63 @@
 *
 */
 
+import * as childProcess from "child_process";
 import * as jsonfile from "jsonfile";
 import * as npmPackageArg from "npm-package-arg";
 import * as pacote from "pacote";
-import { getPackageInfo } from "../../../src/plugins/utilities/NpmFunctions";
+import * as npmFunctions from "../../../src/plugins/utilities/NpmFunctions";
 import { PMFConstants } from "../../../src/plugins/utilities/PMFConstants";
 
+jest.mock("child_process");
 jest.mock("jsonfile");
 jest.mock("pacote");
 
 describe("NpmFunctions", () => {
+    const fakeRegistry = "http://localhost:4873/";
+    const npmCmd = npmFunctions.findNpmOnPath();
+
+    afterEach(() => {
+        jest.clearAllMocks();
+    });
+
+    afterAll(() => {
+        jest.restoreAllMocks();
+    });
+
+    it("installPackages should run npm install command", () => {
+        const stdoutBuffer = Buffer.from("Install Succeeded");
+        jest.spyOn(PMFConstants, "instance", "get").mockReturnValueOnce({ PMF_ROOT: __dirname } as any);
+        const spawnSyncSpy = jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({
+            status: 0,
+            stdout: stdoutBuffer
+        } as any);
+        const result = npmFunctions.installPackages("fakePrefix", fakeRegistry, "samplePlugin");
+        expect(spawnSyncSpy.mock.calls[0][0]).toBe(npmCmd);
+        expect(spawnSyncSpy.mock.calls[0][1]).toEqual(expect.arrayContaining(["install", "samplePlugin"]));
+        expect(spawnSyncSpy.mock.calls[0][1]).toEqual(expect.arrayContaining(["--prefix", "fakePrefix"]));
+        expect(spawnSyncSpy.mock.calls[0][1]).toEqual(expect.arrayContaining(["--registry", fakeRegistry]));
+        expect(result).toBe(stdoutBuffer.toString());
+    });
+
+    it("getRegistry should run npm config command", () => {
+        const stdoutBuffer = Buffer.from(fakeRegistry);
+        const spawnSyncSpy = jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({
+            status: 0,
+            stdout: stdoutBuffer
+        } as any);
+        const result = npmFunctions.getRegistry();
+        expect(spawnSyncSpy.mock.calls[0][0]).toBe(npmCmd);
+        expect(spawnSyncSpy.mock.calls[0][1]).toEqual(["config", "get", "registry"]);
+        expect(result).toBe(stdoutBuffer.toString());
+    });
+
+    it("npmLogin should run npm login command", () => {
+        const spawnSyncSpy = jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({ status: 0 } as any);
+        npmFunctions.npmLogin(fakeRegistry);
+        expect(spawnSyncSpy.mock.calls[0][0]).toBe(npmCmd);
+        expect(spawnSyncSpy.mock.calls[0][1]).toContain("login");
+        expect(spawnSyncSpy.mock.calls[0][1]).toEqual(expect.arrayContaining(["--registry", fakeRegistry]));
+    });
 
     describe("getPackageInfo", () => {
         const expectedInfo = { name: "@zowe/imperative", version: "latest" };
@@ -28,22 +75,14 @@ describe("NpmFunctions", () => {
             jest.spyOn(pacote, "manifest").mockResolvedValue(expectedInfo as any);
         });
 
-        afterEach(() => {
-            jest.clearAllMocks();
-        });
-
-        afterAll(() => {
-            jest.restoreAllMocks();
-        });
-
         it("should fetch info for package installed from registry", async () => {
             const pkgSpec = "@zowe/imperative";
             expect(npmPackageArg(pkgSpec).type).toEqual("tag");
 
             jest.spyOn(PMFConstants, "instance", "get").mockReturnValueOnce({
                 PLUGIN_HOME_LOCATION: ""
             } as any);
-            const actualInfo = await getPackageInfo(pkgSpec);
+            const actualInfo = await npmFunctions.getPackageInfo(pkgSpec);
             expect(actualInfo).toBe(expectedInfo);
             expect(jsonfile.readFileSync).toHaveBeenCalledTimes(1);
         });
@@ -52,7 +91,7 @@ describe("NpmFunctions", () => {
             const pkgSpec = "./imperative";
             expect(npmPackageArg(pkgSpec).type).toEqual("directory");
 
-            const actualInfo = await getPackageInfo(pkgSpec);
+            const actualInfo = await npmFunctions.getPackageInfo(pkgSpec);
             expect(actualInfo).toBe(expectedInfo);
             expect(pacote.manifest).toHaveBeenCalledTimes(1);
         });
@@ -61,7 +100,7 @@ describe("NpmFunctions", () => {
             const pkgSpec = "imperative.tgz";
             expect(npmPackageArg(pkgSpec).type).toEqual("file");
 
-            const actualInfo = await getPackageInfo(pkgSpec);
+            const actualInfo = await npmFunctions.getPackageInfo(pkgSpec);
             expect(actualInfo).toBe(expectedInfo);
             expect(pacote.manifest).toHaveBeenCalledTimes(1);
         });
@@ -70,7 +109,7 @@ describe("NpmFunctions", () => {
             const pkgSpec = "github:zowe/imperative";
             expect(npmPackageArg(pkgSpec).type).toEqual("git");
 
-            const actualInfo = await getPackageInfo(pkgSpec);
+            const actualInfo = await npmFunctions.getPackageInfo(pkgSpec);
             expect(actualInfo).toBe(expectedInfo);
             expect(pacote.manifest).toHaveBeenCalledTimes(1);
         });
@@ -79,7 +118,7 @@ describe("NpmFunctions", () => {
             const pkgSpec = "http://example.com/zowe/imperative.tgz";
             expect(npmPackageArg(pkgSpec).type).toEqual("remote");
 
-            const actualInfo = await getPackageInfo(pkgSpec);
+            const actualInfo = await npmFunctions.getPackageInfo(pkgSpec);
             expect(actualInfo).toBe(expectedInfo);
             expect(pacote.manifest).toHaveBeenCalledTimes(1);
         });

packages/imperative/__tests__/plugins/utilities/npm-interface/uninstall.test.ts

@@ -27,7 +27,7 @@ import { IPluginJson } from "../../../../src/plugins/doc/IPluginJson";
 import { Logger } from "../../../../../logger";
 import { PMFConstants } from "../../../../src/plugins/utilities/PMFConstants";
 import { readFileSync, writeFileSync } from "jsonfile";
-import { cmdToRun } from "../../../../src/plugins/utilities/NpmFunctions";
+import { findNpmOnPath } from "../../../../src/plugins/utilities/NpmFunctions";
 import { uninstall } from "../../../../src/plugins/utilities/npm-interface";
 
 
@@ -42,7 +42,7 @@ describe("PMF: Uninstall Interface", () => {
     const samplePackageName = "imperative-sample-plugin";
     const packageName = "a";
     const packageRegistry = "https://registry.npmjs.org/";
-    const npmCmd = cmdToRun();
+    const npmCmd = findNpmOnPath();
 
     beforeEach(() => {
         // Mocks need cleared after every test for clean test runs
@@ -101,7 +101,10 @@ describe("PMF: Uninstall Interface", () => {
 
     describe("Basic uninstall", () => {
         beforeEach(() => {
-            mocks.spawnSync.mockReturnValue(`+ ${packageName}`);
+            mocks.spawnSync.mockReturnValue({
+                status: 0,
+                stdout: Buffer.from(`+ ${packageName}`)
+            } as any);
         });
 
         it("should uninstall", () => {

packages/imperative/__tests__/plugins/utilities/runValidatePlugin.test.ts

@@ -25,7 +25,10 @@ const cmdOutputJson = {
     stderr: "The validate commands's standard error",
     data: {}
 };
-const spawnSyncOutput = { stdout: JSON.stringify(cmdOutputJson) };
+const spawnSyncOutput = {
+    status: 0,
+    stdout: JSON.stringify(cmdOutputJson)
+};
 
 describe("runValidatePlugin", () => {
     const mainModule = process.mainModule;

packages/imperative/src/config/cmd/report-env/EnvQuery.ts

@@ -190,7 +190,7 @@ export class EnvQuery {
                 }
             }
         } catch (err) {
-            cmdOutput = "Failed to run commmand = " + cmdToRun + " " + args.join(" ");
+            cmdOutput = "Failed to run command = " + cmdToRun + " " + args.join(" ");
             if (err.message) {
                 cmdOutput += `${os.EOL}Details = ` + err.message;
             }

packages/imperative/src/plugins/utilities/NpmFunctions.ts

@@ -12,21 +12,19 @@
 import { PMFConstants } from "./PMFConstants";
 import * as path from "path";
 import * as which from "which";
-import { spawnSync, StdioOptions } from "child_process";
+import { StdioOptions } from "child_process";
 import { readFileSync } from "jsonfile";
 import * as npmPackageArg from "npm-package-arg";
 import * as pacote from "pacote";
-import * as fs from "fs";
-import { ImperativeError } from "../../../../error/src/ImperativeError";
-import * as findUp from "find-up";
-const npmCmd = cmdToRun();
+import { ProcessUtils } from "../../../../utilities";
+const npmCmd = findNpmOnPath();
 
 /**
  * Common function that returns npm command as a string.
  *
  * @return {string} command with npm path
  */
-export function cmdToRun() {
+export function findNpmOnPath(): string {
     return which.sync("npm");
 }
 
@@ -43,23 +41,19 @@ export function cmdToRun() {
  */
 export function installPackages(prefix: string, registry: string, npmPackage: string): string {
     const pipe: StdioOptions = ["pipe", "pipe", process.stderr];
-    try {
-        const execOutput = spawnSync(npmCmd,
-            [
-                "install", npmPackage,
-                "--prefix", prefix,
-                "-g",
-                "--registry", registry,
-                "--legacy-peer-deps"
-            ], {
-                cwd: PMFConstants.instance.PMF_ROOT,
-                stdio: pipe
-            }
-        );
-        return execOutput.stdout.toString();
-    } catch (err) {
-        throw (err.message);
-    }
+    const execOutput = ProcessUtils.execAndCheckOutput(npmCmd,
+        [
+            "install", npmPackage,
+            "--prefix", prefix,
+            "-g",
+            "--registry", registry,
+            "--legacy-peer-deps"
+        ], {
+            cwd: PMFConstants.instance.PMF_ROOT,
+            stdio: pipe
+        }
+    );
+    return execOutput.toString();
 }
 
 /**
@@ -68,33 +62,25 @@ export function installPackages(prefix: string, registry: string, npmPackage: st
  * @return {string}
  */
 export function getRegistry(): string {
-    try {
-        const execOutput = spawnSync(npmCmd, [ "config", "get", "registry" ]);
-        return execOutput.stdout.toString();
-    } catch (err) {
-        throw(err.message);
-    }
+    const execOutput = ProcessUtils.execAndCheckOutput(npmCmd, [ "config", "get", "registry" ]);
+    return execOutput.toString();
 }
 
 /**
  * NPM login to be able to install from secure registry
  * @param {string} registry The npm registry to install from.
  */
 export function npmLogin(registry: string) {
-    try {
-        spawnSync(npmCmd,
-            [
-                "login",
-                "--registry", registry,
-                "--always-auth",
-                "--auth-type=legacy"
-            ], {
-                stdio: [0,1,2]
-            }
-        );
-    } catch (err) {
-        throw(err.message);
-    }
+    ProcessUtils.execAndCheckOutput(npmCmd,
+        [
+            "login",
+            "--registry", registry,
+            "--always-auth",
+            "--auth-type=legacy"
+        ], {
+            stdio: [0,1,2]
+        }
+    );
 }
 
 /**
@@ -111,17 +97,3 @@ export async function getPackageInfo(pkgSpec: string): Promise<{ name: string, v
         return pacote.manifest(pkgSpec);
     }
 }
-
-/**
- * Normalize the NPM path so that it works between older and newer versions of node
- *
- * @return {string} The NPM path
- */
-export function getNpmPath(): string {
-    let npmPath = require.resolve("npm");
-    if (npmPath.split(path.sep).includes("npm")) {
-        npmPath = findUp.sync("npm", {cwd: npmPath, type: "directory"});
-        if (npmPath != null && fs.existsSync(npmPath)) { return npmPath; }
-    }
-    throw new ImperativeError({msg: "Unable to resolve 'npm' path."});
-}