fix: use process.env when running fiddle (#1472)

* fix: use process.env when running fiddle

* test: rename env to originalEnv for clarity

src/interfaces.ts

@@ -266,6 +266,7 @@ export interface PackageJsonOptions {
 
 export interface StartFiddleParams {
   localPath: string | undefined;
+  enableElectronLogging: boolean;
   isValidBuild: boolean; // If the localPath is a valid Electron build
   version: string; // The user selected version
   dir: string;

src/main/fiddle-core.ts

@@ -28,7 +28,28 @@ export async function startFiddle(
   webContents: WebContents,
   params: StartFiddleParams,
 ): Promise<void> {
-  const { dir, env, isValidBuild, localPath, options, version } = params;
+  const {
+    dir,
+    enableElectronLogging,
+    isValidBuild,
+    localPath,
+    options,
+    version,
+  } = params;
+  const env = { ...process.env };
+
+  if (enableElectronLogging) {
+    env.ELECTRON_ENABLE_LOGGING = 'true';
+    env.ELECTRON_DEBUG_NOTIFICATIONS = 'true';
+    env.ELECTRON_ENABLE_STACK_DUMPING = 'true';
+  } else {
+    delete env.ELECTRON_ENABLE_LOGGING;
+    delete env.ELECTRON_DEBUG_NOTIFICATIONS;
+    delete env.ELECTRON_ENABLE_STACK_DUMPING;
+  }
+
+  Object.assign(env, params.env);
+
   const child = await runner.spawn(
     isValidBuild && localPath ? Installer.getExecPath(localPath) : version,
     dir,

src/renderer/runner.ts

@@ -331,19 +331,9 @@ export class Runner {
   }
 
   private buildChildEnvVars(): { [x: string]: string | undefined } {
-    const { isEnablingElectronLogging, environmentVariables } = this.appState;
-
-    const env = { ...process.env };
-
-    if (isEnablingElectronLogging) {
-      env.ELECTRON_ENABLE_LOGGING = 'true';
-      env.ELECTRON_DEBUG_NOTIFICATIONS = 'true';
-      env.ELECTRON_ENABLE_STACK_DUMPING = 'true';
-    } else {
-      delete env.ELECTRON_ENABLE_LOGGING;
-      delete env.ELECTRON_DEBUG_NOTIFICATIONS;
-      delete env.ELECTRON_ENABLE_STACK_DUMPING;
-    }
+    const { environmentVariables } = this.appState;
+
+    const env: Record<string, string> = {};
 
     for (const v of environmentVariables) {
       const [key, value] = v.split('=');
@@ -379,6 +369,7 @@ export class Runner {
       try {
         await window.ElectronFiddle.startFiddle({
           ...params,
+          enableElectronLogging: this.appState.isEnablingElectronLogging,
           options,
           env,
         });

tests/main/fiddle-core-spec.ts

@@ -0,0 +1,130 @@
+/**
+ * @jest-environment node
+ */
+
+import { ElectronVersions, Runner } from '@electron/fiddle-core';
+import { WebContents } from 'electron';
+import { mocked } from 'jest-mock';
+
+import { setupFiddleCore, startFiddle } from '../../src/main/fiddle-core';
+import { ChildProcessMock } from '../mocks/child-process';
+import { ElectronVersionsMock, FiddleRunnerMock } from '../mocks/fiddle-core';
+import { WebContentsMock } from '../mocks/web-contents';
+
+jest.mock('@electron/fiddle-core', () => {
+  const { FiddleRunnerMock, InstallerMock } = require('../mocks/fiddle-core');
+
+  return {
+    Installer: InstallerMock,
+    Runner: FiddleRunnerMock,
+  };
+});
+
+describe('fiddle-core', () => {
+  let runner: FiddleRunnerMock;
+  let originalEnv: NodeJS.ProcessEnv;
+  const mockEnv = Object.seal({
+    PATH: '/path/to/bin/',
+  });
+
+  beforeEach(() => {
+    runner = new FiddleRunnerMock();
+    originalEnv = { ...process.env };
+    process.env = mockEnv;
+    mocked(Runner.create).mockResolvedValue(runner as unknown as Runner);
+    setupFiddleCore(new ElectronVersionsMock() as unknown as ElectronVersions);
+  });
+
+  afterEach(() => {
+    process.env = originalEnv;
+  });
+
+  describe('startFiddle', () => {
+    const dir = '/path/to/fiddle';
+    const version = '18.0.0';
+
+    it('uses provided env', async () => {
+      const child = new ChildProcessMock();
+      mocked(runner.spawn).mockResolvedValue(child);
+
+      await startFiddle(new WebContentsMock() as unknown as WebContents, {
+        dir,
+        enableElectronLogging: false,
+        env: {
+          NODE_OPTIONS: '--inspect-brk',
+        },
+        isValidBuild: true,
+        localPath: undefined,
+        options: [],
+        version,
+      });
+
+      expect(runner.spawn).toHaveBeenCalledWith(
+        version,
+        dir,
+        expect.objectContaining({
+          env: {
+            ...mockEnv,
+            NODE_OPTIONS: '--inspect-brk',
+          },
+        }),
+      );
+    });
+
+    it('runs with logging when enabled', async () => {
+      const child = new ChildProcessMock();
+      mocked(runner.spawn).mockResolvedValue(child);
+
+      await startFiddle(new WebContentsMock() as unknown as WebContents, {
+        dir,
+        enableElectronLogging: true,
+        env: {},
+        isValidBuild: true,
+        localPath: undefined,
+        options: [],
+        version,
+      });
+
+      expect(runner.spawn).toHaveBeenCalledWith(
+        version,
+        dir,
+        expect.objectContaining({
+          env: {
+            ...mockEnv,
+            ELECTRON_DEBUG_NOTIFICATIONS: 'true',
+            ELECTRON_ENABLE_LOGGING: 'true',
+            ELECTRON_ENABLE_STACK_DUMPING: 'true',
+          },
+        }),
+      );
+    });
+
+    it('can set ELECTRON_ENABLE_LOGGING in env', async () => {
+      const child = new ChildProcessMock();
+      mocked(runner.spawn).mockResolvedValue(child);
+
+      await startFiddle(new WebContentsMock() as unknown as WebContents, {
+        dir,
+        enableElectronLogging: false,
+        env: {
+          ELECTRON_ENABLE_LOGGING: 'true',
+        },
+        isValidBuild: true,
+        localPath: undefined,
+        options: [],
+        version,
+      });
+
+      expect(runner.spawn).toHaveBeenCalledWith(
+        version,
+        dir,
+        expect.objectContaining({
+          env: {
+            ...mockEnv,
+            ELECTRON_ENABLE_LOGGING: 'true',
+          },
+        }),
+      );
+    });
+  });
+});

tests/mocks/fiddle-core.ts

@@ -1,17 +1,15 @@
-import { Installer } from '@electron/fiddle-core';
+import { EventEmitter } from 'node:events';
 
 import { ChildProcessMock } from './child-process';
 import { InstallState } from '../../src/interfaces';
 
-export class InstallerMock extends Installer {
+export class InstallerMock extends EventEmitter {
   public state = () => InstallState.installed;
 }
 
 export class FiddleRunnerMock {
-  public child = new ChildProcessMock();
-  public spawn = (): ChildProcessMock => {
-    return this.child;
-  };
+  public spawn: () => Promise<ChildProcessMock> = jest.fn();
+  public static create = jest.fn();
 }
 
 export class ElectronVersionsMock {

tests/renderer/runner-spec.tsx

@@ -60,11 +60,6 @@ describe('Runner component', () => {
 
     it('runs with logging when enabled', async () => {
       store.isEnablingElectronLogging = true;
-      const spy = jest.spyOn(window.ElectronFiddle, 'startFiddle');
-      spy.mockImplementationOnce(async (params) => {
-        expect(params.env).toHaveProperty('ELECTRON_ENABLE_LOGGING');
-        expect(params.env).toHaveProperty('ELECTRON_ENABLE_STACK_DUMPING');
-      });
 
       // wait for run() to get running
       const runPromise = instance.run();
@@ -79,6 +74,11 @@ describe('Runner component', () => {
       expect(store.isRunning).toBe(false);
       expect(fileManager.saveToTemp).toHaveBeenCalled();
       expect(window.ElectronFiddle.addModules).toHaveBeenCalled();
+      expect(window.ElectronFiddle.startFiddle).toBeCalledWith(
+        expect.objectContaining({
+          enableElectronLogging: true,
+        }),
+      );
     });
 
     it('emits output with exitCode', async () => {