Repository: saucelabs/node-saucelabs
Pull request #278 (2025-01-31 13:27:31)

Fix running sauce connect with --log-file
# One-line summary

## Description

Use the API exposed by Sauce Connect for health checks instead of parsing log lines from stdout. SC5 doesn't print anything (almost) to stdout and stderr when `--log-file` is passed.

## Types of Changes
_What types of changes does your code introduce? Keep the ones that apply:_

- Refactor/improvements


Patch:
diff --git a/jest.config.js b/jest.config.js
index 0d80a7f..c35166d 100644
--- a/jest.config.js
+++ b/jest.config.js
@@ -6,7 +6,7 @@ module.exports = {
   coverageThreshold: {
     global: {
       branches: 93,
-      functions: 98,
+      functions: 97,
       lines: 97,
       statements: 97,
     },
diff --git a/src/constants.js b/src/constants.js
index f886c17..b3344c9 100644
--- a/src/constants.js
+++ b/src/constants.js
@@ -329,7 +329,6 @@ export const SC_PARAMS_TO_STRIP = [
   ...SAUCE_CONNECT_CLI_PARAM_ALIASES,
 ];
 
-export const SC_READY_MESSAGE = 'Sauce Connect is up, you may start your tests';
-export const SC_FAILURE_MESSAGES = ['fatal error exiting'];
-export const SC_CLOSE_MESSAGE = 'tunnel was shutdown';
 export const SC_CLOSE_TIMEOUT = 5000;
+export const SC_READY_TIMEOUT = 30000;
+export const SC_HEALTHCHECK_TIMEOUT = 1000;
diff --git a/src/index.js b/src/index.js
index 7dedfc8..ea39251 100644
--- a/src/index.js
+++ b/src/index.js
@@ -30,15 +30,12 @@ import {
   SYMBOL_ITERATOR,
   TO_STRING_TAG,
   SC_PARAMS_TO_STRIP,
-  SC_READY_MESSAGE,
-  SC_CLOSE_MESSAGE,
-  SC_CLOSE_TIMEOUT,
   DEFAULT_SAUCE_CONNECT_VERSION,
-  SC_FAILURE_MESSAGES,
   SC_BOOLEAN_CLI_PARAMS,
   DEFAULT_RUNNER_NAME,
 } from './constants';
 import SauceConnectLoader from './sauceConnectLoader';
+import {SauceConnectManager} from './sauceConnectManager';
 
 export default class SauceLabs {
   constructor(options) {
@@ -249,16 +246,6 @@ export default class SauceLabs {
       );
     }
 
-    // Provide a default runner name. It's used for identifying the tunnel's initiation method.
-    if (!argv['metadata']) {
-      argv = {...argv, metadata: `runner=${DEFAULT_RUNNER_NAME}`};
-    } else if (!argv['metadata'].includes('runner=')) {
-      argv = {
-        ...argv,
-        metadata: `runner=${DEFAULT_RUNNER_NAME},${argv['metadata']}`,
-      };
-    }
-
     const scUpstreamProxy = argv.scUpstreamProxy;
     const args = Object.entries(argv)
       /**
@@ -269,6 +256,8 @@ export default class SauceLabs {
           ![
             '_',
             '$0',
+            'api-address',
+            'metadata',
             'sc-version',
             'sc-upstream-proxy',
             'tunnel-name',
@@ -289,6 +278,7 @@ export default class SauceLabs {
       );
     args.push(`--username=${this.username}`);
     args.push(`--access-key=${this._accessKey}`);
+
     if (scUpstreamProxy) {
       // map `--sc-upstream-proxy` to sc's `--proxy`. It's done because the app CLI
       // conflicts with sc's CLI, `--proxy` here is equivalent to `--proxy-sauce` in sc.
@@ -301,6 +291,18 @@ export default class SauceLabs {
       args.push(`--proxy-sauce=${this.proxy}`);
     }
 
+    // Provide a default runner name. It's used for identifying the tunnel's initiation method.
+    let metadata = argv.metadata || '';
+    if (!metadata.includes('runner=')) {
+      metadata = [metadata, `runner=${DEFAULT_RUNNER_NAME}`]
+        .filter(Boolean)
+        .join(',');
+    }
+    args.push(`--metadata=${metadata}`);
+
+    const apiAddress = argv.apiAddress || ':8032';
+    args.push(`--api-address=${apiAddress}`);
+
     const region = argv.region || this.region;
     if (region) {
       const scRegion = getRegionSubDomain({region});
@@ -337,60 +339,20 @@ export default class SauceLabs {
       args.unshift('run');
     }
 
+    const logger = fromCLI
+      ? process.stdout.write.bind(process.stdout)
+      : argv.logger;
     const cp = spawn(scLoader.path, args);
-    return new Promise((resolve, reject) => {
-      const close = () =>
-        new Promise((resolveClose) => {
-          process.kill(cp.pid, 'SIGINT');
-          const timeout = setTimeout(resolveClose, SC_CLOSE_TIMEOUT);
-          cp.stdout.on('data', (data) => {
-            const output = data.toString();
-            if (output.includes(SC_CLOSE_MESSAGE)) {
-              clearTimeout(timeout);
-              return resolveClose(returnObj);
-            }
-          });
-        });
-      const returnObj = {cp, close};
+    const manager = new SauceConnectManager(cp, logger);
+    process.on('SIGINT', () => manager.close());
 
-      cp.stderr.on('data', (data) => {
-        const output = data.toString();
-        return reject(new Error(output));
-      });
-      cp.stdout.on('data', (data) => {
-        const logger = fromCLI
-          ? process.stdout.write.bind(process.stdout)
-          : argv.logger;
-        const output = data.toString();
-        /**
-         * print to stdout if called via CLI
-         */
-        if (typeof logger === 'function') {
-          logger(output);
-        }
-
-        /**
-         * fail if SauceConnect could not establish a connection
-         */
-        if (
-          SC_FAILURE_MESSAGES.find((msg) =>
-            escape(output).includes(escape(msg))
-          )
-        ) {
-          return reject(new Error(output));
-        }
-
-        /**
-         * continue if connection was established
-         */
-        if (output.includes(SC_READY_MESSAGE)) {
-          return resolve(returnObj);
-        }
-      });
-
-      process.on('SIGINT', close);
-      return returnObj;
-    });
+    try {
+      await manager.waitForReady(apiAddress);
+      return {cp, close: () => manager.close()};
+    } catch (err) {
+      await manager.close();
+      throw err;
+    }
   }
 
   /**
diff --git a/src/sauceConnectHealthcheck.js b/src/sauceConnectHealthcheck.js
new file mode 100644
index 0000000..0c1d79a
--- /dev/null
+++ b/src/sauceConnectHealthcheck.js
@@ -0,0 +1,12 @@
+import {SC_HEALTHCHECK_TIMEOUT} from './constants';
+
+export default class SauceConnectHealthCheck {
+  async perform(apiAddress) {
+    const response = await fetch(`${apiAddress}/readyz`, {
+      signal: AbortSignal.timeout(SC_HEALTHCHECK_TIMEOUT),
+    });
+    if (response.status !== 200) {
+      throw new Error(`response status code ${response.status} != 200`);
+    }
+  }
+}
diff --git a/src/sauceConnectManager.js b/src/sauceConnectManager.js
new file mode 100644
index 0000000..f775f96
--- /dev/null
+++ b/src/sauceConnectManager.js
@@ -0,0 +1,94 @@
+import {
+  SC_CLOSE_TIMEOUT,
+  SC_HEALTHCHECK_TIMEOUT,
+  SC_READY_TIMEOUT,
+} from './constants';
+import SauceConnectHealthCheck from './sauceConnectHealthcheck';
+
+export class SauceConnectManager {
+  constructor(cp, logger) {
+    this.cp = cp;
+    this.logger = logger || (() => {});
+    this.healthcheck = new SauceConnectHealthCheck();
+    this._healthcheckInterval = null;
+    this._readyTimeout = null;
+  }
+
+  waitForReady(apiAddress) {
+    apiAddress = this._parseApiAddress(apiAddress);
+
+    return new Promise((resolve, reject) => {
+      this.cp.stderr.on('data', (data) => {
+        const output = data.toString();
+        return reject(new Error(output));
+      });
+
+      this.cp.stdout.on('data', (data) => {
+        const logger = this.logger;
+        if (typeof logger === 'function') {
+          logger(data.toString());
+        }
+      });
+
+      this._healthcheckInterval = setInterval(async () => {
+        if (this.cp.exitCode !== null && this.cp.exitCode !== undefined) {
+          clearInterval(this._healthcheckInterval);
+          clearTimeout(this._readyTimeout);
+          return reject(
+            new Error('Sauce Connect exited before reaching a ready state')
+          );
+        }
+
+        this.healthcheck
+          .perform(apiAddress)
+          .then(() => clearInterval(this._healthcheckInterval))
+          .then(() => clearTimeout(this._readyTimeout))
+          .then(resolve)
+          .catch(() => {});
+      }, SC_HEALTHCHECK_TIMEOUT);
+
+      this._readyTimeout = setTimeout(() => {
+        clearInterval(this._healthcheckInterval);
+        reject(new Error('Timeout waiting for healthcheck endpoint'));
+      }, SC_READY_TIMEOUT);
+    });
+  }
+
+  close() {
+    this.logger('Terminating Sauce Connect...');
+
+    return new Promise((resolve) => {
+      if (this.cp.exitCode !== null && this.cp.exitCode !== undefined) {
+        return resolve();
+      }
+
+      const timeout = setTimeout(() => {
+        this.logger('Forcefully terminating Sauce Connect...');
+        killSilently(this.cp.pid, 'SIGKILL');
+        resolve();
+      }, SC_CLOSE_TIMEOUT);
+
+      this.cp.on('exit', () => {
+        clearTimeout(timeout);
+        resolve();
+      });
+
+      clearInterval(this._healthcheckInterval);
+      clearTimeout(this._readyTimeout);
+      killSilently(this.cp.pid, 'SIGINT');
+    });
+  }
+
+  _parseApiAddress(value) {
+    const [host, port] = value.split(':');
+    return `http://${host || 'localhost'}:${port}`;
+  }
+}
+
+function killSilently(pid, signal) {
+  try {
+    process.kill(pid, signal);
+  } catch (err) {
+    // ignore
+  }
+}


Test patch:
diff --git a/e2e/sc.test.js b/e2e/sc.test.js
index 0dff093..3b351e4 100644
--- a/e2e/sc.test.js
+++ b/e2e/sc.test.js
@@ -45,7 +45,9 @@ test('should not be able to run Sauce Connect due to invalid credentials', async
       tunnelName: `node-saucelabs E2E test - ${ID}`,
     })
     .catch((err) => err);
-  expect(err.message).toContain('Not authorized');
+  expect(err.message).toContain(
+    'Sauce Connect exited before reaching a ready state'
+  );
 });
 
 test('should not be able to run Sauce Connect due to missing tunnel-name', async () => {
diff --git a/tests/__snapshots__/index.test.js.snap b/tests/__snapshots__/index.test.js.snap
index 7897bb3..b089835 100644
--- a/tests/__snapshots__/index.test.js.snap
+++ b/tests/__snapshots__/index.test.js.snap
@@ -161,11 +161,13 @@ exports[`startSauceConnect should start sauce connect with proper parsed args 1`
     [
       "run",
       "--proxy-tunnel=abc",
-      "--metadata=runner=example",
       "--verbose=true",
       "--username=foo",
       "--access-key=bar",
       "--proxy=http://example.com:8080",
+      "--proxy-sauce=http://example.com",
+      "--metadata=runner=example",
+      "--api-address=:8032",
       "--region=eu-central-1",
       "--tunnel-name=my-tunnel",
     ],
diff --git a/tests/index.test.js b/tests/index.test.js
index 24cfa13..79738e4 100644
--- a/tests/index.test.js
+++ b/tests/index.test.js
@@ -47,6 +47,21 @@ jest.mock('../src/sauceConnectLoader.js', () => {
   return SauceConnectLoaderMock;
 });
 
+jest.mock('../src/sauceConnectHealthcheck.js', () => {
+  class SauceConnectHealthcheckMock {
+    constructor() {
+      this.perform = jest.fn().mockResolvedValue(undefined);
+    }
+  }
+
+  return SauceConnectHealthcheckMock;
+});
+
+jest.mock('../src/constants.js', () => ({
+  ...jest.requireActual('../src/constants.js'),
+  SC_CLOSE_TIMEOUT: 10,
+}));
+
 const stdoutEmitter = spawn().stdout;
 const stderrEmitter = spawn().stderr;
 const origKill = process.kill;
@@ -55,6 +70,7 @@ beforeEach(() => {
   process.kill = jest.fn();
   // clean instances array
   instances.splice(0, instances.length);
+  // scHealthcheckPerformMock.mockRejectedValue(new Error("failure"))
 });
 test('should be inspectable', () => {
   const api = new SauceLabs({user: 'foo', key: 'bar'});
@@ -482,7 +498,11 @@ test('should contain expected windows download link', async () => {
 describe('startSauceConnect', () => {
   it('should start sauce connect with proper parsed args', async () => {
     const logs = [];
-    const api = new SauceLabs({user: 'foo', key: 'bar'});
+    const api = new SauceLabs({
+      user: 'foo',
+      key: 'bar',
+      proxy: 'http://example.com',
+    });
     setTimeout(
       () =>
         stdoutEmitter.emit(
@@ -572,14 +592,6 @@ describe('startSauceConnect', () => {
         },
       })
     );
-    setTimeout(
-      () =>
-        stdoutEmitter.emit(
-          'data',
-          'Sauce Connect is up, you may start your tests'
-        ),
-      50
-    );
     await api.startSauceConnect({
       tunnelName: 'my-tunnel',
       'proxy-tunnel': 'abc',
@@ -587,35 +599,9 @@ describe('startSauceConnect', () => {
     });
   });
 
-  it('should properly fail on fatal error', async () => {
-    const errMessage = 'fatal error exiting: any error message';
-    const api = new SauceLabs({user: 'foo', key: 'bar'});
-    setTimeout(() => stdoutEmitter.emit('data', errMessage), 50);
-    const err = await api
-      .startSauceConnect({
-        scVersion: '1.2.3',
-        tunnelName: 'my-tunnel',
-        'proxy-tunnel': 'abc',
-      })
-      .catch((err) => err);
-    expect(err.message).toBe(errMessage);
-  });
-
   it('should close sauce connect', async () => {
     const api = new SauceLabs({user: 'foo', key: 'bar'});
-    setTimeout(
-      () =>
-        stdoutEmitter.emit(
-          'data',
-          'Sauce Connect is up, you may start your tests'
-        ),
-      50
-    );
     const sc = await api.startSauceConnect({tunnelName: 'my-tunnel'}, true);
-    setTimeout(() => {
-      sc.cp.stdout.emit('data', 'Some other message');
-      sc.cp.stdout.emit('data', 'tunnel was shutdown');
-    }, 100);
     await sc.close();
     expect(process.kill).toBeCalledWith(123, 'SIGINT');
   });
diff --git a/tests/sauceConnectHealthcheck.test.js b/tests/sauceConnectHealthcheck.test.js
new file mode 100644
index 0000000..536e641
--- /dev/null
+++ b/tests/sauceConnectHealthcheck.test.js
@@ -0,0 +1,30 @@
+import SauceConnectHealthCheck from '../src/sauceConnectHealthcheck';
+
+const fetchMock = jest.fn();
+global.fetch = fetchMock;
+
+describe('SauceConnectHealthcheck', () => {
+  beforeEach(() => {
+    fetchMock.mockClear();
+  });
+
+  describe('perform', () => {
+    test('raises error if response code != 200', async () => {
+      fetchMock.mockImplementation(async () => ({
+        status: 503,
+      }));
+      const healthcheck = new SauceConnectHealthCheck();
+      const error = await healthcheck.perform(':8042').catch((err) => err);
+      expect(error.message).toBe('response status code 503 != 200');
+    });
+
+    test('all good when response code == 200', async () => {
+      fetchMock.mockImplementation(async () => ({
+        status: 200,
+      }));
+      const healthcheck = new SauceConnectHealthCheck();
+      const result = await healthcheck.perform(':8042');
+      expect(result).toBe(undefined);
+    });
+  });
+});
diff --git a/tests/sauceConnectManager.test.js b/tests/sauceConnectManager.test.js
new file mode 100644
index 0000000..158bb61
--- /dev/null
+++ b/tests/sauceConnectManager.test.js
@@ -0,0 +1,69 @@
+import {SauceConnectManager} from '../src/sauceConnectManager';
+import * as constants from '../src/constants';
+
+describe('SauceConnectManager', () => {
+  describe('waitForReady', () => {
+    test('process terminated with exitCode', async () => {
+      const manager = new SauceConnectManager({
+        exitCode: 1,
+        stderr: {
+          on: jest.fn(),
+        },
+        stdout: {
+          on: jest.fn(),
+        },
+      });
+      const error = await manager.waitForReady(':8042').catch((err) => err);
+      expect(error.message).toBe(
+        'Sauce Connect exited before reaching a ready state'
+      );
+    });
+
+    describe('waiting too long for healthcheck', () => {
+      let origTimeout = constants.SC_READY_TIMEOUT;
+      beforeEach(() => {
+        // eslint-disable-next-line no-import-assign
+        Object.defineProperty(constants, 'SC_READY_TIMEOUT', {value: 10});
+      });
+
+      test('waiting too long for healthcheck', async () => {
+        const manager = new SauceConnectManager({
+          stderr: {
+            on: jest.fn(),
+          },
+          stdout: {
+            on: jest.fn(),
+          },
+        });
+        const error = await manager.waitForReady(':8042').catch((err) => err);
+        expect(error.message).toBe('Timeout waiting for healthcheck endpoint');
+      });
+
+      afterEach(() => {
+        // eslint-disable-next-line no-import-assign
+        Object.defineProperty(constants, 'SC_READY_TIMEOUT', {
+          value: origTimeout,
+        });
+      });
+    });
+  });
+
+  describe('close', () => {
+    test('process terminated with exitCode', async () => {
+      const manager = new SauceConnectManager({
+        exitCode: 1,
+      });
+      const result = await manager.close();
+      expect(result).toBe(undefined);
+    });
+
+    test('reacts to process exit event', async () => {
+      const manager = new SauceConnectManager({
+        pid: 123,
+        on: (event, callback) => setTimeout(callback, 10),
+      });
+
+      await manager.close();
+    });
+  });
+});