Repository: saucelabs/node-saucelabs
Pull request #140 (2022-05-06 00:09:34)

SC default version update
### Description
- Upgrade the default SC version to 4.8.0
- Clean up SC API (removing SC options that are either outdated or scheduled for deprecation).
- Fix tests flakiness by adding `tunnelPool` to e2e tests to prevent concurrent tests to interfere with each other by shutting down "colliding" tunnels. This works as long as e2e tests don't run selenium tests with those tunnels. We'd need to have unique tunnel names if we add selenium e2e tests in order to ensure that each test uses its own tunnel.
- removing the translation of `region-name-1` to `region-name` for SC since SC supports either.

### Motivation and Context

AFAIK, SauceConnect developers contributed to this repo only to update tunnels REST API endpoints. They never contributed to the SauceConnect node wrapper.  Starting with 4.8.0 release, we'd try to add this client to our test suite and have our new features tested here as well. One instance of lack of collaboration is SauceConnect CLI deprecation message that writes to stderr (which is just how the framework does it). We simply didn't know that it interferes with this client, otherwise, we could have figured out another way.  Options like ,`--rest-url`,  are being deprecated but it's a long process and, for some use cases (like when only one API URL is whitelisted), it's the best possible option right now, until the global Sauce Labs API exists.

Moreover, SC CLI is just a user-facing tool that fronts a fairly complex backend system.  The release lifecycle of the tool and a fair number of backend services is, sometimes, difficult to sync up. 
Some CLI options, even if present, only make sense with the backend support.  Options like `--no-proxy-caching` are not well-designed because they modify the backend behavior and not the CLI tool behavior and therefore should be updated when the backend changes (unlike options like `--proxy` that modify the user-side behavior and are guaranteed to be consistent across the releases). As an example,  caching is no longer supported due to Squid Proxy changes that are beyond our control but, since some users have `--no-proxy-caching` in their CI scripts, SC CLI flag wasn't removed even though it doesn't do anything meaningful). We are trying our best to reconcile the backend releases with SC CLI releases but, due to some technical reasons, in some instances, some options are out of sync with the backend, we are trying to change that.

Reviewing the API being used here, I noticed that it's way out of line with the Sauce Connect team-supported official CLI published [here](https://docs.saucelabs.com/dev/cli/sauce-connect-proxy/index.html). This PR will attempt to bring this client up to speed.

boneskull: > This works as long as e2e tests don't run selenium tests with those tunnels

Can you explain more here?
boneskull: > cleaning up API (removing SC options that can't be used in this context).

What is the context you mean, exactly? 😄 
waggledans: > What is the context you mean, exactly?

I should prob change the wording. I guess, what I'm trying to do, @boneskull is to not have `c&p` of SC CLI from 3 years ago. I'm removing (or, per your suggestion marking them deprecated) obscure flags that don't have any usage and are scheduled to be removed in SC-5.x.   Like `max-missed-acks`.  Or `doctor`. Or `scproxy-read-limit`.
boneskull: @waggledans Thanks.  BTW, I reformatted the codebase, so you'll need to rebase (and run `npm install`).  
waggledans: Tnx for the review @boneskull. 
I think it's enough changes for one PR, could please take another look.
boneskull: lgtm

Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 16a63d0..790ffb2 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -80,6 +80,14 @@ $ npm run watch
 
 ### Test Project
 
+To run e2e tests, run:
+
+```sh
+$ export SAUCE_API_KEY=<YOUR KEY>
+$ export SAUCE_USERNAME=<YOUR USER>
+$ npm run test:e2e
+```
+
 To test the project, run:
 
 ```sh
diff --git a/src/constants.js b/src/constants.js
index 5c15dcd..92b221f 100644
--- a/src/constants.js
+++ b/src/constants.js
@@ -3,7 +3,7 @@ import os from 'os';
 
 import {version} from '../package.json';
 
-export const DEFAULT_SAUCE_CONNECT_VERSION = '4.7.1';
+export const DEFAULT_SAUCE_CONNECT_VERSION = '4.8.0';
 export const SAUCE_CONNECT_BASE = 'https://saucelabs.com/downloads';
 export const SAUCE_CONNECT_VERSIONS_ENDPOINT =
   'https://saucelabs.com/versions.json';
@@ -184,12 +184,12 @@ export const SAUCE_CONNECT_CLI_PARAMS = [
     name: 'capath',
     description:
       'Directory of CA certs to use for verifying REST connections. (default "/etc/ssl/certs")',
+    deprecated: true,
   },
   {
     alias: 'c',
     name: 'config-file',
-    description:
-      'Path to YAML config file. Please refer to https://wiki.saucelabs.com/display/DOCS/Sauce+Connect+Command+Line+Reference for a sample configuration file.',
+    description: 'Path to YAML config file.',
   },
   {
     alias: 'D',
@@ -199,14 +199,14 @@ export const SAUCE_CONNECT_CLI_PARAMS = [
   },
   {
     name: 'dns',
-    description:
-      'Use specified name server. To specify multiple servers, separate them with comma. Use IP addresses, optionally with a port number, the two separated by a colon. Example: --dns 8.8.8.8,8.8.4.4:53',
+    description: 'Use specified name server(s). Example: --dns 8.8.8.8,8.8.4.4:53',
   },
   {
     name: 'doctor',
     description:
       'Perform checks to detect possible misconfiguration or problems.',
     type: 'boolean',
+    deprecated: true,
   },
   {
     alias: 'F',
@@ -219,6 +219,7 @@ export const SAUCE_CONNECT_CLI_PARAMS = [
     name: 'log-stats',
     description: 'Log statistics about HTTP traffic every <seconds>.',
     type: 'number',
+    deprecated: true,
   },
   {
     alias: 'l',
@@ -234,15 +235,18 @@ export const SAUCE_CONNECT_CLI_PARAMS = [
   {
     alias: 'M',
     name: 'max-missed-acks',
-    description:
-      'The maximum amount of keepalive acks that can be missed before the client will trigger a reconnect. (default 30)',
+    description: 'The max number of keepalive acks that can be missed before triggering reconnect.',
     type: 'number',
-    default: 30,
+    deprecated: true,
   },
   {
     name: 'metrics-address',
-    description:
-      'host:port for the internal web server used to expose client side metrics. (default "localhost:8888")',
+    description: 'host:port server used to expose client-side metrics.',
+    deprecated: true,
+  },
+  {
+    name: 'status-address',
+    description: 'host:port server used to expose client status.',
   },
   {
     name: 'no-autodetect',
@@ -306,11 +310,13 @@ export const SAUCE_CONNECT_CLI_PARAMS = [
     name: 'scproxy-read-limit',
     description:
       'Rate limit reads in scproxy to X bytes per second. This option can be used to adjust local network transfer rate in order not to overload the tunnel connection.',
+    deprecated: true,
   },
   {
     name: 'scproxy-write-limit',
     description:
       'Rate limit writes in scproxy to X bytes per second. This option can be used to adjust local network transfer rate in order not to overload the tunnel connection.',
+    deprecated: true,
   },
   {
     alias: 'P',
@@ -336,6 +342,7 @@ export const SAUCE_CONNECT_CLI_PARAMS = [
     name: 'tunnel-capath',
     description:
       'Directory of CA certs to use for verifying tunnel connections. (default "/etc/ssl/certs")',
+    deprecated: true,
   },
   {
     name: 'tunnel-cert',
@@ -350,14 +357,13 @@ export const SAUCE_CONNECT_CLI_PARAMS = [
   },
   {
     name: 'tunnel-identifier',
-    description:
-      "[Depcrecated] Don't automatically assign jobs to this tunnel. Jobs will use it only by explicitly providing the right identifier.",
+    description: 'Tunnel name used for this tunnel or the tunnels in the same HA pool.',
+    deprecated: true,
   },
   {
     alias: 'i',
     name: 'tunnel-name',
-    description:
-      "Don't automatically assign jobs to this tunnel. Jobs will use it only by explicitly providing the right identifier.",
+    description: 'Tunnel name used for this tunnel or the tunnels in the same HA pool.',
   },
   {
     name: 'tunnel-pool',
diff --git a/src/index.js b/src/index.js
index 2d0642c..c1461f9 100644
--- a/src/index.js
+++ b/src/index.js
@@ -279,10 +279,7 @@ export default class SauceLabs {
 
     const region = argv.region || this.region;
     if (region) {
-      const scRegion = getRegionSubDomain({region})
-        .split('-')
-        .slice(0, 2)
-        .join('-');
+      const scRegion = getRegionSubDomain({region});
       args.push(`--region=${scRegion}`);
     }
     const scLoader = new SauceConnectLoader({sauceConnectVersion});


Test patch:
diff --git a/e2e/sc.test.js b/e2e/sc.test.js
index e899382..e21d19d 100644
--- a/e2e/sc.test.js
+++ b/e2e/sc.test.js
@@ -29,6 +29,8 @@ test('should be able to run Sauce Connect', async () => {
   const api = new SauceLabs();
   const sc = await api.startSauceConnect({
     logger: console.log.bind(console),
+    // tunnelPool is set here in order for the concurrent tests to not shut down "colliding" tunnels.
+    tunnelPool: true,
     tunnelName: `node-saucelabs E2E test - ${ID}`,
   });
   console.log('Sauce Connect started successfully, shutting down...');
diff --git a/tests/__snapshots__/index.test.js.snap b/tests/__snapshots__/index.test.js.snap
index 9b5210e..a7da1e4 100644
--- a/tests/__snapshots__/index.test.js.snap
+++ b/tests/__snapshots__/index.test.js.snap
@@ -138,7 +138,7 @@ Array [
       "--tunnel-name=my-tunnel",
       "--user=foo",
       "--api-key=bar",
-      "--region=us-west",
+      "--region=us-west-1",
     ],
   ],
 ]
@@ -152,10 +152,9 @@ Array [
       "--proxy-tunnel",
       "--verbose",
       "--tunnel-name=my-tunnel",
-      "--no-proxy-caching",
       "--user=foo",
       "--api-key=bar",
-      "--region=eu-central",
+      "--region=eu-central-1",
     ],
   ],
 ]
diff --git a/tests/index.test.js b/tests/index.test.js
index ea7ebf5..60aa0c3 100644
--- a/tests/index.test.js
+++ b/tests/index.test.js
@@ -476,7 +476,6 @@ describe('startSauceConnect', () => {
       'proxy-tunnel': 'abc',
       verbose: true,
       region: 'eu',
-      noProxyCaching: true,
       logger: (log) => logs.push(log),
     });
     expect(spawn).toBeCalledTimes(1);