squarebracket / karma-selenium-grid-launcher

Launcher for Remote WebDriver instances on a Selenium Grid; forked from karma-webdriver-launcher.
MIT License
1 stars 10 forks source link

Fix promise resolve function name #2

Closed mcpengelly closed 5 years ago

mcpengelly commented 5 years ago

Error Message:

01 08 2019 13:38:28.765:INFO [SeleniumGrid]: Killed chrome-mac1011.
(node:84022) UnhandledPromiseRejectionWarning: ReferenceError: resolve is not defined
    at _stopSession.then (/Users/mapengel/Documents/workspace/Kandy.js/node_modules/karma-selenium-grid-launcher/index.js:417:11)

Issue: Had an issue where one of my selenium grid launcher tests was crashing because stopSession was looking for resolve() but it was undefined. Based on the code and the error message we can clearly see that the resolve() function is not in scope and is improperly named likely due to a refactor.

Resolution: Call the correct resolve function instead.

mcpengelly commented 5 years ago

I dont have permission to merge this myself :(

squarebracket commented 5 years ago

Sorry about taking so long to respond, the notification email must have slipped by me.

What versions of karma and node? I'm pretty sure I tested that resolve would be in scope, but maybe there's different scoping happening somehow.

The idea was that the function for this.on('kill') would return a Promise that would resolve. That's separate from resolving (or rejecting, in this case) startPromise, which this does.

I thought arrow functions inherit the scope from where they're called? In this case, the resolve I'm trying to reference is this one here.

In any case, this PR wouldn't resolve that promise.

Does the following patch fix this issue for you?

diff --git i/index.js w/index.js
index 049cce2..acd6d0a 100644
--- i/index.js
+++ w/index.js
@@ -410,15 +410,6 @@ var SeleniumGridInstance = function (name, args, logger, baseLauncherDecorator,
         done();
       }
     };
-    const stopSession = (err) => {
-      return new Promise((startPromiseResolve, startPromiseReject) => {
-        this._stopSession(end, err).then(() => {
-          clearInterval(killInterval);
-          resolve('shutting down');
-          startPromiseReject('shutting down');
-        });
-      });
-    };

     if (!self.browser) {
         log.info('Browser ' + self.name + ' has not yet launched.');
@@ -433,6 +424,15 @@ var SeleniumGridInstance = function (name, args, logger, baseLauncherDecorator,
     }, 10000);

     return new Promise((resolve, reject) => {
+      const stopSession = (err) => {
+        return new Promise((startPromiseResolve, startPromiseReject) => {
+          this._stopSession(end, err).then(() => {
+            clearInterval(killInterval);
+            resolve('shutting down');
+            startPromiseReject('shutting down');
+          });
+        });
+      };
       startPromise = startPromise.then(stopSession, stopSession);
     });
   });
mcpengelly commented 5 years ago

Not sure on the versions, Dont have access to the codebase i was on anymore either. The code was throwing before for the reasons described. It looks like your logic would work. im happy to close this PR.

mcpengelly commented 5 years ago

closing. Solving the wrong issue