test(node-runtime-worker-thread): Refactor tests to make it clearer when done is called; make blocking script less aggressive

Author: gribnoysup

packages/node-runtime-worker-thread/src/spawn-child-from-source.spec.ts

@@ -1,47 +1,69 @@
-import chai, { expect } from 'chai';
-import chaiAsPromised from 'chai-as-promised';
-import childProcess from 'child_process';
+import { expect } from 'chai';
+import childProcess, { ChildProcess } from 'child_process';
 import { once } from 'events';
 
 import spawnChildFromSource, { kill } from './spawn-child-from-source';
 
-chai.use(chaiAsPromised);
-
 describe('spawnChildFromSource', () => {
+  let spawned: ChildProcess;
+
+  afterEach(async() => {
+    if (spawned) {
+      await kill(spawned, 'SIGKILL');
+      spawned = null;
+    }
+  });
+
   it('should resolve with a child process', async() => {
-    const spawned = await spawnChildFromSource('');
+    spawned = await spawnChildFromSource('');
     expect(spawned).to.be.instanceof((childProcess as any).ChildProcess);
-    await kill(spawned);
   });
 
   it('should spawn a process with an ipc channel open', async() => {
-    const spawned = await spawnChildFromSource(
+    spawned = await spawnChildFromSource(
       'process.on("message", (data) => process.send(data))'
     );
     spawned.send('Hi!');
     const [message] = await once(spawned, 'message');
     expect(message).to.equal('Hi!');
-    await kill(spawned);
   });
 
-  it('should fail if process exited before successfully starting', () => {
-    return expect(
-      spawnChildFromSource(
+  it('should fail if process exited before successfully starting', async() => {
+    let err: Error;
+
+    try {
+      spawned = await spawnChildFromSource(
         'throw new Error("Whoops!")',
         {},
         undefined,
         'ignore',
         'ignore'
-      )
-    ).to.eventually.be.rejectedWith(
+      );
+    } catch (e) {
+      err = e;
+    }
+
+    expect(err).to.be.instanceof(Error);
+    expect(err.message).to.match(
       /Child process exited with error before starting/
     );
   });
 
-  it('should fail if a timeout exceeded before the process is "ready"', () => {
-    return expect(
-      spawnChildFromSource('while(true){}', {}, 200)
-    ).to.eventually.be.rejectedWith(
+  it('should fail if a timeout exceeded before the process is "ready"', async() => {
+    let err: Error;
+
+    try {
+      spawned = await spawnChildFromSource(
+        'let i = 0; while(i++ < 1000000000){};',
+        {},
+        0
+      );
+    } catch (e) {
+      err = e;
+    }
+
+    expect(err).to.be.instanceof(Error);
+    expect(err.message).to.match(
       /Timed out while waiting for child process to start/
     );
   });

packages/node-runtime-worker-thread/src/spawn-child-from-source.ts

@@ -10,12 +10,12 @@ import {
 } from 'child_process';
 import { once } from 'events';
 
-export async function kill(childProcess: ChildProcess, code: NodeJS.Signals | number = 'SIGTERM') {
+export async function kill(
+  childProcess: ChildProcess,
+  code: NodeJS.Signals | number = 'SIGTERM'
+) {
   childProcess.kill(code);
-  if (
-    childProcess.exitCode === null &&
-    childProcess.signalCode === null
-  ) {
+  if (childProcess.exitCode === null && childProcess.signalCode === null) {
     await once(childProcess, 'exit');
   }
 }
@@ -25,7 +25,7 @@ export default function spawnChildFromSource(
   spawnOptions: SpawnOptionsWithoutStdio = {},
   timeoutMs?: number,
   _stdout: StdioNull | StdioPipe = 'inherit',
-  _stderr: StdioNull | StdioPipe = 'inherit',
+  _stderr: StdioNull | StdioPipe = 'inherit'
 ): Promise<ChildProcess> {
   return new Promise(async(resolve, reject) => {
     const readyToken = Date.now().toString(32);
@@ -37,6 +37,7 @@ export default function spawnChildFromSource(
 
     if (!childProcess.stdin) {
       await kill(childProcess);
+
       return reject(
         new Error("Can't write src to the spawned process, missing stdin")
       );
@@ -45,25 +46,44 @@ export default function spawnChildFromSource(
     // eslint-disable-next-line prefer-const
     let timeoutId: NodeJS.Timeout | null;
 
-    const onExit = (exitCode: number | null) => {
+    function cleanupListeners() {
+      if (timeoutId) {
+        clearTimeout(timeoutId);
+      }
+      if (childProcess.stdin) {
+        childProcess.stdin.off('error', onWriteError);
+        // To swallow a possible error about writing into a closed pipe if we
+        // e.g., killed a process before our script finished writing
+        childProcess.stdin.on('error', () => {});
+      }
+      childProcess.off('message', onMessage);
+      childProcess.off('exit', onExit);
+    }
+
+    async function onExit(exitCode: number | null) {
       if (exitCode && exitCode > 0) {
+        cleanupListeners();
+        await kill(childProcess);
         reject(new Error('Child process exited with error before starting'));
       }
-    };
+    }
 
-    const onMessage = (data: Serializable) => {
+    async function onWriteError(error: Error) {
+      cleanupListeners();
+      await kill(childProcess);
+      reject(error);
+    }
+
+    function onMessage(data: Serializable) {
       if (data === readyToken) {
-        if (timeoutId) {
-          clearTimeout(timeoutId);
-        }
-        childProcess.off('exit', onExit);
+        cleanupListeners();
         resolve(childProcess);
       }
-    };
+    }
 
     childProcess.on('message', onMessage);
-
     childProcess.on('exit', onExit);
+    childProcess.stdin.on('error', onWriteError);
 
     childProcess.stdin.write(src);
     childProcess.stdin.write(`;process.send(${JSON.stringify(readyToken)})`);
@@ -72,6 +92,7 @@ export default function spawnChildFromSource(
     timeoutId =
       timeoutMs !== undefined
         ? setTimeout(async() => {
+          cleanupListeners();
           await kill(childProcess);
           reject(
             new Error('Timed out while waiting for child process to start')