From ab769531fd163d0b5a820dbb61ddd589f8f527c6 Mon Sep 17 00:00:00 2001 From: Timothy Johnson Date: Mon, 13 Mar 2023 14:47:34 -0400 Subject: [PATCH 1/4] Replace spawnSync with wrapper method that handles errors Signed-off-by: Timothy Johnson --- .../src/config/cmd/report-env/EnvQuery.ts | 2 +- .../src/plugins/utilities/NpmFunctions.ts | 34 +++++-------------- .../utilities/npm-interface/uninstall.ts | 10 +++--- .../plugins/utilities/runValidatePlugin.ts | 6 ++-- packages/utilities/src/ProcessUtils.ts | 27 +++++++++++++++ 5 files changed, 45 insertions(+), 34 deletions(-) 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..7f7ecc6ba 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"); } @@ -44,7 +42,7 @@ 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, + const execOutput = ProcessUtils.execAndCheckOutput(npmCmd, [ "install", npmPackage, "--prefix", prefix, @@ -56,7 +54,7 @@ export function installPackages(prefix: string, registry: string, npmPackage: st stdio: pipe } ); - return execOutput.stdout.toString(); + return execOutput.toString(); } catch (err) { throw (err.message); } @@ -69,8 +67,8 @@ export function installPackages(prefix: string, registry: string, npmPackage: st */ export function getRegistry(): string { try { - const execOutput = spawnSync(npmCmd, [ "config", "get", "registry" ]); - return execOutput.stdout.toString(); + const execOutput = ProcessUtils.execAndCheckOutput(npmCmd, [ "config", "get", "registry" ]); + return execOutput.toString(); } catch (err) { throw(err.message); } @@ -82,7 +80,7 @@ export function getRegistry(): string { */ export function npmLogin(registry: string) { try { - spawnSync(npmCmd, + ProcessUtils.execAndCheckOutput(npmCmd, [ "login", "--registry", registry, @@ -111,17 +109,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/src/ProcessUtils.ts b/packages/utilities/src/ProcessUtils.ts index 16a9f6437..90d8b83e4 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,30 @@ 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 (options?.stdio == null && result.stderr != null) { + process.stderr.write(result.stderr); + } + 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; + } } From 90d359df13dd2682bb3df1d2a1ac5e31ba6d4267 Mon Sep 17 00:00:00 2001 From: Timothy Johnson Date: Wed, 15 Mar 2023 01:00:50 -0400 Subject: [PATCH 2/4] Update unit tests for plugin install failure Signed-off-by: Timothy Johnson --- CHANGELOG.md | 7 +- .../cmd/report-env/EnvQuery.unit.test.ts | 2 +- .../cmd/install/install.handler.test.ts | 24 ++++++ .../utilities/npm-interface/uninstall.test.ts | 9 ++- .../utilities/runValidatePlugin.test.ts | 5 +- .../src/plugins/utilities/NpmFunctions.ts | 62 +++++++--------- .../utilities/__tests__/ProcessUtils.test.ts | 73 ++++++++++++++++++- packages/utilities/src/ProcessUtils.ts | 10 ++- 8 files changed, 146 insertions(+), 46 deletions(-) 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..b528a84c2 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; @@ -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/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/plugins/utilities/NpmFunctions.ts b/packages/imperative/src/plugins/utilities/NpmFunctions.ts index 7f7ecc6ba..cd17eafa7 100644 --- a/packages/imperative/src/plugins/utilities/NpmFunctions.ts +++ b/packages/imperative/src/plugins/utilities/NpmFunctions.ts @@ -41,23 +41,19 @@ export function findNpmOnPath(): string { */ export function installPackages(prefix: string, registry: string, npmPackage: string): string { const pipe: StdioOptions = ["pipe", "pipe", process.stderr]; - try { - 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(); - } 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(); } /** @@ -66,12 +62,8 @@ export function installPackages(prefix: string, registry: string, npmPackage: st * @return {string} */ export function getRegistry(): string { - try { - const execOutput = ProcessUtils.execAndCheckOutput(npmCmd, [ "config", "get", "registry" ]); - return execOutput.toString(); - } catch (err) { - throw(err.message); - } + const execOutput = ProcessUtils.execAndCheckOutput(npmCmd, [ "config", "get", "registry" ]); + return execOutput.toString(); } /** @@ -79,20 +71,16 @@ export function getRegistry(): string { * @param {string} registry The npm registry to install from. */ export function npmLogin(registry: string) { - try { - ProcessUtils.execAndCheckOutput(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] + } + ); } /** diff --git a/packages/utilities/__tests__/ProcessUtils.test.ts b/packages/utilities/__tests__/ProcessUtils.test.ts index e2d467e28..4bbc0d51e 100644 --- a/packages/utilities/__tests__/ProcessUtils.test.ts +++ b/packages/utilities/__tests__/ProcessUtils.test.ts @@ -10,7 +10,7 @@ */ import * as childProcess from "child_process"; -import { GuiResult, ImperativeConfig, ProcessUtils } from "../../utilities"; +import { DaemonRequest, GuiResult, ImperativeConfig, ProcessUtils } from "../../utilities"; jest.mock("child_process"); jest.mock("opener"); @@ -292,4 +292,75 @@ 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()}`); + }); + + it("logs stderr output to daemon stream", () => { + const stderrBuffer = Buffer.from("Task failed successfully\n"); + const streamWriteMock = jest.fn(); + jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({ + status: 0, + stderr: stderrBuffer + } as any); + jest.spyOn(ImperativeConfig, "instance", "get").mockReturnValueOnce({ + daemonContext: { + stream: { write: streamWriteMock } + } + } as any); + ProcessUtils.execAndCheckOutput("fail"); + expect(childProcess.spawnSync).toHaveBeenCalledWith("fail", undefined, undefined); + expect(streamWriteMock).toHaveBeenCalledWith(JSON.stringify({ + stderr: stderrBuffer.toString() + }) + DaemonRequest.EOW_DELIMITER); + }); + }); }); diff --git a/packages/utilities/src/ProcessUtils.ts b/packages/utilities/src/ProcessUtils.ts index 90d8b83e4..f6252b488 100644 --- a/packages/utilities/src/ProcessUtils.ts +++ b/packages/utilities/src/ProcessUtils.ts @@ -13,6 +13,7 @@ import { spawnSync, SpawnSyncOptions } from "child_process"; import { Logger } from "../../logger"; import { ImperativeConfig } from "./ImperativeConfig"; import { ISystemInfo } from "./doc/ISystemInfo"; +import { DaemonRequest } from "./DaemonRequest"; /** * This enum represents the possible results from isGuiAvailable. @@ -155,8 +156,13 @@ export class ProcessUtils { // 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 (options?.stdio == null && result.stderr != null) { - process.stderr.write(result.stderr); + if (options?.stdio == null && result.stderr?.length > 0) { + const daemonStream = ImperativeConfig.instance.daemonContext?.stream; + if (daemonStream != null) { + daemonStream.write(DaemonRequest.create({ stderr: result.stderr.toString() })); + } else { + process.stderr.write(result.stderr); + } } if (result.error != null) { throw result.error; From 9abfba4df1436f4ba495166edf9482f1c32fb421 Mon Sep 17 00:00:00 2001 From: Timothy Johnson Date: Wed, 15 Mar 2023 08:21:26 -0400 Subject: [PATCH 3/4] Remove unnecessary logging of stderr Signed-off-by: Timothy Johnson --- .../utilities/__tests__/ProcessUtils.test.ts | 21 +------------------ packages/utilities/src/ProcessUtils.ts | 9 -------- 2 files changed, 1 insertion(+), 29 deletions(-) diff --git a/packages/utilities/__tests__/ProcessUtils.test.ts b/packages/utilities/__tests__/ProcessUtils.test.ts index 4bbc0d51e..da8f66d44 100644 --- a/packages/utilities/__tests__/ProcessUtils.test.ts +++ b/packages/utilities/__tests__/ProcessUtils.test.ts @@ -10,7 +10,7 @@ */ import * as childProcess from "child_process"; -import { DaemonRequest, GuiResult, ImperativeConfig, ProcessUtils } from "../../utilities"; +import { GuiResult, ImperativeConfig, ProcessUtils } from "../../utilities"; jest.mock("child_process"); jest.mock("opener"); @@ -343,24 +343,5 @@ describe("ProcessUtils tests", () => { expect(childProcess.spawnSync).toHaveBeenCalledWith("cat", [filename], undefined); expect(caughtError.message).toBe(`Command failed: cat ${filename}\n${stderrBuffer.toString()}`); }); - - it("logs stderr output to daemon stream", () => { - const stderrBuffer = Buffer.from("Task failed successfully\n"); - const streamWriteMock = jest.fn(); - jest.spyOn(childProcess, "spawnSync").mockReturnValueOnce({ - status: 0, - stderr: stderrBuffer - } as any); - jest.spyOn(ImperativeConfig, "instance", "get").mockReturnValueOnce({ - daemonContext: { - stream: { write: streamWriteMock } - } - } as any); - ProcessUtils.execAndCheckOutput("fail"); - expect(childProcess.spawnSync).toHaveBeenCalledWith("fail", undefined, undefined); - expect(streamWriteMock).toHaveBeenCalledWith(JSON.stringify({ - stderr: stderrBuffer.toString() - }) + DaemonRequest.EOW_DELIMITER); - }); }); }); diff --git a/packages/utilities/src/ProcessUtils.ts b/packages/utilities/src/ProcessUtils.ts index f6252b488..13f9577ef 100644 --- a/packages/utilities/src/ProcessUtils.ts +++ b/packages/utilities/src/ProcessUtils.ts @@ -13,7 +13,6 @@ import { spawnSync, SpawnSyncOptions } from "child_process"; import { Logger } from "../../logger"; import { ImperativeConfig } from "./ImperativeConfig"; import { ISystemInfo } from "./doc/ISystemInfo"; -import { DaemonRequest } from "./DaemonRequest"; /** * This enum represents the possible results from isGuiAvailable. @@ -156,14 +155,6 @@ export class ProcessUtils { // 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 (options?.stdio == null && result.stderr?.length > 0) { - const daemonStream = ImperativeConfig.instance.daemonContext?.stream; - if (daemonStream != null) { - daemonStream.write(DaemonRequest.create({ stderr: result.stderr.toString() })); - } else { - process.stderr.write(result.stderr); - } - } if (result.error != null) { throw result.error; } else if (result.status !== 0) { From cd0bc886cd77eb0d252b23e0feb30aa9f5ebe60c Mon Sep 17 00:00:00 2001 From: Timothy Johnson Date: Wed, 15 Mar 2023 10:12:03 -0400 Subject: [PATCH 4/4] Add tests to increase NpmFunctions coverage Signed-off-by: Timothy Johnson --- .../cmd/install/install.handler.test.ts | 4 +- .../plugins/utilities/NpmFunctions.test.ts | 67 +++++++++++++++---- 2 files changed, 55 insertions(+), 16 deletions(-) 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 b528a84c2..8e8ae271a 100644 --- a/packages/imperative/__tests__/plugins/cmd/install/install.handler.test.ts +++ b/packages/imperative/__tests__/plugins/cmd/install/install.handler.test.ts @@ -73,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"; @@ -277,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); 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); });