Add tests and refactor manifest validation logic

Author: Mrtenz

packages/examples/packages/bip32/snap.manifest.json

@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "S8JJzZaDPhHp16nyJVCPY++KVgOpYSCUrEVp/dmbsRE=",
+    "shasum": "I0xKCYPmg9IuOUQO/JbXr5YFHLhIKvNfr6JDfe+CsTE=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",
@@ -17,7 +17,6 @@
     }
   },
   "initialPermissions": {
-    "endowment:page-home": {},
     "snap_dialog": {},
     "snap_getBip32Entropy": [
       {

packages/examples/packages/wasm/snap.manifest.json

@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "0iz4fr9QPZehD0EQCZ5rkpOKiKCKVhUwKEZ7ZhjUBLk=",
+    "shasum": "CUAG/rLnbWYRzl9u72XRYmlYDg5Lh6vKG7s0Io3iLOU=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",
@@ -17,9 +17,6 @@
     }
   },
   "initialPermissions": {
-    "endowment:rpc": {
-      "dapps": true
-    },
     "endowment:webassembly": {}
   },
   "platformVersion": "6.22.1",

packages/snaps-cli/src/__mocks__/ora.ts

@@ -9,7 +9,9 @@ class MockSpinner {
 
   fail = jest.fn();
 
-  stop = jest.fn();
+  stop = jest.fn().mockImplementation(() => {
+    this.isSpinning = false;
+  });
 
   clear = jest.fn();
 

packages/snaps-cli/src/commands/build/build.e2e.test.ts

@@ -1,6 +1,3 @@
-import { promises as fs } from 'fs';
-import { join } from 'path';
-
 import type { TestRunner } from '../../test-utils';
 import { getCommandRunner } from '../../test-utils';
 
@@ -21,20 +18,19 @@ describe('mm-snap build', () => {
         expect.stringMatching(/Checking the input file\./u),
       );
       expect(runner.stdout).toContainEqual(
-        expect.stringMatching(/Building the snap bundle\./u),
+        expect.stringMatching(/Building the Snap bundle\./u),
       );
-      expect(runner.stderr).toContainEqual(
-        expect.stringMatching(
-          /Compiled \d+ files? in \d+ms with \d+ warnings?\./u,
-        ),
+
+      expect(runner.stdout).toContainEqual(
+        expect.stringMatching(/Compiled \d+ files? in \d+ms\./u),
       );
-      expect(runner.stderr).toContainEqual(
+      expect(runner.stdout).toContainEqual(
         expect.stringContaining(
           'No icon found in the Snap manifest. It is recommended to include an icon for the Snap. See https://docs.metamask.io/snaps/how-to/design-a-snap/#guidelines-at-a-glance for more information.',
         ),
       );
       expect(runner.stdout).toContainEqual(
-        expect.stringMatching(/Evaluating the snap bundle\./u),
+        expect.stringMatching(/Evaluating the Snap bundle\./u),
       );
       expect(runner.exitCode).toBe(0);
     },

packages/snaps-cli/src/commands/build/build.test.ts

@@ -7,17 +7,34 @@ import { buildHandler } from './build';
 import { build } from './implementation';
 import { getMockConfig } from '../../test-utils';
 import { evaluate } from '../eval';
+import { manifest } from '../manifest';
 
 jest.mock('fs');
 jest.mock('../eval');
+jest.mock('../manifest');
 jest.mock('./implementation');
 
 jest.mock('webpack-bundle-analyzer', () => ({
   BundleAnalyzerPlugin: jest.fn(),
 }));
 
 describe('buildHandler', () => {
-  it('builds a snap', async () => {
+  beforeEach(() => {
+    jest.mocked(evaluate).mockResolvedValue({
+      exports: [],
+      stdout: '',
+      stderr: '',
+    });
+
+    jest.mocked(manifest).mockResolvedValue({
+      valid: true,
+      errors: 0,
+      warnings: 0,
+      fixed: 0,
+    });
+  });
+
+  it('builds a Snap', async () => {
     await fs.promises.writeFile('/input.js', DEFAULT_SNAP_BUNDLE);
 
     jest.spyOn(console, 'log').mockImplementation();
@@ -43,7 +60,7 @@ describe('buildHandler', () => {
     );
   });
 
-  it('analyzes a snap bundle', async () => {
+  it('analyzes a Snap bundle', async () => {
     await fs.promises.writeFile('/input.js', DEFAULT_SNAP_BUNDLE);
 
     jest.spyOn(console, 'log').mockImplementation();
@@ -121,7 +138,7 @@ describe('buildHandler', () => {
     expect(process.exitCode).toBe(1);
     expect(log).toHaveBeenCalledWith(
       expect.stringMatching(
-        /Input file not found: ".+"\. Make sure that the "input" field in your snap config is correct\./u,
+        /Input file not found: ".+"\. Make sure that the "input" field in your Snap config is correct\./u,
       ),
     );
   });

packages/snaps-cli/src/commands/build/build.ts

@@ -1,26 +1,23 @@
-import { handlerEndowments } from '@metamask/snaps-rpc-methods';
-import { checkManifest, isFile } from '@metamask/snaps-utils/node';
-import { writeManifest } from '@metamask/snaps-webpack-plugin';
+import { isFile } from '@metamask/snaps-utils/node';
 import { assert } from '@metamask/utils';
-import { red, reset, yellow } from 'chalk';
-import { readFile } from 'fs/promises';
-import { dirname, resolve as pathResolve } from 'path';
+import { resolve } from 'path';
 
 import { build } from './implementation';
 import { getBundleAnalyzerPort } from './utils';
 import type { ProcessedConfig } from '../../config';
 import { CommandError } from '../../errors';
 import type { Steps } from '../../utils';
-import { error, success, executeSteps, info, warn } from '../../utils';
-import { formatError } from '../../webpack/utils';
+import { info, success, executeSteps } from '../../utils';
 import { evaluate } from '../eval';
+import { manifest } from '../manifest';
+import { showManifestMessage } from '../manifest/manifest';
 
 export type BuildContext = {
   analyze: boolean;
   build: boolean;
   config: ProcessedConfig;
-  port?: number;
   exports?: string[];
+  port?: number;
 };
 
 export const steps: Steps<BuildContext> = [
@@ -40,7 +37,9 @@ export const steps: Steps<BuildContext> = [
   {
     name: 'Building the Snap bundle.',
     condition: ({ build: enableBuild }) => enableBuild,
-    task: async ({ analyze, build: enableBuild, config, spinner }) => {
+    task: async (context) => {
+      const { analyze, config, spinner } = context;
+
       // We don't evaluate the bundle here, because it's done in a separate
       // step.
       const compiler = await build(config, {
@@ -51,10 +50,7 @@ export const steps: Steps<BuildContext> = [
 
       if (analyze) {
         return {
-          analyze,
-          build: enableBuild,
-          config,
-          spinner,
+          ...context,
           port: await getBundleAnalyzerPort(compiler),
         };
       }
@@ -68,7 +64,7 @@ export const steps: Steps<BuildContext> = [
       enableBuild && config.evaluate,
     task: async (context) => {
       const { config, spinner } = context;
-      const path = pathResolve(
+      const path = resolve(
         process.cwd(),
         config.output.path,
         config.output.filename,
@@ -84,65 +80,18 @@ export const steps: Steps<BuildContext> = [
       };
     },
   },
-
-  // TODO: Share this between the `build` and `manifest` commands.
   {
     name: 'Validating the Snap manifest.',
-    condition: ({ config }) => config.evaluate,
     task: async ({ config, exports, spinner }) => {
-      const bundlePath = pathResolve(
-        process.cwd(),
-        config.output.path,
-        config.output.filename,
-      );
-
-      const { reports } = await checkManifest(dirname(config.manifest.path), {
-        updateAndWriteManifest: config.manifest.update,
-        sourceCode: await readFile(bundlePath, 'utf-8'),
+      const stats = await manifest(
+        config,
+        config.manifest.path,
+        config.manifest.update,
         exports,
-        handlerEndowments,
-        writeFileFn: async (path, data) => {
-          return writeManifest(path, data);
-        },
-      });
-
-      // TODO: Use `Object.groupBy` when available.
-      const errors = reports
-        .filter((report) => report.severity === 'error' && !report.wasFixed)
-        .map((report) => report.message);
-      const warnings = reports
-        .filter((report) => report.severity === 'warning' && !report.wasFixed)
-        .map((report) => report.message);
-      const fixed = reports
-        .filter((report) => report.wasFixed)
-        .map((report) => report.message);
-
-      if (errors.length > 0) {
-        error(
-          `The following errors were found in the manifest:\n\n${errors
-            .map((value) => formatError(value, '', red))
-            .join('\n\n')}\n`,
-          spinner,
-        );
-      }
-
-      if (warnings.length > 0) {
-        warn(
-          `The following warnings were found in the manifest:\n\n${warnings
-            .map((value) => formatError(value, '', yellow))
-            .join('\n\n')}\n`,
-          spinner,
-        );
-      }
+        spinner,
+      );
 
-      if (fixed.length > 0) {
-        info(
-          `The following issues were fixed in the manifest:\n\n${reset(
-            fixed.map((value) => formatError(value, '', reset)).join('\n\n'),
-          )}\n`,
-          spinner,
-        );
-      }
+      showManifestMessage(stats, config.manifest.update, spinner);
     },
   },
   {

packages/snaps-cli/src/commands/build/implementation.test.ts

@@ -8,7 +8,7 @@ import {
 } from '@metamask/snaps-utils/test-utils';
 import type { SemVerVersion } from '@metamask/utils';
 import normalFs from 'fs';
-import { dirname, resolve } from 'path';
+import { dirname } from 'path';
 import type { Configuration } from 'webpack';
 
 import { build } from './implementation';
@@ -86,7 +86,7 @@ describe('build', () => {
 
   it('builds the snap bundle using Webpack', async () => {
     jest.spyOn(process, 'cwd').mockReturnValue('/snap');
-    const warn = jest.spyOn(console, 'warn').mockImplementation();
+    const log = jest.spyOn(console, 'log').mockImplementation();
 
     const config = getMockConfig({
       input: '/snap/input.js',
@@ -107,8 +107,8 @@ describe('build', () => {
     await build(config);
 
     // Manifest checksum mismatch is the warning
-    expect(warn).toHaveBeenCalledWith(
-      expect.stringMatching(/Compiled 1 file in \d+ms with 1 warning\./u),
+    expect(log).toHaveBeenCalledWith(
+      expect.stringMatching(/Compiled 1 file in \d+ms\./u),
     );
 
     const output = await fs.readFile('/snap/output.js', 'utf8');
@@ -119,7 +119,7 @@ describe('build', () => {
 
   it('builds an unminimized snap bundle using Webpack', async () => {
     jest.spyOn(process, 'cwd').mockReturnValue('/snap');
-    const warn = jest.spyOn(console, 'warn').mockImplementation();
+    const log = jest.spyOn(console, 'log').mockImplementation();
 
     const config = getMockConfig({
       input: '/snap/input.js',
@@ -141,8 +141,8 @@ describe('build', () => {
     await build(config);
 
     // Manifest checksum mismatch is the warning
-    expect(warn).toHaveBeenCalledWith(
-      expect.stringMatching(/Compiled 1 file in \d+ms with 1 warning\./u),
+    expect(log).toHaveBeenCalledWith(
+      expect.stringMatching(/Compiled 1 file in \d+ms\./u),
     );
 
     const output = await fs.readFile('/snap/output.js', 'utf8');

packages/snaps-cli/src/commands/build/implementation.ts

@@ -11,10 +11,7 @@ import { getCompiler } from '../../webpack';
  * @param options - The Webpack options.
  * @returns A promise that resolves when the bundle is built.
  */
-export async function build(
-  config: ProcessedConfig,
-  options?: WebpackOptions,
-) {
+export async function build(config: ProcessedConfig, options?: WebpackOptions) {
   const compiler = await getCompiler(config, options);
   return await new Promise<Compiler>((resolve, reject) => {
     compiler.run((runError) => {

packages/snaps-cli/src/commands/build/index.ts

@@ -8,8 +8,7 @@ const command = {
   command: ['build', 'b'],
   desc: 'Build snap from source',
   builder: (yarg: yargs.Argv) => {
-    yarg
-      .option('analyze', builders.analyze)
+    yarg.option('analyze', builders.analyze);
   },
   handler: async (argv: YargsArgs) =>
     buildHandler(argv.context.config, argv.analyze),