Promise.thenFinally should not suppress the original failure.

Fixes issue 7156.
SeleniumHQ · May 26, 2014 · f1d59a0 · f1d59a0
1 parent 227674b
commit f1d59a0
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 7 deletions.
diff --git a/javascript/node/selenium-webdriver/CHANGES.md b/javascript/node/selenium-webdriver/CHANGES.md
@@ -1,3 +1,7 @@
+## v2.43.0-dev
+
+* FIXED: 7156: `Promise#thenFinally` should not suppress original error
+
 ## v2.42.0
 
 * Removed deprecated functions `Promise#addCallback()`,

diff --git a/javascript/node/selenium-webdriver/package.json b/javascript/node/selenium-webdriver/package.json
@@ -1,6 +1,6 @@
 {
     "name": "selenium-webdriver",
-    "version": "2.42.0",
+    "version": "2.43.0-dev",
     "description": "The official WebDriver JavaScript bindings from the Selenium project",
     "keywords": [
         "automation",

diff --git a/javascript/webdriver/promise.js b/javascript/webdriver/promise.js
@@ -184,7 +184,15 @@ webdriver.promise.Promise.prototype.thenCatch = function(errback) {
  * @template R
  */
 webdriver.promise.Promise.prototype.thenFinally = function(callback) {
-  return this.then(callback, callback);
+  return this.then(callback, function(err) {
+    var value = callback();
+    if (webdriver.promise.isPromise(value)) {
+      return value.then(function() {
+        throw err;
+      });
+    }
+    throw err;
+  });
 };
 
 

diff --git a/javascript/webdriver/test/promise_flow_test.html b/javascript/webdriver/test/promise_flow_test.html
@@ -1175,10 +1175,13 @@
 
     scheduleAction('doFoo and throw', function() {
       webdriver.test.testutil.messages.push('foo');
-      throw new Error('ouch');
+      throw STUB_ERROR;
     }).then(goog.partial(schedulePush, 'bar')).
         thenFinally(goog.partial(schedulePush, 'baz'));
-    runAndExpectSuccess(assertingMessages('foo', 'baz'));
+    runAndExpectFailure(function(e) {
+      assertIsStubError(e);
+      webdriver.test.testutil.assertMessages('foo', 'baz');
+    });
   }
 
 
@@ -1199,11 +1202,13 @@
             throw STUB_ERROR;
           });
         }).
-        thenFinally(function(e) {
-          assertIsStubError(e);
+        thenFinally(function() {
           return schedulePush('baz');
         });
-    runAndExpectSuccess(assertingMessages('foo', 'bar', 'baz'));
+    runAndExpectFailure(function(e) {
+      assertIsStubError(e);
+      webdriver.test.testutil.assertMessages('foo', 'bar' , 'baz');
+    });
   }
 
 

diff --git a/javascript/webdriver/test/promise_test.html b/javascript/webdriver/test/promise_test.html
@@ -248,6 +248,44 @@
 }
 
 
+function testThenFinally_nonFailingCallbackDoesNotSuppressOriginalError() {
+  var done = callbackHelper(assertIsStubError);
+  webdriver.promise.rejected(STUB_ERROR).
+      thenFinally(goog.nullFunction).
+      thenCatch(done);
+  done.assertCalled();
+}
+
+
+function testThenFinally_failingCallbackSuppressesOriginalError() {
+  var done = callbackHelper(assertIsStubError);
+  webdriver.promise.rejected(new Error('original')).
+      thenFinally(throwStubError).
+      thenCatch(done);
+  done.assertCalled();
+}
+
+
+function testThenFinally_callbackThrowsAfterFulfilledPromise() {
+  var done = callbackHelper(assertIsStubError);
+  webdriver.promise.fulfilled().
+      thenFinally(throwStubError).
+      thenCatch(done);
+  done.assertCalled();
+}
+
+
+function testThenFinally_callbackReturnsRejectedPromise() {
+  var done = callbackHelper(assertIsStubError);
+  webdriver.promise.fulfilled().
+      thenFinally(function() {
+        return webdriver.promise.rejected(STUB_ERROR);
+      }).
+      thenCatch(done);
+  done.assertCalled();
+}
+
+
 function testChainingThen_AllResolved() {
   var callbacks = [
     callbackHelper(function(value) {