test(wallet-cli): add subprocess daemon lifecycle e2e

.github/workflows/lint-build-test.yml

@@ -222,3 +222,31 @@ jobs:
             echo "Working tree dirty at end of job"
             exit 1
           fi
+
+  # The wallet-cli daemon e2e spawns the BUILT `mm` CLI and the native
+  # better-sqlite3 addon as real child processes, so it needs its dependency
+  # subtree built first and cannot run in the per-package `test-*` matrix above.
+  test-wallet-cli-e2e:
+    name: Test wallet-cli daemon e2e (${{ matrix.node-version }})
+    runs-on: ubuntu-latest
+    needs: prepare
+    strategy:
+      matrix:
+        node-version: [20.x, 22.x, 24.x]
+    steps:
+      - name: Checkout and setup environment
+        uses: MetaMask/action-checkout-and-setup@v3
+        with:
+          is-high-risk-environment: false
+          persist-credentials: false
+          node-version: ${{ matrix.node-version }}
+      - name: Build wallet-cli and its dependencies
+        run: yarn workspaces foreach --topological-dev --recursive --from '@metamask/wallet-cli' run build
+      - run: yarn workspace @metamask/wallet-cli run test:e2e
+      - name: Require clean working directory
+        shell: bash
+        run: |
+          if ! git diff --exit-code; then
+            echo "Working tree dirty at end of job"
+            exit 1
+          fi

eslint.config.mjs

@@ -321,6 +321,7 @@ const config = createConfig([
   {
     files: [
       'packages/wallet-cli/src/**/*.test.{js,ts}',
+      'packages/wallet-cli/tests/**/*.{js,ts}',
       'packages/platform-api-docs/**/*.{js,ts}',
     ],
     rules: {

packages/wallet-cli/README.md

@@ -55,6 +55,12 @@ Or invoke `prebuild-install` directly from the monorepo root (where `better-sqli
 cd node_modules/better-sqlite3 && node ../.bin/prebuild-install
 ```
 
+## Testing
+
+Unit tests run with `yarn workspace @metamask/wallet-cli test`.
+
+The subprocess end-to-end suite (in `tests/`) spawns the built `mm` CLI and the native `better-sqlite3` addon as real processes, so it is kept out of the unit run and its coverage gate. Build the workspace dependencies first (`yarn build` from the repo root), then run it with `yarn workspace @metamask/wallet-cli test:e2e`.
+
 ## Contributing
 
 This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme).

packages/wallet-cli/jest.config.e2e.js

@@ -0,0 +1,30 @@
+/*
+ * Jest configuration for the subprocess e2e suite in `tests/`.
+ *
+ * Kept separate from `jest.config.js` because these suites spawn the BUILT `mm`
+ * CLI and the native `better-sqlite3` addon as real child processes: they must
+ * stay out of the fast unit `test` run and must not be held to that run's
+ * 100%-coverage gate (subprocess work is invisible to in-process coverage).
+ * Run it with `yarn test:e2e`.
+ */
+
+const merge = require('deepmerge');
+
+const baseConfig = require('../../jest.config.packages');
+
+module.exports = merge(baseConfig, {
+  displayName: 'wallet-cli:e2e',
+
+  // Every test under `tests/` is a subprocess e2e; the default config runs
+  // everything in `src/`.
+  roots: ['<rootDir>/tests'],
+
+  // Coverage is meaningless here — the work happens in spawned processes — so
+  // collecting it would only report the e2e harness as uncovered source.
+  collectCoverage: false,
+
+  // The CLI runs in a normal Node process with the Web Crypto globals, so this
+  // suite needs neither the `jest.environment.js` polyfill nor any coverage
+  // threshold.
+  testEnvironment: 'node',
+});

packages/wallet-cli/jest.config.js

@@ -24,6 +24,11 @@ module.exports = merge(baseConfig, {
   // production code's test infrastructure, not production code itself.
   coveragePathIgnorePatterns: ['.*/src/test/.*'],
 
+  // The subprocess e2e suite lives in `tests/` and has its own config
+  // (`jest.config.e2e.js`, run via `yarn test:e2e`); it spawns the built CLI
+  // and must not run in the fast unit suite.
+  testPathIgnorePatterns: ['/node_modules/', '<rootDir>/tests/'],
+
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {

packages/wallet-cli/package.json

@@ -39,6 +39,7 @@
     "test:prepare": "./scripts/install-binaries.sh",
     "test": "yarn test:prepare && NODE_OPTIONS=--experimental-vm-modules jest --reporters=jest-silent-reporter",
     "test:clean": "NODE_OPTIONS=--experimental-vm-modules jest --clearCache",
+    "test:e2e": "yarn test:prepare && NODE_OPTIONS=--experimental-vm-modules jest --config jest.config.e2e.js",
     "test:verbose": "NODE_OPTIONS=--experimental-vm-modules jest --verbose",
     "test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch"
   },

packages/wallet-cli/src/daemon/daemon-spawn.test.ts

@@ -1,21 +1,31 @@
 import { spawn } from 'node:child_process';
-import { existsSync } from 'node:fs';
+import type { ChildProcess } from 'node:child_process';
+import { closeSync, existsSync, openSync } from 'node:fs';
 
 import { pingDaemon } from './daemon-client';
 import { ensureDaemon } from './daemon-spawn';
+import { ensureOwnerOnlyDirectory } from './data-dir';
 import { getDaemonPaths } from './paths';
 import type { DaemonSpawnConfig } from './types';
 
 jest.mock('node:child_process');
 jest.mock('node:fs');
 jest.mock('./daemon-client');
+jest.mock('./data-dir');
 jest.mock('./paths');
 
 const mockSpawn = jest.mocked(spawn);
 const mockExistsSync = jest.mocked(existsSync);
+const mockOpenSync = jest.mocked(openSync);
+const mockCloseSync = jest.mocked(closeSync);
+const mockEnsureOwnerOnlyDirectory = jest.mocked(ensureOwnerOnlyDirectory);
 const mockPingDaemon = jest.mocked(pingDaemon);
 const mockGetDaemonPaths = jest.mocked(getDaemonPaths);
 
+// Arbitrary file descriptor handed back by the mocked `openSync` so tests can
+// assert it is wired into the child's stdio and later closed in the parent.
+const LOG_FILE_DESCRIPTOR = 7;
+
 const CONFIG: DaemonSpawnConfig = {
   dataDir: '/tmp/data',
   infuraProjectId: 'test-key',
@@ -65,7 +75,7 @@ function setupSpawnMock(): SpawnMock {
       listeners.get('exit')?.(code, signal);
     },
   };
-  mockSpawn.mockReturnValue(result as never);
+  mockSpawn.mockReturnValue(result as unknown as ChildProcess);
   return result;
 }
 
@@ -79,6 +89,8 @@ describe('ensureDaemon', () => {
       logPath: '/tmp/test.log',
       dbPath: '/tmp/wallet.db',
     });
+    mockOpenSync.mockReturnValue(LOG_FILE_DESCRIPTOR);
+    mockEnsureOwnerOnlyDirectory.mockResolvedValue(undefined);
     setupSpawnMock();
   });
 
@@ -128,7 +140,7 @@ describe('ensureDaemon', () => {
       ['/pkg/dist/daemon/daemon-entry.mjs'],
       expect.objectContaining({
         detached: true,
-        stdio: 'ignore',
+        stdio: ['ignore', 'ignore', LOG_FILE_DESCRIPTOR],
         env: expect.objectContaining({
           MM_DAEMON_DATA_DIR: '/tmp/data',
           MM_DAEMON_SOCKET_PATH: '/tmp/test.sock',
@@ -141,6 +153,57 @@ describe('ensureDaemon', () => {
     );
   });
 
+  it('redirects the daemon stderr to its log file and closes the parent file descriptor', async () => {
+    mockPingDaemon
+      .mockResolvedValueOnce(ABSENT)
+      .mockResolvedValueOnce(RESPONSIVE);
+    mockExistsSync.mockReturnValue(true);
+
+    await ensureDaemon(CONFIG);
+
+    expect(mockOpenSync).toHaveBeenCalledWith('/tmp/test.log', 'a');
+    const spawnOptions = mockSpawn.mock.calls[0][2] as { stdio: unknown };
+    expect(spawnOptions.stdio).toStrictEqual([
+      'ignore',
+      'ignore',
+      LOG_FILE_DESCRIPTOR,
+    ]);
+    expect(mockCloseSync).toHaveBeenCalledWith(LOG_FILE_DESCRIPTOR);
+  });
+
+  it('propagates a log-file open failure without spawning', async () => {
+    mockPingDaemon.mockResolvedValue(ABSENT);
+    mockExistsSync.mockReturnValue(true);
+    mockOpenSync.mockImplementation(() => {
+      throw Object.assign(new Error('EACCES'), { code: 'EACCES' });
+    });
+
+    await expect(ensureDaemon(CONFIG)).rejects.toThrow('EACCES');
+    expect(mockSpawn).not.toHaveBeenCalled();
+  });
+
+  it('creates the data directory before opening the log file', async () => {
+    mockPingDaemon
+      .mockResolvedValueOnce(ABSENT)
+      .mockResolvedValueOnce(RESPONSIVE);
+    mockExistsSync.mockReturnValue(true);
+    // The log lives inside the data dir, so the dir must be created first (else
+    // openSync ENOENTs on a fresh dir).
+    const order: string[] = [];
+    mockEnsureOwnerOnlyDirectory.mockImplementation(async () => {
+      order.push('ensureDir');
+    });
+    mockOpenSync.mockImplementation(() => {
+      order.push('openLog');
+      return LOG_FILE_DESCRIPTOR;
+    });
+
+    await ensureDaemon(CONFIG);
+
+    expect(mockEnsureOwnerOnlyDirectory).toHaveBeenCalledWith('/tmp/data');
+    expect(order).toStrictEqual(['ensureDir', 'openLog']);
+  });
+
   it('returns started when the spawned daemon becomes responsive', async () => {
     mockPingDaemon
       .mockResolvedValueOnce(ABSENT)
@@ -226,7 +289,10 @@ describe('ensureDaemon', () => {
         }
       },
     );
-    mockSpawn.mockReturnValue({ unref: jest.fn(), on } as never);
+    mockSpawn.mockReturnValue({
+      unref: jest.fn(),
+      on,
+    } as unknown as ChildProcess);
 
     jest.useFakeTimers();
     const promise = ensureDaemon(CONFIG);
@@ -267,7 +333,10 @@ describe('ensureDaemon', () => {
         }
       },
     );
-    mockSpawn.mockReturnValue({ unref: jest.fn(), on } as never);
+    mockSpawn.mockReturnValue({
+      unref: jest.fn(),
+      on,
+    } as unknown as ChildProcess);
 
     jest.useFakeTimers();
     const promise = ensureDaemon(CONFIG);
@@ -283,6 +352,35 @@ describe('ensureDaemon', () => {
     expect((thrownError as Error).message).toContain('/tmp/test.log');
   });
 
+  it('reports the spawn error when the child both errors and exits', async () => {
+    mockPingDaemon.mockResolvedValue(ABSENT);
+    mockExistsSync.mockReturnValue(true);
+    const on = jest.fn(
+      (event: string, handler: (...args: unknown[]) => void) => {
+        if (event === 'error') {
+          handler(new Error('spawn ENOENT'));
+        }
+        if (event === 'exit') {
+          handler(1, null);
+        }
+      },
+    );
+    mockSpawn.mockReturnValue({
+      unref: jest.fn(),
+      on,
+    } as unknown as ChildProcess);
+
+    jest.useFakeTimers();
+    const promise = ensureDaemon(CONFIG);
+    const rejection = promise.catch((thrown: unknown) => thrown);
+    await jest.advanceTimersByTimeAsync(200);
+
+    const thrownError = await rejection;
+    expect((thrownError as Error).message).toContain(
+      'Failed to spawn daemon process',
+    );
+  });
+
   it('writes spawn errors to stderr', async () => {
     mockPingDaemon
       .mockResolvedValueOnce(ABSENT)
@@ -297,7 +395,10 @@ describe('ensureDaemon', () => {
         }
       },
     );
-    mockSpawn.mockReturnValue({ unref: jest.fn(), on } as never);
+    mockSpawn.mockReturnValue({
+      unref: jest.fn(),
+      on,
+    } as unknown as ChildProcess);
 
     await ensureDaemon(CONFIG);
     errorHandler?.(new Error('spawn ENOENT'));