fix: retry finding port when port is null and get ports in sequence (#1993)

Author: evilebottnawi

Diff for: lib/utils/findPort.js

@@ -1,12 +1,26 @@
 'use strict';
 
-const { getPortPromise } = require('portfinder');
+const pRetry = require('p-retry');
+const portfinder = require('portfinder');
 const defaultPort = require('./defaultPort');
 const defaultTo = require('./defaultTo');
 const tryParseInt = require('./tryParseInt');
 
+function runPortFinder() {
+  return new Promise((resolve, reject) => {
+    portfinder.basePort = defaultPort;
+    portfinder.getPort((error, port) => {
+      if (error) {
+        return reject(error);
+      }
+
+      return resolve(port);
+    });
+  });
+}
+
 function findPort(port) {
-  if (typeof port !== 'undefined') {
+  if (port) {
     return Promise.resolve(port);
   }
 
@@ -19,10 +33,7 @@ function findPort(port) {
     3
   );
 
-  return getPortPromise({
-    port: defaultPort,
-    stopPort: defaultPort + defaultPortRetry,
-  });
+  return pRetry(runPortFinder, { retries: defaultPortRetry });
 }
 
 module.exports = findPort;

Diff for: package-lock.json

@@ -51,6 +51,7 @@
     "killable": "^1.0.1",
     "loglevel": "^1.6.2",
     "opn": "^5.5.0",
+    "p-retry": "^3.0.1",
     "portfinder": "^1.0.20",
     "schema-utils": "^1.0.0",
     "selfsigned": "^1.10.4",

Diff for: package.json

@@ -1,3 +1,3 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`findPort util should throws the error when the port isn't found 1`] = `"No open ports found in between 8080 and 8085"`;
+exports[`findPort util should throws the error when the port isn't found 1`] = `"busy"`;

Diff for: test/server/utils/__snapshots__/findPort.test.js.snap

@@ -1,6 +1,7 @@
 'use strict';
 
 const http = require('http');
+const portfinder = require('portfinder');
 const findPort = require('../../../lib/utils/findPort');
 
 describe('findPort util', () => {
@@ -23,7 +24,7 @@ describe('findPort util', () => {
   });
 
   function createDummyServers(n) {
-    return [...new Array(n)].reduce((p, _, i) => {
+    return (Array.isArray(n) ? n : [...new Array(n)]).reduce((p, _, i) => {
       return p.then(() => {
         return new Promise((resolve) => {
           const server = http.createServer();
@@ -42,25 +43,88 @@ describe('findPort util', () => {
     });
   });
 
-  it('should retry finding the port for up to defaultPortRetry times', () => {
-    const retryCount = 5;
+  it.only('should returns the port when the port is null', () => {
+    const retryCount = 2;
+
+    process.env.DEFAULT_PORT_RETRY = 2;
+
+    return createDummyServers(retryCount)
+      .then(() => findPort(null))
+      .then((port) => {
+        expect(port).toEqual(8080 + retryCount);
+      });
+  });
+
+  it('should returns the port when the port is undefined', () => {
+    const retryCount = 2;
+
+    process.env.DEFAULT_PORT_RETRY = 2;
+
+    return (
+      createDummyServers(retryCount)
+        // eslint-disable-next-line no-undefined
+        .then(() => findPort(undefined))
+        .then((port) => {
+          expect(port).toEqual(8080 + retryCount);
+        })
+    );
+  });
+
+  it('should retry finding the port for up to defaultPortRetry times (number)', () => {
+    const retryCount = 3;
 
     process.env.DEFAULT_PORT_RETRY = retryCount;
 
     return createDummyServers(retryCount)
-      .then(findPort)
+      .then(() => findPort())
       .then((port) => {
         expect(port).toEqual(8080 + retryCount);
       });
   });
 
+  it('should retry finding the port for up to defaultPortRetry times (string)', () => {
+    const retryCount = 3;
+
+    process.env.DEFAULT_PORT_RETRY = `${retryCount}`;
+
+    return createDummyServers(retryCount)
+      .then(() => findPort())
+      .then((port) => {
+        expect(port).toEqual(8080 + retryCount);
+      });
+  });
+
+  it('should retry finding the port when serial ports are busy', () => {
+    const busyPorts = [8080, 8081, 8082, 8083];
+
+    process.env.DEFAULT_PORT_RETRY = 3;
+
+    return createDummyServers(busyPorts)
+      .then(() => findPort())
+      .then((port) => {
+        expect(port).toEqual(8080 + busyPorts.length);
+      });
+  });
+
   it("should throws the error when the port isn't found", () => {
-    process.env.DEFAULT_PORT_RETRY = 5;
+    expect.assertions(1);
+
+    const spy = jest
+      .spyOn(portfinder, 'getPort')
+      .mockImplementation((callback) => {
+        return callback(new Error('busy'));
+      });
 
-    return createDummyServers(10)
-      .then(findPort)
+    const retryCount = 1;
+
+    process.env.DEFAULT_PORT_RETRY = 0;
+
+    return createDummyServers(retryCount)
+      .then(() => findPort())
       .catch((err) => {
         expect(err.message).toMatchSnapshot();
+
+        spy.mockRestore();
       });
   });
 });