cli: improve test coverage and prefer logger over console

Author: grypez

packages/cli/src/app.ts

@@ -1,14 +1,17 @@
+import { Logger } from '@ocap/utils';
 import path from 'node:path';
 import yargs from 'yargs';
 import { hideBin } from 'yargs/helpers';
 
-import { createBundle } from './commands/bundle.ts';
+import { bundleSource } from './commands/bundle.ts';
 import { getServer } from './commands/serve.ts';
 import { watchDir } from './commands/watch.ts';
 import { defaultConfig } from './config.ts';
 import type { Config } from './config.ts';
 import { withTimeout } from './utils.ts';
 
+const logger = new Logger('cli');
+
 await yargs(hideBin(process.argv))
   .usage('$0 <command> [options]')
   .demandCommand(1)
@@ -26,7 +29,9 @@ await yargs(hideBin(process.argv))
         describe: 'The files or directories of files to bundle',
       }),
     async (args) => {
-      await Promise.all(args.targets.map(createBundle));
+      await Promise.all(
+        args.targets.map(async (target) => bundleSource(logger, target)),
+      );
     },
   )
   .command(
@@ -55,7 +60,7 @@ await yargs(hideBin(process.argv))
         },
         dir: resolvedDir,
       };
-      console.info(`starting ${appName} in ${resolvedDir} on ${url}`);
+      logger.info(`starting ${appName} in ${resolvedDir} on ${url}`);
       const server = getServer(config);
       await server.listen();
     },
@@ -71,19 +76,19 @@ await yargs(hideBin(process.argv))
         describe: 'The directory to watch',
       }),
     (args) => {
-      const { ready, error } = watchDir(args.dir);
+      const { ready, error } = watchDir(args.dir, logger);
       let handleClose: undefined | (() => Promise<void>);
 
       ready
         .then((close) => {
           handleClose = close;
-          console.info(`Watching ${args.dir}...`);
+          logger.info(`Watching ${args.dir}...`);
           return undefined;
         })
-        .catch(console.error);
+        .catch(logger.error);
 
       error.catch(async (reason) => {
-        console.error(reason);
+        logger.error(reason);
         // If watching started, close the watcher.
         return handleClose ? withTimeout(handleClose(), 400) : undefined;
       });
@@ -109,18 +114,21 @@ await yargs(hideBin(process.argv))
       const closeHandlers: (() => Promise<void>)[] = [];
       const resolvedDir = path.resolve(args.dir);
 
-      await createBundle(resolvedDir);
+      await bundleSource(logger, resolvedDir);
 
       const handleClose = async (): Promise<void> => {
         await Promise.all(
           closeHandlers.map(async (close) => withTimeout(close(), 400)),
         );
       };
 
-      const { ready: watchReady, error: watchError } = watchDir(resolvedDir);
+      const { ready: watchReady, error: watchError } = watchDir(
+        resolvedDir,
+        logger,
+      );
 
       watchError.catch(async (reason) => {
-        console.error(reason);
+        logger.error(reason);
         await handleClose();
       });
 
@@ -136,7 +144,7 @@ await yargs(hideBin(process.argv))
       const { close: closeServer, port } = await server.listen();
       closeHandlers.push(closeServer);
 
-      console.info(`bundling and serving ${resolvedDir} on localhost:${port}`);
+      logger.info(`bundling and serving ${resolvedDir} on localhost:${port}`);
     },
   )
   .help('help')

packages/cli/src/commands/bundle.test.ts

@@ -1,25 +1,45 @@
+import { Logger } from '@ocap/utils';
 import { readFile, rm } from 'fs/promises';
 import { basename } from 'path';
 import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest';
 
-import { createBundleFile, createBundleDir } from './bundle.ts';
+import { bundleFile, bundleDir, bundleSource } from './bundle.ts';
 import {
   makeTestBundleStage,
   validTestBundleNames,
 } from '../../test/bundles.ts';
 import { fileExists } from '../file.ts';
 
-const mocks = vi.hoisted(() => ({
-  bundleSource: vi.fn(),
-}));
+const mocks = vi.hoisted(() => {
+  return {
+    endoBundleSource: vi.fn(),
+    Logger: vi.fn(() => ({
+      info: vi.fn(),
+      error: vi.fn(),
+      subLogger: vi.fn(),
+    })),
+    isDirectory: vi.fn(),
+  };
+});
 
 vi.mock('@endo/bundle-source', () => ({
-  default: mocks.bundleSource,
+  default: mocks.endoBundleSource,
 }));
 
 vi.mock('@endo/init', () => ({}));
 
+vi.mock('@ocap/utils', () => ({
+  Logger: mocks.Logger,
+}));
+
+vi.mock('../file.ts', async (importOriginal) => ({
+  ...(await importOriginal()),
+  isDirectory: mocks.isDirectory,
+}));
+
 describe('bundle', async () => {
+  let logger: Logger;
+
   const { testBundleRoot, getTestBundleSpecs, globBundles, resolveBundlePath } =
     await makeTestBundleStage();
   const testBundleSpecs = getTestBundleSpecs(validTestBundleNames);
@@ -32,20 +52,22 @@ describe('bundle', async () => {
   afterAll(deleteTestBundles);
 
   beforeEach(async () => {
-    vi.resetModules();
     await deleteTestBundles();
+    vi.resetModules();
+    logger = new Logger();
+    vi.resetAllMocks();
   });
 
-  describe('createBundleFile', () => {
+  describe('bundleFile', () => {
     it.each(testBundleSpecs)(
       'bundles a single file: $name',
       async ({ source, bundle }) => {
         expect(await fileExists(bundle)).toBe(false);
 
         const testContent = { source: 'test-content' };
-        mocks.bundleSource.mockImplementationOnce(() => testContent);
+        mocks.endoBundleSource.mockImplementationOnce(() => testContent);
 
-        await createBundleFile(source);
+        await bundleFile(logger, source);
 
         expect(await fileExists(bundle)).toBe(true);
 
@@ -57,27 +79,27 @@ describe('bundle', async () => {
       },
     );
 
-    it('calls console.error if bundling fails', async () => {
-      const consoleErrorSpy = vi.spyOn(console, 'error');
+    it('calls logger.error if bundling fails', async () => {
+      const loggerErrorSpy = vi.spyOn(logger, 'error');
       const badBundle = resolveBundlePath('bad-vat.fails');
-      await createBundleFile(badBundle);
-      expect(consoleErrorSpy).toHaveBeenCalledOnce();
+      await bundleFile(logger, badBundle);
+      expect(loggerErrorSpy).toHaveBeenCalledOnce();
     });
   });
 
-  describe('createBundleDir', () => {
+  describe('bundleDir', () => {
     it('bundles a directory', async () => {
       expect(await globBundles()).toStrictEqual([]);
 
       // mocked bundleSource fails iff the target filename has '.fails.'
-      mocks.bundleSource.mockImplementation((bundlePath) => {
+      mocks.endoBundleSource.mockImplementation((bundlePath) => {
         if (bundlePath.includes('.fails.')) {
           throw new Error(`Failed to bundle ${bundlePath}`);
         }
         return 'test content';
       });
 
-      await createBundleDir(testBundleRoot);
+      await bundleDir(logger, testBundleRoot);
 
       const bundledOutputs = (await globBundles()).map((bundlePath) =>
         basename(bundlePath, '.bundle'),
@@ -88,4 +110,18 @@ describe('bundle', async () => {
       expect(bundledOutputs).toStrictEqual(validTestBundleNames);
     });
   });
+
+  describe('bundleSource', () => {
+    it('calls logger.error if bundling fails', async () => {
+      mocks.isDirectory.mockImplementationOnce(() => {
+        throw new Error('test error');
+      });
+      const loggerErrorSpy = vi.spyOn(logger, 'error');
+      await bundleSource(logger, resolveBundlePath('test'));
+      expect(loggerErrorSpy).toHaveBeenCalledWith(
+        expect.stringContaining('error bundling target'),
+        expect.any(Error),
+      );
+    });
+  });
 });

packages/cli/src/commands/bundle.ts

@@ -1,5 +1,6 @@
 import '@endo/init';
-import bundleSource from '@endo/bundle-source';
+import endoBundleSource from '@endo/bundle-source';
+import { Logger } from '@ocap/utils';
 import { glob } from 'glob';
 import { writeFile } from 'node:fs/promises';
 import { resolve, join } from 'node:path';
@@ -10,50 +11,63 @@ import { resolveBundlePath } from '../path.ts';
 /**
  * Create a bundle given path to an entry point.
  *
+ * @param logger - The logger to use for logging.
  * @param sourcePath - Path to the source file that is the root of the bundle.
- * @param destinationPath - Optional path to which to write the bundle.
+ * @param targetPath - Optional path to which to write the bundle.
  *  If not provided, defaults to sourcePath with `.bundle` extension.
  * @returns A promise that resolves when the bundle has been written.
  */
-export async function createBundleFile(
+export async function bundleFile(
+  logger: Logger,
   sourcePath: string,
-  destinationPath?: string,
+  targetPath?: string,
 ): Promise<void> {
   const sourceFullPath = resolve(sourcePath);
-  const bundlePath = destinationPath ?? resolveBundlePath(sourceFullPath);
+  const bundlePath = targetPath ?? resolveBundlePath(sourceFullPath);
   try {
-    const bundle = await bundleSource(sourceFullPath);
-    const bundleString = JSON.stringify(bundle);
-    await writeFile(bundlePath, bundleString);
-    console.log(`wrote ${bundlePath}: ${new Blob([bundleString]).size} bytes`);
+    const bundle = await endoBundleSource(sourceFullPath);
+    const bundleContent = JSON.stringify(bundle);
+    await writeFile(bundlePath, bundleContent);
+    logger.info(`wrote ${bundlePath}: ${new Blob([bundleContent]).size} bytes`);
   } catch (problem) {
-    console.error(problem);
+    logger.error(`error bundling file ${sourceFullPath}`, problem);
   }
 }
 
 /**
  * Create a bundle given path to an entry point.
  *
+ * @param logger - The logger to use for logging.
  * @param sourceDir - Path to a directory of source files to bundle.
  * @returns A promise that resolves when the bundles have been written.
  */
-export async function createBundleDir(sourceDir: string): Promise<void> {
-  console.log('bundling dir', sourceDir);
+export async function bundleDir(
+  logger: Logger,
+  sourceDir: string,
+): Promise<void> {
+  logger.info('bundling dir', sourceDir);
   await Promise.all(
     (await glob(join(sourceDir, '*.js'))).map(
-      async (source) => await createBundleFile(source),
+      async (source) => await bundleFile(logger, source),
     ),
   );
 }
 
 /**
  * Bundle a target file or every file in the target directory.
  *
- * @param target The file or directory to apply the bundler to.
+ * @param logger - The logger to use for logging.
+ * @param target - The file or directory to apply the bundler to.
  * @returns A promise that resolves when bundling is done.
  */
-export async function createBundle(target: string): Promise<void> {
-  await ((await isDirectory(target)) ? createBundleDir : createBundleFile)(
-    target,
-  );
+export async function bundleSource(
+  logger: Logger,
+  target: string,
+): Promise<void> {
+  try {
+    const targetIsDirectory = await isDirectory(target);
+    await (targetIsDirectory ? bundleDir : bundleFile)(logger, target);
+  } catch (problem) {
+    logger.error(`error bundling target ${target}`, problem);
+  }
 }