Skip to content

Commit

Permalink
Deprecate duplicate IO functions, prefer fs functions, and fix broken…
Browse files Browse the repository at this point in the history
… tests.

Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
  • Loading branch information
awharn committed Nov 5, 2024
1 parent 90924fa commit 9f8124c
Show file tree
Hide file tree
Showing 11 changed files with 23 additions and 139 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe("imperative-test-cli config convert-profiles", () => {
if (fs.existsSync(configJsonPath)) {
fs.unlinkSync(configJsonPath);
}
fsExtra.removeSync(TEST_ENVIRONMENT.workingDir + "/profiles-old");
fs.rmSync(TEST_ENVIRONMENT.workingDir + "/profiles-old", {recursive: true, force: true});
});

it("should display the help", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { ISetupEnvironmentParms } from "./doc/parms/ISetupEnvironmentParms";
import { ImperativeExpect } from "../../../src";
import * as nodePath from "path";
import { TEST_RESULT_DATA_DIR } from "../TestConstants";
import { mkdirpSync } from "fs-extra";
import { mkdirSync } from "fs";
import { ITestEnvironment } from "./doc/response/ITestEnvironment";
import { v4 as uuidv4 } from "uuid";

Expand Down Expand Up @@ -80,7 +80,7 @@ export class SetupTestEnvironment {
public static createUniqueTestDataDir(testName: string): string {
const app = uuidv4() + "_" + testName + "/";
const path = nodePath.resolve(TEST_RESULT_DATA_DIR + "/" + app);
mkdirpSync(path);
mkdirSync(path, {recursive: true});
return path;
}
}
3 changes: 1 addition & 2 deletions packages/imperative/__tests__/src/TestUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { ICommandResponse } from "../../src/cmd";
import { ICompareParms } from "./doc/ICompareParms";
import { TestLogger } from "./TestLogger";
import * as nodePath from "path";
import { mkdirpSync } from "fs-extra";
import * as fs from "fs";
import { randomBytes } from "crypto";
import * as os from "os";
Expand Down Expand Up @@ -410,7 +409,7 @@ export function compareJsonObjects(actual: any, expected: any, parms?: ICompareP
export function createUniqueTestDataDir(append = ""): string {
const app = uuidv4() + "/" + append + "/";
const path = nodePath.resolve(TEST_RESULT_DIR + "/data/" + app);
mkdirpSync(path);
fs.mkdirSync(path, {recursive: true});
return path;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
jest.mock("jsonfile");

import * as fs from "fs";
import * as fsExtra from "fs-extra";
import * as jsonfile from "jsonfile";
import { CredentialManagerFactory } from "../..";
import { ConvertV1Profiles } from "../";
Expand Down Expand Up @@ -834,7 +833,7 @@ describe("ConvertV1Profiles tests", () => {
beforeAll(() => {
ConvertV1Profiles["oldProfilesDir"] = oldProfileDir;
existsSyncSpy = jest.spyOn(fs, "existsSync");
removeSyncSpy = jest.spyOn(fsExtra, "removeSync");
removeSyncSpy = jest.spyOn(fs, "rmSync");
});

beforeEach(() => {
Expand Down Expand Up @@ -907,7 +906,7 @@ describe("ConvertV1Profiles tests", () => {
existsSyncSpy.mockReturnValue(true);

// pretend that remove crashed
const removeError = "fsExtra.removeSync threw a horrible error";
const removeError = "fs.rmSync threw a horrible error";
removeSyncSpy.mockImplementation(() => {
throw new Error(removeError);
});
Expand Down
3 changes: 1 addition & 2 deletions packages/imperative/src/config/src/ConvertV1Profiles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import * as fs from "fs";
import * as path from "path";
import { readFileSync } from "jsonfile";
import { removeSync } from "fs-extra";
import stripAnsi = require("strip-ansi");
import { V1ProfileRead, ProfilesConstants, ProfileUtils } from "../../profiles";
import { Config } from "./Config";
Expand Down Expand Up @@ -446,7 +445,7 @@ export class ConvertV1Profiles {
// Delete the profiles directory
try {
if (fs.existsSync(ConvertV1Profiles.oldProfilesDir)) {
removeSync(ConvertV1Profiles.oldProfilesDir);
fs.rmSync(ConvertV1Profiles.oldProfilesDir, {recursive: true});
ConvertV1Profiles.addToConvertMsgs(
ConvertMsgFmt.REPORT_LINE | ConvertMsgFmt.PARAGRAPH,
`Deleted the old profiles directory ${ConvertV1Profiles.oldProfilesDir}.`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe("Plugin Management Facility", () => {
existsSync: jest.spyOn(fs, "existsSync"),
writeFileSync: jest.spyOn(jsonfile, "writeFileSync"),
readFileSync: jest.spyOn(jsonfile, "readFileSync"),
mkdirp: jest.spyOn(IO, "mkdirp")
mkdirp: jest.spyOn(IO, "createDirSync")
};

/* Put a base CLI config into ImperativeConfig. It is required by infrastructure
Expand Down
101 changes: 2 additions & 99 deletions packages/imperative/src/io/__tests__/IO.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,17 +119,6 @@ describe("IO tests", () => {
expect(fnFm).toHaveBeenCalled();
});

it("should not create a dir if file exists", () => {
existsSyncSpy = jest.spyOn(fs, "existsSync").mockReturnValue(true);
const fnFm = jest.mocked(fs.mkdirSync);
fnFm.mockImplementation(((file: fs.PathLike) => {
return; // do nothing but pretend to write
}) as any);
IO.createDirSync("pretend/already/exists");
expect(existsSyncSpy).toHaveBeenCalled();
expect(fnFm).not.toHaveBeenCalled();
});

it("should get an error for no input on createDirsSync", () => {
let error;
try {
Expand All @@ -155,45 +144,7 @@ describe("IO tests", () => {
const dir = willBeADir.join(path.posix.sep);
// eslint-disable-next-line deprecation/deprecation
IO.createDirsSync(dir);
expect(fnFm).toHaveBeenCalledTimes(willBeADir.length);
});

it("should not create several dirs if dirs already exist", () => {
existsSyncSpy = jest.spyOn(fs, "existsSync").mockReturnValue(true);
const fnFm = jest.mocked(fs.mkdirSync);
fnFm.mockImplementation(((file: fs.PathLike) => {
return; // do nothing but pretend to write
}) as any);
const fnPr = jest.mocked(path.resolve);
fnPr.mockImplementation((...pathSegments: any[]) => {
return pathSegments[0];
});
const willBeADir = ["pretend", "to", "create"];
const dir = willBeADir.join(path.posix.sep);
// eslint-disable-next-line deprecation/deprecation
IO.createDirsSync(dir);
expect(fnFm).not.toHaveBeenCalled();
});

it("should only create dirs that do not exist", () => {
// pretend that only one of our directories exist
const willBeADir = ["pretend", "to", "create"];
existsSyncSpy = jest.spyOn(fs, "existsSync")
.mockReturnValue(false)
.mockReturnValueOnce(true);

const fnFm = jest.mocked(fs.mkdirSync);
fnFm.mockImplementation(((file: fs.PathLike) => {
return; // do nothing but pretend to write
}) as any);
const fnPr = jest.mocked(path.resolve);
fnPr.mockImplementation((...pathSegments: any[]) => {
return pathSegments[0];
});
const dir = willBeADir.join(path.posix.sep);
// eslint-disable-next-line deprecation/deprecation
IO.createDirsSync(dir);
expect(fnFm).toHaveBeenCalledTimes(willBeADir.length - 1);
expect(fnFm).toHaveBeenCalledTimes(1);
});

it("should create several dirs if dirs do not exist from input file", () => {
Expand All @@ -215,55 +166,7 @@ describe("IO tests", () => {
const willBeADir = ["pretend", "to", "create", "test.txt"];
const dir = willBeADir.join(path.posix.sep);
IO.createDirsSyncFromFilePath(dir);
expect(fnFm).toHaveBeenCalledTimes(willBeADir.length - 1);
});

it("should not create several dirs if dirs already exist from input file", () => {
existsSyncSpy = jest.spyOn(fs, "existsSync").mockReturnValue(true);
const fnFm = jest.mocked(fs.mkdirSync);
fnFm.mockImplementation(((file: fs.PathLike) => {
return; // do nothing but pretend to write
}) as any);
const fnPr = jest.mocked(path.resolve);
fnPr.mockImplementation((...pathSegments: any[]) => {
return pathSegments[0];
});
const fnPd = jest.mocked(path.dirname);
fnPd.mockImplementation(((...pathSegments: any[]) => {
const toDir: string[] = pathSegments[0].split(path.posix.sep);
toDir.pop();
return toDir.join(path.posix.sep);
}) as any);
const willBeADir = ["pretend", "to", "create", "test.txt"];
const dir = willBeADir.join(path.posix.sep);
IO.createDirsSyncFromFilePath(dir);
expect(fnFm).not.toHaveBeenCalled();
});

it("should only create dirs that do not exist from input file", () => {
// pretend that only one of our three directories exist
const willBeADir = ["pretend", "to", "create", "test.txt"];
existsSyncSpy = jest.spyOn(fs, "existsSync")
.mockReturnValue(false)
.mockReturnValueOnce(true);

const fnFm = jest.mocked(fs.mkdirSync);
fnFm.mockImplementation(((file: fs.PathLike) => {
return; // do nothing but pretend to write
}) as any);
const fnPr = jest.mocked(path.resolve);
fnPr.mockImplementation((...pathSegments: any[]) => {
return pathSegments[0];
});
const fnPd = jest.mocked(path.dirname);
fnPd.mockImplementation(((...pathSegments: any[]) => {
const toDir: string[] = pathSegments[0].split(path.posix.sep);
toDir.pop();
return toDir.join(path.posix.sep);
}) as any);
const dir = willBeADir.join(path.posix.sep);
IO.createDirsSyncFromFilePath(dir);
expect(fnFm).toHaveBeenCalledTimes(willBeADir.length - 2);
expect(fnFm).toHaveBeenCalledTimes(1);
});

it("processNewLines should replace LF line endings with CRLF on Windows", () => {
Expand Down
19 changes: 4 additions & 15 deletions packages/imperative/src/io/src/IO.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { ImperativeExpect } from "../../expect";
// use complete path to ExecUtils to avoid circular dependency that results from utilities/index
import { ExecUtils } from "../../utilities/src/ExecUtils";
import { Readable, Writable } from "stream";
import { mkdirpSync } from "fs-extra";

/**
* This class will handle common sequences of node I/O and issue messages /
Expand Down Expand Up @@ -109,7 +108,7 @@ export class IO {
}

/**
* Create a directory if it does not yet exist synchronously.
* Create a directory and all subdirectories if they do not yet exist synchronously.
* @static
* @param {string} dir - directory to create
* @return {undefined}
Expand All @@ -130,17 +129,7 @@ export class IO {
* @deprecated Please use IO.createDirSync
*/
public static createDirsSync(dir: string) {
ImperativeExpect.toBeDefinedAndNonBlank(dir, "dir");
// we're splitting on a specific separator character, so replace \ with /
// before splitting
const dirs = path.resolve(dir).replace(/\\/g, path.posix.sep).split(path.posix.sep);

let createDir: string = "";
for (const crDir of dirs) {

createDir += crDir + path.posix.sep;
IO.createDirSync(createDir);
}
this.createDirSync(dir);
}

/**
Expand Down Expand Up @@ -197,15 +186,15 @@ export class IO {
}

/**
* Uses the fs-extra package to create a directory (and all subdirectories)
* Create a directory and all subdirectories if they do not yet exist synchronously.
* @static
* @param {string} dir - the directory (do not include a file name)
* @memberof IO
* @deprecated Please use IO.createDirSync
*/
public static mkdirp(dir: string) {
ImperativeExpect.toBeDefinedAndNonBlank(dir, "dir");
mkdirpSync(dir);
this.createDirSync(dir);

Check warning on line 197 in packages/imperative/src/io/src/IO.ts

View check run for this annotation

Codecov / codecov/patch

packages/imperative/src/io/src/IO.ts#L197

Added line #L197 was not covered by tests
}

/**
Expand Down
7 changes: 1 addition & 6 deletions packages/imperative/src/operations/src/Operation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import { IOperationResult } from "./doc/IOperationResult";
import { TaskStage } from "./TaskStage";
import * as fs from "fs";
import { removeSync } from "fs-extra";
import { TextUtils } from "../../utilities";
import { ITaskWithStatus } from "./doc/ITaskWithStatus";
import { TaskProgress } from "./TaskProgress";
Expand Down Expand Up @@ -475,11 +474,7 @@ export abstract class Operation<T> implements ITaskWithStatus {
for (let x = 0; x < order.length; x++) {
this.log.info("Cleaning file: " + this.fileToUndo[x]);
try {
if (fs.statSync(order[x]).isDirectory()) {
removeSync(order[x]);
} else {
fs.unlinkSync(order[x]);
}
fs.rmSync(order[x]);

Check warning on line 477 in packages/imperative/src/operations/src/Operation.ts

View check run for this annotation

Codecov / codecov/patch

packages/imperative/src/operations/src/Operation.ts#L477

Added line #L477 was not covered by tests
} catch (error) {
this.log.error("An error occurred deleting: " + order[x]);
this.log.error("Message: " + error.message);
Expand Down
12 changes: 6 additions & 6 deletions packages/zosjobs/__tests__/__unit__/DeleteJobs.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe("Delete Jobs unit tests", () => {

describe("Positive tests", () => {
it("should allow users to call deleteJob with correct parameters", async () => {
ZosmfRestClient.deleteExpectJSON = jest.fn(returnDeleteJobsDataAsync);
ZosmfRestClient.deleteExpectJSON = jest.fn().mockReturnValue(CancelJobsData.SAMPLE_JOB_FEEDBACK_GOOD);
let caughtError;
let response;
try {
Expand All @@ -60,11 +60,11 @@ describe("Delete Jobs unit tests", () => {
caughtError = error;
}
expect(caughtError).toBeUndefined();
expect(response).toEqual(CancelJobsData.SAMPLE_JOB_FEEDBACK_ASYNC);
expect(response).toEqual(CancelJobsData.SAMPLE_JOB_FEEDBACK_GOOD);
});

it("should allow users to call deleteJobForJob with correct parameters", async () => {
ZosmfRestClient.deleteExpectJSON = jest.fn(returnDeleteJobsDataAsync);
ZosmfRestClient.deleteExpectJSON = jest.fn().mockReturnValue(CancelJobsData.SAMPLE_JOB_FEEDBACK_GOOD);
let caughtError;
let response;
try {
Expand All @@ -73,7 +73,7 @@ describe("Delete Jobs unit tests", () => {
caughtError = error;
}
expect(caughtError).toBeUndefined();
expect(response).toEqual(CancelJobsData.SAMPLE_JOB_FEEDBACK_ASYNC);
expect(response).toEqual(CancelJobsData.SAMPLE_JOB_FEEDBACK_GOOD);
});

it("should allow users to call deleteJobForJob with correct parameters (with modify version 1_0)", async () => {
Expand Down Expand Up @@ -104,7 +104,7 @@ describe("Delete Jobs unit tests", () => {
});

it("should allow users to call deleteJobCommon with correct parameters", async () => {
ZosmfRestClient.deleteExpectJSON = jest.fn(returnDeleteJobsDataAsync);
ZosmfRestClient.deleteExpectJSON = jest.fn().mockReturnValue(CancelJobsData.SAMPLE_JOB_FEEDBACK_GOOD);
let caughtError;
let response;
try {
Expand All @@ -113,7 +113,7 @@ describe("Delete Jobs unit tests", () => {
caughtError = error;
}
expect(caughtError).toBeUndefined();
expect(response).toEqual(CancelJobsData.SAMPLE_JOB_FEEDBACK_ASYNC);
expect(response).toEqual(CancelJobsData.SAMPLE_JOB_FEEDBACK_GOOD);
});

it("should allow users to call deleteJobCommon with correct parameters (with modify version 1_0)", async () => {
Expand Down
4 changes: 2 additions & 2 deletions scripts/bundleCliTgz.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@
*/

const childProcess = require("child_process");
const fs = require("fs-extra");
const fs = require("fs");
const path = require("path");

// Workaround for https://github.com/npm/cli/issues/3466
process.chdir(__dirname + "/..");
const cliPkgDir = path.join(process.cwd(), "packages", "cli");
const pkgJsonFile = path.join(cliPkgDir, "package.json");
const execCmd = (cmd) => childProcess.execSync(cmd, { cwd: cliPkgDir, stdio: "inherit" });
fs.mkdirpSync("dist");
fs.mkdirSync("dist", {recursive: true});
fs.renameSync(path.join(cliPkgDir, "node_modules"), path.join(cliPkgDir, "node_modules_old"));
fs.copyFileSync(pkgJsonFile, pkgJsonFile + ".bak");

Expand Down

0 comments on commit 9f8124c

Please sign in to comment.