diff --git a/CHANGELOG.md b/CHANGELOG.md index 12e79877b..fdb4cc567 100644 --- a/CHANGELOG.md +++ b/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` diff --git a/packages/imperative/__tests__/config/cmd/report-env/EnvQuery.unit.test.ts b/packages/imperative/__tests__/config/cmd/report-env/EnvQuery.unit.test.ts index 7685526a3..4822d3348 100644 --- a/packages/imperative/__tests__/config/cmd/report-env/EnvQuery.unit.test.ts +++ b/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 diff --git a/packages/imperative/__tests__/plugins/cmd/install/install.handler.test.ts b/packages/imperative/__tests__/plugins/cmd/install/install.handler.test.ts index 6c579b19b..8e8ae271a 100644 --- a/packages/imperative/__tests__/plugins/cmd/install/install.handler.test.ts +++ b/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"); + }); }); diff --git a/packages/imperative/__tests__/plugins/utilities/NpmFunctions.test.ts b/packages/imperative/__tests__/plugins/utilities/NpmFunctions.test.ts index 4571f838b..4029aa9fc 100644 --- a/packages/imperative/__tests__/plugins/utilities/NpmFunctions.test.ts +++ b/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,14 +75,6 @@ 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"); @@ -43,7 +82,7 @@ describe("NpmFunctions", () => { 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); }); diff --git a/packages/imperative/__tests__/plugins/utilities/npm-interface/uninstall.test.ts b/packages/imperative/__tests__/plugins/utilities/npm-interface/uninstall.test.ts index c048cb4e1..5ea9e800f 100644 --- a/packages/imperative/__tests__/plugins/utilities/npm-interface/uninstall.test.ts +++ b/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", () => { diff --git a/packages/imperative/__tests__/plugins/utilities/runValidatePlugin.test.ts b/packages/imperative/__tests__/plugins/utilities/runValidatePlugin.test.ts index 1edc9e597..2fe904c57 100644 --- a/packages/imperative/__tests__/plugins/utilities/runValidatePlugin.test.ts +++ b/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; diff --git a/packages/imperative/src/config/cmd/report-env/EnvQuery.ts b/packages/imperative/src/config/cmd/report-env/EnvQuery.ts index 56eaf8a2a..4e477e126 100644 --- a/packages/imperative/src/config/cmd/report-env/EnvQuery.ts +++ b/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; } diff --git a/packages/imperative/src/plugins/utilities/NpmFunctions.ts b/packages/imperative/src/plugins/utilities/NpmFunctions.ts index 01d5f4181..cd17eafa7 100644 --- a/packages/imperative/src/plugins/utilities/NpmFunctions.ts +++ b/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,12 +62,8 @@ 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(); } /** @@ -81,20 +71,16 @@ export function getRegistry(): string { * @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."}); -} \ No newline at end of file diff --git a/packages/imperative/src/plugins/utilities/npm-interface/uninstall.ts b/packages/imperative/src/plugins/utilities/npm-interface/uninstall.ts index 9902612e5..e5c773ff5 100644 --- a/packages/imperative/src/plugins/utilities/npm-interface/uninstall.ts +++ b/packages/imperative/src/plugins/utilities/npm-interface/uninstall.ts @@ -16,10 +16,10 @@ import { readFileSync, writeFileSync } from "jsonfile"; import { IPluginJson } from "../../doc/IPluginJson"; import { Logger } from "../../../../../logger"; import { ImperativeError } from "../../../../../error"; -import { TextUtils } from "../../../../../utilities"; -import { spawnSync, StdioOptions } from "child_process"; -import { cmdToRun } from "../NpmFunctions"; -const npmCmd = cmdToRun(); +import { ProcessUtils, TextUtils } from "../../../../../utilities"; +import { StdioOptions } from "child_process"; +import { findNpmOnPath } from "../NpmFunctions"; +const npmCmd = findNpmOnPath(); /** * @TODO - allow multiple packages to be uninstalled? @@ -64,7 +64,7 @@ export function uninstall(packageName: string): void { // formatting or colors but at least I can get the output of stdout right. (comment from install handler) iConsole.info("Uninstalling package...this may take some time."); - spawnSync(npmCmd, + ProcessUtils.execAndCheckOutput(npmCmd, [ "uninstall", npmPackage, diff --git a/packages/imperative/src/plugins/utilities/runValidatePlugin.ts b/packages/imperative/src/plugins/utilities/runValidatePlugin.ts index 9b8d97bb7..6f0fcd2be 100644 --- a/packages/imperative/src/plugins/utilities/runValidatePlugin.ts +++ b/packages/imperative/src/plugins/utilities/runValidatePlugin.ts @@ -9,8 +9,8 @@ * */ -import { spawnSync } from "child_process"; import { Logger } from "../../../../logger"; +import { ProcessUtils } from "../../../../utilities"; import { PMFConstants } from "./PMFConstants"; /** @@ -38,7 +38,7 @@ export function runValidatePlugin(pluginName: string): string { const impLogger = Logger.getImperativeLogger(); impLogger.debug(`Running plugin validation command = ${cmdToRun} plugins validate "${pluginName}" --response-format-json --no-fail-on-error`); - const valOutputJsonTxt = spawnSync(cmdToRun, + const valOutputJsonTxt = ProcessUtils.execAndCheckOutput(cmdToRun, [ ...cmdToRunArgs, "plugins", "validate", pluginName, @@ -47,7 +47,7 @@ export function runValidatePlugin(pluginName: string): string { ], { cwd: PMFConstants.instance.PMF_ROOT } - ).stdout.toString(); + ).toString(); // Debug trace information impLogger.trace(`Command Output: ${valOutputJsonTxt}`); diff --git a/packages/utilities/__tests__/ProcessUtils.test.ts b/packages/utilities/__tests__/ProcessUtils.test.ts index e2d467e28..da8f66d44 100644 --- a/packages/utilities/__tests__/ProcessUtils.test.ts +++ b/packages/utilities/__tests__/ProcessUtils.test.ts @@ -292,4 +292,56 @@ describe("ProcessUtils tests", () => { expect(childProcess.spawn).toHaveBeenCalledWith("vi", ["filePath"], { stdio: "inherit" }); }); }); + + describe("execAndCheckOutput", () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + it("returns stdout if command succeeds", () => { + const message = "Hello world!"; + const options: any = { cwd: __dirname }; + const stdoutBuffer = Buffer.from(message + "\n"); + jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({ + status: 0, + stdout: stdoutBuffer + } as any); + const execOutput = ProcessUtils.execAndCheckOutput("echo", [message], options); + expect(childProcess.spawnSync).toHaveBeenCalledWith("echo", [message], options); + expect(execOutput).toBe(stdoutBuffer); + }); + + it("throws error if command fails and returns error object", () => { + const filename = "invalid.txt"; + const errMsg = `cat: ${filename}: No such file or directory`; + jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({ + error: new Error(errMsg) + } as any); + let caughtError: any; + try { + ProcessUtils.execAndCheckOutput("cat", [filename]); + } catch (error) { + caughtError = error; + } + expect(childProcess.spawnSync).toHaveBeenCalledWith("cat", [filename], undefined); + expect(caughtError.message).toBe(errMsg); + }); + + it("throws error if command fails with non-zero status", () => { + const filename = "invalid.txt"; + const stderrBuffer = Buffer.from(`cat: ${filename}: No such file or directory\n`); + jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({ + status: 1, + stderr: stderrBuffer + } as any); + let caughtError: any; + try { + ProcessUtils.execAndCheckOutput("cat", [filename]); + } catch (error) { + caughtError = error; + } + expect(childProcess.spawnSync).toHaveBeenCalledWith("cat", [filename], undefined); + expect(caughtError.message).toBe(`Command failed: cat ${filename}\n${stderrBuffer.toString()}`); + }); + }); }); diff --git a/packages/utilities/src/ProcessUtils.ts b/packages/utilities/src/ProcessUtils.ts index 16a9f6437..13f9577ef 100644 --- a/packages/utilities/src/ProcessUtils.ts +++ b/packages/utilities/src/ProcessUtils.ts @@ -9,6 +9,7 @@ * */ +import { spawnSync, SpawnSyncOptions } from "child_process"; import { Logger } from "../../logger"; import { ImperativeConfig } from "./ImperativeConfig"; import { ISystemInfo } from "./doc/ISystemInfo"; @@ -142,4 +143,27 @@ export class ProcessUtils { await require("child_process").spawn(editor, [filePath], { stdio: "inherit" }); } } + + /** + * Spawn a process with arguments and throw an error if the process fails. + * Parameters are same as `child_process.spawnSync` (see Node.js docs). + * Use this method if you want the safe argument parsing of `spawnSync` + * combined with the smart output handling of `execSync`. + * @returns Contents of stdout as buffer or string + */ + public static execAndCheckOutput(command: string, args?: string[], options?: SpawnSyncOptions): Buffer | string { + // Implementation based on the child_process module + // https://github.com/nodejs/node/blob/main/lib/child_process.js + const result = spawnSync(command, args, options); + if (result.error != null) { + throw result.error; + } else if (result.status !== 0) { + let msg = `Command failed: ${command} ${args.join(" ")}`; + if (result.stderr?.length > 0) { + msg += `\n${result.stderr.toString()}`; + } + throw new Error(msg); + } + return result.stdout; + } }