[REFACTOR] commands/hld/init.ts and add tests (#197)

* replace any with interface

* correct the type definition for IVariableGroupDataVariable[]

* [REFACTOR] making code compact

* add test to create-variable-group command

* breaking larger function in scaffold class into smaller one, so we can add tests

* simplier commander interface

* using type guard in hasValue function

* incorporate Evan's feedback

* fix jsdoc

* fixed the problem where definition.yaml file is not created

* [REFACTOR] commands/hld/init.ts and add tests

1. move the command decoration out to a JSON file
2. standardized on command decorator function name, initCommandDecorator -> commandDecorator
3. added support for having default values in command builder
4. added tests to boost the coverage from 59% to 89%
5. create helper functions in tests so test code is more manage-able

* modified code to make sure that log is flushed before the command exit

* adding missing Promise<void> return type

Co-authored-by: Yvonne Radsmikham <yvonne.radsmikham@gmail.com>

Author: dennisseah

src/commands/hld/index.ts

@@ -1,5 +1,5 @@
 import { Command } from "../command";
-import { initCommandDecorator } from "./init";
+import { commandDecorator as initCommandDecorator } from "./init";
 import { installHldToManifestPipelineDecorator } from "./pipeline";
 import { reconcileHldDecorator } from "./reconcile";
 export const hldCommand = Command(

src/commands/hld/init.decorator.json

@@ -0,0 +1,13 @@
+{
+  "command": "init",
+  "alias": "i",
+  "description": "Initialize High Level Definition repository. Add manifest-generation.yaml file to working directory/repository if it does not already exist.",
+  "options": [
+    {
+      "arg": "--git-push",
+      "description": "SPK CLI will try to commit and push these changes to a new origin/branch.",
+      "required": false,
+      "defaultValue": false
+    }
+  ]
+}

src/commands/hld/init.test.ts

@@ -8,7 +8,7 @@ import {
   enableVerboseLogging,
   logger
 } from "../../logger";
-import { initialize } from "./init";
+import { execute, initialize } from "./init";
 jest.mock("../../lib/gitutils");
 
 beforeAll(() => {
@@ -19,53 +19,54 @@ afterAll(() => {
   disableVerboseLogging();
 });
 
-describe("Initializing an HLD Repository.", () => {
-  test("New directory is created under root directory with required service files.", async () => {
-    // Create random directory to initialize
-    const randomTmpDir = path.join(os.tmpdir(), uuid());
-    fs.mkdirSync(randomTmpDir);
-
-    logger.info(`creating randomTmpDir ${randomTmpDir}`);
-
-    // addService call
-    await initialize(randomTmpDir, false);
-
-    // Check temp test directory exists
-    expect(fs.existsSync(randomTmpDir)).toBe(true);
-
-    // Verify new azure-pipelines created
-    const filepaths = ["manifest-generation.yaml", "component.yaml"].map(
-      filename => path.join(randomTmpDir, filename)
-    );
-
-    for (const filepath of filepaths) {
-      expect(fs.existsSync(filepath)).toBe(true);
-    }
+const testExecuteFn = async (
+  projectRootPath: string,
+  gitPush: boolean,
+  expectedExitCode: number
+) => {
+  const exitFn = jest.fn();
+  await execute(projectRootPath, gitPush, exitFn);
+  expect(exitFn).toBeCalledTimes(1);
+  expect(exitFn.mock.calls).toEqual([[expectedExitCode]]);
+};
+
+describe("Test execute function", () => {
+  it("simulate an error: missing project path", async () => {
+    await testExecuteFn("", false, 1);
   });
+  it("positive test", async () => {
+    await testExecuteFn(os.tmpdir(), false, 0);
+  });
+});
 
-  test("New directory is created and git push is enabled.", async () => {
-    // Create random directory to initialize
-
-    const randomTmpDir = path.join(os.tmpdir(), uuid());
-    fs.mkdirSync(randomTmpDir);
+const testRepoInitialization = async (gitPush: boolean) => {
+  // Create random directory to initialize
+  const randomTmpDir = path.join(os.tmpdir(), uuid());
+  fs.mkdirSync(randomTmpDir);
 
-    logger.info(`creating randomTmpDir ${randomTmpDir}`);
+  logger.info(`creating randomTmpDir ${randomTmpDir}`);
 
-    // addService call
-    await initialize(randomTmpDir, true);
+  // addService call
+  await initialize(randomTmpDir, gitPush);
 
-    // Check temp test directory exists
-    expect(fs.existsSync(randomTmpDir)).toBe(true);
+  // Check temp test directory exists
+  expect(fs.existsSync(randomTmpDir)).toBe(true);
 
-    // Verify new azure-pipelines created
-    const filepaths = ["manifest-generation.yaml", "component.yaml"].map(
-      filename => path.join(randomTmpDir, filename)
-    );
+  // Verify new azure-pipelines created
+  ["manifest-generation.yaml", "component.yaml"]
+    .map(filename => path.join(randomTmpDir, filename))
+    .forEach(filePath => {
+      expect(fs.existsSync(filePath)).toBe(true);
+    });
+};
 
-    for (const filepath of filepaths) {
-      expect(fs.existsSync(filepath)).toBe(true);
-    }
+describe("Initializing an HLD Repository.", () => {
+  test("New directory is created under root directory with required service files.", async () => {
+    await testRepoInitialization(false);
+  });
 
+  test("New directory is created and git push is enabled.", async () => {
+    await testRepoInitialization(true);
     expect(checkoutCommitPushCreatePRLink).toHaveBeenCalled();
   });
 });

src/commands/hld/init.ts

@@ -1,48 +1,51 @@
 import commander from "commander";
 
+import { build as buildCmd, exit as exitCmd } from "../../lib/commandBuilder";
 import {
   generateDefaultHldComponentYaml,
   generateGitIgnoreFile,
   generateHldAzurePipelinesYaml
 } from "../../lib/fileutils";
 import { checkoutCommitPushCreatePRLink } from "../../lib/gitutils";
+import { hasValue } from "../../lib/validator";
 import { logger } from "../../logger";
+import decorator from "./init.decorator.json";
 
-/**
- * Adds the init command to the hld command object
- *
- * @param command Commander command object to decorate
- */
-export const initCommandDecorator = (command: commander.Command): void => {
-  command
-    .command("init")
-    .alias("i")
-    .description(
-      "Initialize your hld repository. Will add the manifest-generation.yaml file to your working directory/repository if it does not already exist."
-    )
-    .option(
-      "--git-push",
-      "SPK CLI will try to commit and push these changes to a new origin/branch.",
-      false
-    )
-    .action(async opts => {
-      const { gitPush = false } = opts;
-      const projectPath = process.cwd();
-      try {
-        // Type check all parsed command line args here.
-        if (typeof gitPush !== "boolean") {
-          throw new Error(
-            `gitPush must be of type boolean, ${typeof gitPush} given.`
-          );
-        }
-        await initialize(projectPath, gitPush);
-      } catch (err) {
-        logger.error(
-          `Error occurred while initializing hld repository ${projectPath}`
-        );
-        logger.error(err);
-      }
+// values that we need to pull out from command operator
+interface ICommandOptions {
+  gitPush: boolean;
+}
+
+export const execute = async (
+  projectPath: string,
+  gitPush: boolean,
+  exitFn: (status: number) => Promise<void>
+) => {
+  try {
+    if (!hasValue(projectPath)) {
+      throw new Error("project path is not provided");
+    }
+    await initialize(projectPath, gitPush);
+    await exitFn(0);
+  } catch (err) {
+    logger.error(
+      `Error occurred while initializing hld repository ${projectPath}`
+    );
+    logger.error(err);
+    await exitFn(1);
+  }
+};
+
+export const commandDecorator = (command: commander.Command): void => {
+  buildCmd(command, decorator).action(async (opts: ICommandOptions) => {
+    const projectPath = process.cwd();
+    // gitPush will is always true or false. It shall not be
+    // undefined because default value is set in the commander decorator
+    await execute(projectPath, opts.gitPush, async (status: number) => {
+      await exitCmd(logger);
+      process.exit(status);
     });
+  });
 };
 
 export const initialize = async (rootProjectPath: string, gitPush: boolean) => {

src/commands/project/create-variable-group.ts

@@ -6,6 +6,7 @@ import { echo } from "shelljs";
 import { Bedrock, Config, readYaml, write } from "../../config";
 import {
   build as buildCmd,
+  exit as exitCmd,
   validateForRequiredValues
 } from "../../lib/commandBuilder";
 import { IAzureDevOpsOpts } from "../../lib/git";
@@ -72,7 +73,7 @@ export const validateRequiredArguments = (
 export const execute = async (
   variableGroupName: string,
   opts: ICommandOptions,
-  exitFn: (status: number) => void
+  exitFn: (status: number) => Promise<void>
 ) => {
   if (!hasValue(variableGroupName)) {
     exitFn(1);
@@ -109,7 +110,7 @@ export const execute = async (
       );
 
       if (errors.length !== 0) {
-        exitFn(1);
+        await exitFn(1);
       } else {
         const variableGroup = await create(
           variableGroupName,
@@ -134,12 +135,12 @@ export const execute = async (
         logger.info(
           "Successfully created a variable group in Azure DevOps project!"
         );
-        exitFn(0);
+        await exitFn(0);
       }
     } catch (err) {
       logger.error(`Error occurred while creating variable group`);
       logger.error(err);
-      exitFn(1);
+      await exitFn(1);
     }
   }
 };
@@ -152,7 +153,10 @@ export const execute = async (
 export const commandDecorator = (command: commander.Command): void => {
   buildCmd(command, decorator).action(
     async (variableGroupName: string, opts: ICommandOptions) => {
-      await execute(variableGroupName, opts, process.exit);
+      await execute(variableGroupName, opts, async (status: number) => {
+        await exitCmd(logger);
+        process.exit(status);
+      });
     }
   );
 };

src/lib/commandBuilder.test.ts

@@ -1,13 +1,16 @@
 import commander from "commander";
+import { logger } from "../logger";
 import {
   build,
+  exit as exitCmd,
   ICommandBuildElements,
   validateForRequiredValues
 } from "./commandBuilder";
 
 interface ICommandOption {
   flags: string;
   description: string;
+  defaultValue: string | boolean;
 }
 
 describe("Tests Command Builder's build function", () => {
@@ -31,6 +34,18 @@ describe("Tests Command Builder's build function", () => {
           arg: "-c, --option-c <optionC>",
           description: "description for optionC",
           required: false
+        },
+        {
+          arg: "-d, --option-d <optionD>",
+          defaultValue: false,
+          description: "description for optionD",
+          required: false
+        },
+        {
+          arg: "-e, --option-d <optionE>",
+          defaultValue: "test",
+          description: "description for optionE",
+          required: false
         }
       ]
     };
@@ -39,9 +54,15 @@ describe("Tests Command Builder's build function", () => {
 
     expect(cmd.description()).toBe("description of command");
     expect(cmd.alias()).toBe("cbt");
+
     cmd.options.forEach((opt: ICommandOption, i: number) => {
-      expect(opt.flags).toBe(descriptor.options[i].arg);
-      expect(opt.description).toBe(descriptor.options[i].description);
+      const declared = descriptor.options[i];
+      expect(opt.flags).toBe(declared.arg);
+      expect(opt.description).toBe(declared.description);
+
+      if (declared.defaultValue !== undefined) {
+        expect(opt.defaultValue).toBe(declared.defaultValue);
+      }
     });
   });
 });
@@ -82,3 +103,12 @@ describe("Tests Command Builder's validation function", () => {
     expect(errors[0]).toBe("-c --option-c <optionC>");
   });
 });
+
+describe("Tests Command Builder's exit function", () => {
+  it("calling exit function", () => {
+    jest.spyOn(logger, "info");
+    exitCmd(logger).then(() => {
+      expect(logger.info).toBeCalledTimes(1);
+    });
+  });
+});

src/lib/commandBuilder.ts

@@ -1,4 +1,5 @@
 import commander from "commander";
+import { Logger } from "winston";
 import { logger } from "../logger";
 import { hasValue } from "./validator";
 
@@ -9,6 +10,7 @@ export interface ICommandOption {
   arg: string;
   description: string;
   required: boolean;
+  defaultValue?: string | boolean;
 }
 
 /**
@@ -43,7 +45,11 @@ export const build = (
     .description(decorator.description);
 
   decorator.options.forEach(opt => {
-    cmd.option(opt.arg, opt.description);
+    if (opt.defaultValue !== undefined) {
+      cmd.option(opt.arg, opt.description, opt.defaultValue);
+    } else {
+      cmd.option(opt.arg, opt.description);
+    }
   });
 
   return cmd;
@@ -96,3 +102,17 @@ export const validateForRequiredValues = (
   }
   return errors;
 };
+
+/**
+ * Flushs the log, ready to exit the command.
+ * In future there may be other housekeeper tasks.
+ *
+ * @param log Logger instance
+ */
+export const exit = (log: Logger): Promise<void> => {
+  return new Promise(resolve => {
+    log.info("", null, () => {
+      resolve();
+    });
+  });
+};