Fixed #2458 - correctly handling 5xx internal server errors from remo…

…te selenium/webdriver services.
nightwatchjs · Jul 25, 2020 · 4fdf71b · 4fdf71b
1 parent 2eed9cf
commit 4fdf71b
Show file tree

Hide file tree

Showing 12 changed files with 242 additions and 29 deletions.
diff --git a/lib/api/element-commands/_waitFor.js b/lib/api/element-commands/_waitFor.js
@@ -162,7 +162,9 @@ class WaitForElement extends ElementCommand {
     return this.assert({
       response,
       passed: true,
-      err: {}
+      err: {
+        expected: this.expectedValue
+      }
     });
   }
 
@@ -173,7 +175,8 @@ class WaitForElement extends ElementCommand {
       response,
       passed: false,
       err: {
-        actual, expected
+        actual,
+        expected
       }
     });
   }
@@ -182,23 +185,30 @@ class WaitForElement extends ElementCommand {
     this.elapsedTime = this.executor.elapsedTime;
 
     const {reporter} = this.client;
+    const {result} = response;
     const {elapsedTime, message, abortOnFailure, stackTrace} = this;
     const runner = new AssertionRunner({
-      passed, err, message, abortOnFailure, stackTrace, reporter, elapsedTime
+      passed,
+      err,
+      message,
+      abortOnFailure,
+      stackTrace,
+      reporter,
+      elapsedTime
     });
 
-    return runner.run()
+    return runner.run(result)
       .catch(err => (err))
       .then(err => {
         if (err instanceof Error) {
           err.abortOnFailure = this.abortOnFailure;
 
-          return this.complete(err, response.result);
+          return this.complete(err, result);
         }
 
         // FIXME
         // Due to backwards compatibility reasons, we keep here the simplified result object
-        return this.complete(null, response.result);
+        return this.complete(null, result);
       });
   }
 

diff --git a/lib/assertion/assertion-runner.js b/lib/assertion/assertion-runner.js
@@ -1,6 +1,16 @@
 const NightwatchAssertion = require('./assertion.js');
 
 module.exports = class AssertionRunner {
+  static isServerError(result = {}) {
+    if (!result) {
+      return false;
+    }
+
+    const {status, error, httpStatusCode} = result;
+
+    return (status === -1 && error && httpStatusCode && httpStatusCode.toString().startsWith('5'));
+  }
+
   /**
    * @param opts = {passed, err, calleeFn, message, stackTrace, abortOnFailure, reporter, addExpected = true}
    */
@@ -27,6 +37,12 @@ module.exports = class AssertionRunner {
     let assertResult;
     let isError = false;
 
+    if (commandResult && AssertionRunner.isServerError(commandResult)) {
+      const errorMessage = commandResult.error || 'Unknown server error';
+      this.assertion.passed = false;
+      this.assertion.actual = `Server Error: ${errorMessage}`;
+    }
+
     try {
       await this.assertion.assert();
       reporter.registerPassed(this.assertion.message);

diff --git a/lib/http/response.js b/lib/http/response.js
@@ -1,7 +1,7 @@
 const EventEmitter = require('events');
 const HttpUtil = require('./http.js');
 const Formatter = require('./formatter.js');
-const {Logger} = require('../utils');
+const Utils = require('../utils');
 
 class HttpResponse extends EventEmitter {
 
@@ -71,26 +71,35 @@ class HttpResponse extends EventEmitter {
     });
   }
 
-  isResponseJson(data) {
+  isResponseJson() {
     return this.res.headers['content-type'] && this.res.headers['content-type'].indexOf('application/json') > -1;
   }
 
+  isServerError() {
+    return this.res.statusCode.toString().startsWith('5')
+  }
+
   parseResponseData(data) {
     let result;
+    data = data.toString();
     data = Formatter.stripUnknownChars(data);
 
-    if (data && !this.isResponseJson(data)) {
-      return {
-        status: -1,
-        value: data
-      };
-    }
-
     try {
       result = JSON.parse(data);
     } catch (err) {
-      console.error(Logger.colors.red('Error processing the server response:'), '\n', data);
-      result = {status: -1, value: err.message};
+      result = data;
+    }
+
+    // sometimes the server non-json error responses end up parsed as strings and the error in undetected
+    if (Utils.isString(result) && this.isServerError()) {
+      return {
+        status: -1,
+        ['[[isNightwatch]]']: true,
+        value: {
+          error: 'internal server error',
+          message: result
+        }
+      };
     }
 
     return result;

diff --git a/lib/transport/jsonwire.js b/lib/transport/jsonwire.js
@@ -83,6 +83,16 @@ class JsonWireProtocol extends Transport {
   }
 
   formatCommandResponseData(result) {
+    if (!result.value || typeof result.value != 'object') {
+      return;
+    }
+
+    if (result['[[isNightwatch]]']) {
+      delete result['[[isNightwatch]]'];
+
+      return;
+    }
+
     if (result.class) {
       delete result.class;
     }
@@ -91,10 +101,6 @@ class JsonWireProtocol extends Transport {
       delete result.hCode;
     }
 
-    if (!result.value || typeof result.value != 'object') {
-      return;
-    }
-
     let errorDetails = null;
 
     if (result.value.screen) {
@@ -156,6 +162,14 @@ class JsonWireProtocol extends Transport {
       errorMessage += ` ${response.statusCode} ${response.statusMessage}`;
     }
 
+    if (response && response.headers && (response.headers['content-type'] === 'text/html' || response.headers['content-type'] === 'text/plain')) {
+      errorMessage = errorMessage.replace(/(<([^>]+)>)/gi, ''); // strip html tags
+    }
+
+    if (errorMessage.length > 50) {
+      errorMessage = errorMessage.substr(0, 50) + '...';
+    }
+
     return {
       status: -1,
       state: result.state || '',

diff --git a/lib/transport/webdriver.js b/lib/transport/webdriver.js
@@ -53,6 +53,8 @@ class WebdriverProtocol extends Transport {
       errorMessage = result.value.message;
     } else if (result.value && result.value.error && this.Errors.Response[result.value.error]) {
       errorMessage = this.Errors.Response[result.value.error].message;
+    } else if (result.error) {
+      errorMessage = result.error;
     }
 
     return {

diff --git a/test/lib/mockserver.js b/test/lib/mockserver.js
@@ -11,7 +11,7 @@ class MockServer {
       postdata: '',
       weakURLVerification: false,
       responseHeaders: {},
-      responseType: 'application/json',
+      contentType: 'application/json',
       mocks: [],
       finishedCallback() {}
     };
@@ -56,7 +56,7 @@ class MockServer {
         if (item) {
           headers = item.responseHeaders;
           responsedata = JSON.stringify(item.response);
-          headers['Content-Type']   = this.options.responseType;
+          headers['Content-Type']   = item.contentType || this.options.contentType;
           headers['Content-Length'] = responsedata.length;
           res.writeHead(Number(item.statusCode), headers);
 
@@ -98,9 +98,22 @@ class MockServer {
       item.__once = true;
     }
 
+    // if (item.response && typeof item.response == 'string') {
+    //   item.response = JSON.parse(item.response);
+    // }
+
     if (item.response && typeof item.response == 'string') {
-      item.response = JSON.parse(item.response);
+      item.contentType = item.contentType || 'application/json';
+
+      if (item.contentType === 'application/json') {
+        try {
+          item.response = JSON.parse(item.response);
+        } catch (err) {
+          console.warn('Invalid json supplied as response:', item.response);
+        }
+      }
     }
+
     this.mocks.push(item);
   }
 

diff --git a/test/sampletests/withservererrors/sampleTestWithServerError.js b/test/sampletests/withservererrors/sampleTestWithServerError.js
@@ -0,0 +1,19 @@
+module.exports = {
+  demoTest(client) {
+    client
+      .url('http://localhost')
+      .waitForElementPresent('#weblogin')
+      .assert.not.elementPresent('#element-server-error')
+  },
+
+  demoTest2(client) {
+    client
+      .url('http://localhost')
+      .waitForElementPresent('#weblogin')
+      .waitForElementNotPresent('#element-server-error')
+  },
+
+  after(client) {
+    client.end();
+  }
+};
diff --git a/test/src/element/testElementCommands.js b/test/src/element/testElementCommands.js
@@ -126,6 +126,48 @@ describe('element base commands', function() {
     return Nightwatch.start();
   });
 
+
+  it('client.element() with 502 gateway error', async function() {
+    Nightwatch.addMock({
+      url : '/session/13521-10219-202/element',
+      postdata: {
+        using: 'css selector',
+        value: '#weblogin-error'
+      },
+      contentType: 'text/plain',
+      statusCode: 502,
+      response: `<html>
+<head>
+<title>502 Bad Gateway</title>
+</head>
+<body></body>
+</html>`
+    }, true);
+
+    await Nightwatch.initAsync({
+      output: false,
+      silent: false,
+      selenium : {
+        start_process: false,
+      },
+      webdriver:{
+        start_process: true
+      },
+    });
+
+    Nightwatch.api().element('css selector', '#weblogin-error', function(result) {
+      assert.strictEqual(result.status, -1);
+      assert.strictEqual(result.httpStatusCode, 502);
+      assert.strictEqual(result.error, '<html>\n<head>\n<title>502 Bad Gateway</title>\n</head>\n<body></body>\n</html>');
+      assert.deepStrictEqual(result.value, {
+        error: 'internal server error',
+        message: '<html>\n<head>\n<title>502 Bad Gateway</title>\n</head>\n<body></body>\n</html>'
+      });
+    });
+
+    return Nightwatch.start();
+  });
+
   //////////////////////////////////////////////////////////////////////////////////////
   // .elements()
   //////////////////////////////////////////////////////////////////////////////////////

diff --git a/test/src/index/testNightwatchIndex.js b/test/src/index/testNightwatchIndex.js
@@ -202,7 +202,10 @@ describe('test NightwatchIndex', function () {
     client.startSession().catch(err => {
       assert.ok(err instanceof Error);
       assert.equal(typeof err.data, 'string');
-      assert.deepEqual(JSON.parse(err.data), {message: 'Could not find device : iPhone 6', error: []});
+      assert.deepEqual(JSON.parse(err.data), {
+        message: 'Could not find device : iPhone 6',
+        error: []
+      });
       assert.ok(err.message.indexOf('Could not find device : iPhone 6') > 0);
       done();
     }).catch(err => done(err));

diff --git a/test/src/runner/cli/testParallelExecution.js b/test/src/runner/cli/testParallelExecution.js
@@ -94,7 +94,7 @@ describe('test Parallel Execution', function() {
     assert.ok(runner.test_settings.test_workers);
 
     return runner.runTests().then(_ => {
-      assert.strictEqual(allArgs.length, 43);
+      assert.strictEqual(allArgs.length, 44);
       assert.strictEqual(runner.concurrency.globalExitCode, 0);
     });
   });
@@ -136,7 +136,7 @@ describe('test Parallel Execution', function() {
     });
 
     return runner.runTests().then(_ => {
-      assert.strictEqual(allArgs.length, 43);
+      assert.strictEqual(allArgs.length, 44);
     });
   });
 
@@ -165,7 +165,7 @@ describe('test Parallel Execution', function() {
     });
 
     return runner.runTests().then(_ => {
-      assert.strictEqual(allArgs.length, 43);
+      assert.strictEqual(allArgs.length, 44);
     });
   });