fix: make sure env variables are consistently applied when parsing args (#1994)

This PR moves the checks for CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS and
CI env vars to parseArguments making sure it is correctly applied by the
daemon and in tests. It also updates the tests to set explicit
CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS as it might not always be
inherited. This change is likely to affect the metrics collected and
might explain some fluctuations we observed.

src/bin/chrome-devtools-mcp-cli-options.ts

@@ -292,7 +292,11 @@ export const cliOptions = {
 
 export type ParsedArguments = ReturnType<typeof parseArguments>;
 
-export function parseArguments(version: string, argv = process.argv) {
+export function parseArguments(
+  version: string,
+  argv = process.argv,
+  env = process.env,
+) {
   const yargsInstance = yargs(hideBin(argv))
     .scriptName('npx chrome-devtools-mcp@latest')
     .options(cliOptions)
@@ -307,6 +311,12 @@ export function parseArguments(version: string, argv = process.argv) {
       ) {
         args.channel = 'stable';
       }
+      if (env['CI'] || env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS']) {
+        console.error(
+          "turning off usage statistics. process.env['CI'] || process.env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS'] is set.",
+        );
+        args.usageStatistics = false;
+      }
       return true;
     })
     .example([

src/bin/chrome-devtools-mcp-main.ts

@@ -24,15 +24,6 @@ await checkForUpdates(
 export const args = parseArguments(VERSION);
 
 const logFile = args.logFile ? saveLogsToFile(args.logFile) : undefined;
-if (
-  process.env['CI'] ||
-  process.env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS']
-) {
-  console.error(
-    "turning off usage statistics. process.env['CI'] || process.env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS'] is set.",
-  );
-  args.usageStatistics = false;
-}
 
 if (process.env['CHROME_DEVTOOLS_MCP_CRASH_ON_UNCAUGHT'] !== 'true') {
   process.on('unhandledRejection', (reason, promise) => {

tests/cli.test.ts

@@ -32,7 +32,7 @@ describe('cli args parsing', () => {
   };
 
   it('parses with default args', async () => {
-    const args = parseArguments('1.0.0', ['node', 'main.js']);
+    const args = parseArguments('1.0.0', ['node', 'main.js'], {});
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -43,12 +43,11 @@ describe('cli args parsing', () => {
   });
 
   it('parses with browser url', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--browserUrl',
-      'http://localhost:3000',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--browserUrl', 'http://localhost:3000'],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -61,12 +60,11 @@ describe('cli args parsing', () => {
   });
 
   it('parses with user data dir', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--user-data-dir',
-      '/tmp/chrome-profile',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--user-data-dir', '/tmp/chrome-profile'],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -79,12 +77,11 @@ describe('cli args parsing', () => {
   });
 
   it('parses an empty browser url', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--browserUrl',
-      '',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--browserUrl', ''],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -98,12 +95,11 @@ describe('cli args parsing', () => {
   });
 
   it('parses with executable path', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--executablePath',
-      '/tmp/test 123/chrome',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--executablePath', '/tmp/test 123/chrome'],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -116,12 +112,11 @@ describe('cli args parsing', () => {
   });
 
   it('parses viewport', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--viewport',
-      '888x777',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--viewport', '888x777'],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -136,12 +131,16 @@ describe('cli args parsing', () => {
   });
 
   it('parses chrome args', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      `--chrome-arg='--no-sandbox'`,
-      `--chrome-arg='--disable-setuid-sandbox'`,
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      [
+        'node',
+        'main.js',
+        `--chrome-arg='--no-sandbox'`,
+        `--chrome-arg='--disable-setuid-sandbox'`,
+      ],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -154,12 +153,16 @@ describe('cli args parsing', () => {
   });
 
   it('parses ignore chrome args', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      `--ignore-default-chrome-arg='--disable-extensions'`,
-      `--ignore-default-chrome-arg='--disable-cancel-all-touches'`,
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      [
+        'node',
+        'main.js',
+        `--ignore-default-chrome-arg='--disable-extensions'`,
+        `--ignore-default-chrome-arg='--disable-cancel-all-touches'`,
+      ],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -178,12 +181,16 @@ describe('cli args parsing', () => {
   });
 
   it('parses wsEndpoint with ws:// protocol', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--wsEndpoint',
-      'ws://127.0.0.1:9222/devtools/browser/abc123',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      [
+        'node',
+        'main.js',
+        '--wsEndpoint',
+        'ws://127.0.0.1:9222/devtools/browser/abc123',
+      ],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -196,12 +203,16 @@ describe('cli args parsing', () => {
   });
 
   it('parses wsEndpoint with wss:// protocol', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--wsEndpoint',
-      'wss://example.com:9222/devtools/browser/abc123',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      [
+        'node',
+        'main.js',
+        '--wsEndpoint',
+        'wss://example.com:9222/devtools/browser/abc123',
+      ],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -214,14 +225,18 @@ describe('cli args parsing', () => {
   });
 
   it('parses wsHeaders with valid JSON', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--wsEndpoint',
-      'ws://127.0.0.1:9222/devtools/browser/abc123',
-      '--wsHeaders',
-      '{"Authorization":"Bearer token","X-Custom":"value"}',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      [
+        'node',
+        'main.js',
+        '--wsEndpoint',
+        'ws://127.0.0.1:9222/devtools/browser/abc123',
+        '--wsHeaders',
+        '{"Authorization":"Bearer token","X-Custom":"value"}',
+      ],
+      {},
+    );
     assert.deepStrictEqual(args.wsHeaders, {
       Authorization: 'Bearer token',
       'X-Custom': 'value',
@@ -229,11 +244,11 @@ describe('cli args parsing', () => {
   });
 
   it('parses disabled category', async () => {
-    const args = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--no-category-emulation',
-    ]);
+    const args = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--no-category-emulation'],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -245,7 +260,11 @@ describe('cli args parsing', () => {
     });
   });
   it('parses auto-connect', async () => {
-    const args = parseArguments('1.0.0', ['node', 'main.js', '--auto-connect']);
+    const args = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--auto-connect'],
+      {},
+    );
     assert.deepStrictEqual(args, {
       ...defaultArgs,
       _: [],
@@ -259,23 +278,51 @@ describe('cli args parsing', () => {
 
   it('parses usage statistics flag', async () => {
     // Test default (should be true).
-    const defaultArgs = parseArguments('1.0.0', ['node', 'main.js']);
+    const defaultArgs = parseArguments('1.0.0', ['node', 'main.js'], {});
     assert.strictEqual(defaultArgs.usageStatistics, true);
 
     // Test enabling it
-    const enabledArgs = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--usage-statistics',
-    ]);
+    const enabledArgs = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--usage-statistics'],
+      {},
+    );
     assert.strictEqual(enabledArgs.usageStatistics, true);
 
     // Test disabling it
-    const disabledArgs = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--no-usage-statistics',
-    ]);
+    const disabledArgs = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--no-usage-statistics'],
+      {},
+    );
+    assert.strictEqual(disabledArgs.usageStatistics, false);
+  });
+
+  it('respects env variable', async () => {
+    // Test default (should be true).
+    const defaultArgs = parseArguments('1.0.0', ['node', 'main.js'], {
+      CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS: 'true',
+    });
+    assert.strictEqual(defaultArgs.usageStatistics, false);
+
+    // Test enabling it
+    const enabledArgs = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--usage-statistics'],
+      {
+        CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS: 'true',
+      },
+    );
+    assert.strictEqual(enabledArgs.usageStatistics, false);
+
+    // Test disabling it
+    const disabledArgs = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--no-usage-statistics'],
+      {
+        CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS: 'true',
+      },
+    );
     assert.strictEqual(disabledArgs.usageStatistics, false);
   });
 
@@ -284,18 +331,18 @@ describe('cli args parsing', () => {
     assert.strictEqual(defaultArgs.performanceCrux, true);
 
     // force enable
-    const enabledArgs = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--performance-crux',
-    ]);
+    const enabledArgs = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--performance-crux'],
+      {},
+    );
     assert.strictEqual(enabledArgs.performanceCrux, true);
 
-    const disabledArgs = parseArguments('1.0.0', [
-      'node',
-      'main.js',
-      '--no-performance-crux',
-    ]);
+    const disabledArgs = parseArguments(
+      '1.0.0',
+      ['node', 'main.js', '--no-performance-crux'],
+      {},
+    );
     assert.strictEqual(disabledArgs.performanceCrux, false);
   });
 });

tests/index.test.ts

@@ -40,6 +40,7 @@ describe('e2e', () => {
         executablePath(),
         ...extraArgs,
       ],
+      env: {...process.env, CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS: 'true'},
     });
     const client = new Client(
       {

tests/utils.ts

@@ -365,7 +365,9 @@ export async function runCli(
     if (sessionId) {
       finalArgs.push('--sessionId', sessionId);
     }
-    const child = spawn('node', [CLI_PATH, ...finalArgs]);
+    const child = spawn('node', [CLI_PATH, ...finalArgs], {
+      env: process.env,
+    });
     let stdout = '';
     let stderr = '';
     child.stdout.on('data', chunk => {