Properly handle responses in WebDriver.attachToSession

Fixes #2017

Author: jleyba

javascript/node/selenium-webdriver/CHANGES.md

@@ -7,6 +7,8 @@
    Sorry for the churn!
 * FIXED: capabilities serialization now properly handles undefined vs.
    false-like values.
+* FIXED: properly handle responses from the remote end in
+   `WebDriver.attachToSession`
 
 ## v2.53.1
 

javascript/node/selenium-webdriver/http/index.js

@@ -471,15 +471,19 @@ class Executor {
       let parsed =
           parseHttpResponse(/** @type {!HttpResponse} */ (response), this.w3c);
 
-      if (command.getName() === cmd.Name.NEW_SESSION) {
+      if (command.getName() === cmd.Name.NEW_SESSION
+          || command.getName() === cmd.Name.DESCRIBE_SESSION) {
         if (!parsed || !parsed['sessionId']) {
           throw new error.WebDriverError(
               'Unable to parse new session response: ' + response.body);
         }
 
         // The remote end is a W3C compliant server if there is no `status`
-        // field in the response.
-        this.w3c = this.w3c || !('status' in parsed);
+        // field in the response. This is not appliable for the DESCRIBE_SESSION
+        // command, which is not defined in the W3C spec.
+        if (command.getName() === cmd.Name.NEW_SESSION) {
+          this.w3c = this.w3c || !('status' in parsed);
+        }
 
         return new Session(parsed['sessionId'], parsed['value']);
       }

javascript/node/selenium-webdriver/lib/webdriver.js

@@ -87,30 +87,6 @@ class WebElementCondition extends Condition {
 //////////////////////////////////////////////////////////////////////////////
 
 
-/**
- * Sends a command to the server that is expected to return the details for a
- * {@link Session}. This may either be an existing session, or a newly created
- * one.
- *
- * @param {!command.Executor} executor Command executor to use when
- *     querying for session details.
- * @param {!command.Command} command The command to send to fetch the session
- *     details.
- * @param {string} description A descriptive debug label for this action.
- * @param {promise.ControlFlow=} opt_flow The control flow all driver
- *     commands should execute under. Defaults to the
- *     {@link promise.controlFlow() currently active} control flow.
- * @return {!WebDriver} A new WebDriver client for the session.
- */
-function acquireSession(executor, command, description, opt_flow) {
-  let flow = opt_flow || promise.controlFlow();
-  let session = flow.execute(function() {
-    return executeCommand(executor, command);
-  }, description);
-  return new WebDriver(session, executor, flow);
-}
-
-
 /**
  * Translates a command to its wire-protocol representation before passing it
  * to the given `executor` for execution.
@@ -314,11 +290,24 @@ class WebDriver {
    * @return {!WebDriver} A new client for the specified session.
    */
   static attachToSession(executor, sessionId, opt_flow) {
-    return acquireSession(executor,
-        new command.Command(command.Name.DESCRIBE_SESSION).
-            setParameter('sessionId', sessionId),
-        'WebDriver.attachToSession()',
-        opt_flow);
+    let flow = opt_flow || promise.controlFlow();
+    let cmd = new command.Command(command.Name.DESCRIBE_SESSION)
+        .setParameter('sessionId', sessionId);
+    let session = flow.execute(
+        () => executeCommand(executor, cmd),
+        'WebDriver.attachToSession()');
+
+    session = session.catch(err => {
+      // The DESCRIBE_SESSION command is not supported by the W3C spec, so if
+      // we get back an unknown command, just return a session with unknown
+      // capabilities.
+      if (err instanceof error.UnknownCommandError) {
+        return new Session(sessionId, new Capabilities);
+      }
+      throw err;
+    });
+
+    return new WebDriver(session, executor, flow);
   }
 
   /**
@@ -334,11 +323,13 @@ class WebDriver {
    * @return {!WebDriver} The driver for the newly created session.
    */
   static createSession(executor, desiredCapabilities, opt_flow) {
-    return acquireSession(executor,
-        new command.Command(command.Name.NEW_SESSION).
-            setParameter('desiredCapabilities', desiredCapabilities),
-        'WebDriver.createSession()',
-        opt_flow);
+    let flow = opt_flow || promise.controlFlow();
+    let cmd = new command.Command(command.Name.NEW_SESSION)
+        .setParameter('desiredCapabilities', desiredCapabilities) ;
+    let session = flow.execute(
+        () => executeCommand(executor, cmd),
+        'WebDriver.createSession()');
+    return new WebDriver(session, executor, flow);
   }
 
   /**
@@ -418,6 +409,13 @@ class WebDriver {
     this.fileDetector_ = detector;
   }
 
+  /**
+   * @return {!command.Executor} The command executor used by this instance.
+   */
+  getExecutor() {
+    return this.executor_;
+  }
+
   /**
    * @return {!promise.Promise<!Session>} A promise for this client's
    *     session.

javascript/node/selenium-webdriver/test/http/http_test.js

@@ -328,6 +328,111 @@ describe('Executor', function() {
       });
     });
 
+    describe('extracts Session from DESCRIBE_SESSION response', function() {
+      let command;
+
+      beforeEach(function() {
+        executor = new Executor(client);
+        command = new Command(CommandName.DESCRIBE_SESSION)
+            .setParameter('sessionId', 'foo');
+      });
+
+      describe('fails if server returns invalid response', function() {
+        describe('(empty response)', function() {
+          test(true);
+          test(false);
+
+          function test(w3c) {
+            it('w3c === ' + w3c, function() {
+              send.returns(Promise.resolve(new HttpResponse(200, {}, '')));
+              executor.w3c = w3c;
+              return executor.execute(command).then(
+                  () => assert.fail('expected to fail'),
+                  (e) => {
+                    if (!e.message.startsWith('Unable to parse')) {
+                      throw e;
+                    }
+                  });
+            });
+          }
+        });
+
+        describe('(no session ID)', function() {
+          test(true);
+          test(false);
+
+          function test(w3c) {
+            it('w3c === ' + w3c, function() {
+              let resp = {value:{name: 'Bob'}};
+              send.returns(Promise.resolve(
+                  new HttpResponse(200, {}, JSON.stringify(resp))));
+              executor.w3c = w3c;
+              return executor.execute(command).then(
+                  () => assert.fail('expected to fail'),
+                  (e) => {
+                    if (!e.message.startsWith('Unable to parse')) {
+                      throw e;
+                    }
+                  });
+            });
+          }
+        });
+      });
+
+      it('handles legacy response', function() {
+        var rawResponse = {sessionId: 's123', status: 0, value: {name: 'Bob'}};
+
+        send.returns(Promise.resolve(
+            new HttpResponse(200, {}, JSON.stringify(rawResponse))));
+
+        assert.ok(!executor.w3c);
+        return executor.execute(command).then(function(response) {
+          assert.ok(response instanceof Session);
+          assert.equal(response.getId(), 's123');
+
+          let caps = response.getCapabilities();
+          assert.ok(caps instanceof Capabilities);
+          assert.equal(caps.get('name'), 'Bob');
+
+          assert.ok(!executor.w3c);
+        });
+      });
+
+      it('does not auto-upgrade on W3C response', function() {
+        var rawResponse = {sessionId: 's123', value: {name: 'Bob'}};
+
+        send.returns(Promise.resolve(
+            new HttpResponse(200, {}, JSON.stringify(rawResponse))));
+
+        assert.ok(!executor.w3c);
+        return executor.execute(command).then(function(response) {
+          assert.ok(response instanceof Session);
+          assert.equal(response.getId(), 's123');
+
+          let caps = response.getCapabilities();
+          assert.ok(caps instanceof Capabilities);
+          assert.equal(caps.get('name'), 'Bob');
+
+          assert.ok(!executor.w3c);
+        });
+      });
+
+      it('if w3c, does not downgrade on legacy response', function() {
+        var rawResponse = {sessionId: 's123', status: 0, value: null};
+
+        send.returns(Promise.resolve(
+            new HttpResponse(200, {}, JSON.stringify(rawResponse))));
+
+        executor.w3c = true;
+        return executor.execute(command).then(function(response) {
+          assert.ok(response instanceof Session);
+          assert.equal(response.getId(), 's123');
+          assert.equal(response.getCapabilities().size, 0);
+          assert.ok(executor.w3c, 'should never downgrade');
+        });
+      });
+    });
+
     it('handles JSON null', function() {
       var command = new Command(CommandName.GET_CURRENT_URL)
           .setParameter('sessionId', 's123');

javascript/node/selenium-webdriver/test/lib/webdriver_test.js

@@ -272,6 +272,22 @@ describe('WebDriver', function() {
                 (actual) => assert.strictEqual(actual, e));
     });
 
+    it('remote end does not recognize DESCRIBE_SESSION command', function() {
+      let e = new error.UnknownCommandError;
+      let executor = new FakeExecutor().
+          expect(CName.DESCRIBE_SESSION).
+          withParameters({'sessionId': SESSION_ID}).
+          andReturnError(e).
+          end();
+
+      let driver = WebDriver.attachToSession(executor, SESSION_ID);
+      return driver.getSession().then(session => {
+        assert.ok(session instanceof Session);
+        assert.strictEqual(session.getId(), SESSION_ID);
+        assert.equal(session.getCapabilities().size, 0);
+      });
+    });
+
     it('usesActiveFlowByDefault', function() {
       let executor = new FakeExecutor().
           expect(CName.DESCRIBE_SESSION).

javascript/node/selenium-webdriver/test/session_test.js

@@ -0,0 +1,53 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+'use strict';
+
+var WebDriver = require('..').WebDriver,
+    assert = require('../testing/assert'),
+    test = require('../lib/test'),
+    Pages = test.Pages;
+
+
+test.suite(function(env) {
+  var browsers = env.browsers;
+
+  var driver;
+  test.before(function() {
+    driver = env.builder().build();
+  });
+
+  test.after(function() {
+    driver.quit();
+  });
+
+  test.it('can connect to an existing session', function() {
+    driver.get(Pages.simpleTestPage);
+    assert(driver.getTitle()).equalTo('Hello WebDriver');
+
+    return driver.getSession().then(session1 => {
+      let driver2 = WebDriver.attachToSession(
+          driver.getExecutor(),
+          session1.getId());
+
+      assert(driver2.getTitle()).equalTo('Hello WebDriver');
+
+      let session2Id = driver2.getSession().then(s => s.getId());
+      assert(session2Id).equalTo(session1.getId());
+    });
+  });
+});