Teach the network simulator to wait for Promises.

Tools/GardeningServer/lib/network-simulator.html

Index: Tools/GardeningServer/lib/network-simulator.html
diff --git a/Tools/GardeningServer/lib/network-simulator.html b/Tools/GardeningServer/lib/network-simulator.html
index 9583bdc846b0f30f2507d08d90b38109e8fa5268..e20bbb6277c6e5933743253dd995ec0b79a9c279 100644
--- a/Tools/GardeningServer/lib/network-simulator.html
+++ b/Tools/GardeningServer/lib/network-simulator.html
@@ -44,14 +44,14 @@ NetworkSimulator.prototype.runTest = function(testCase) {
   NetworkSimulator._testInProgress = true;
 
   var self = this;
-  return new Promise(function(resolve, reject) {

[Jeffrey Yasskin, 2014/09/03 22:11:50]

I think it's an antipattern to use the resolve/reject functions unless you have
to deal with an existing callback-taking system. Using them makes the first hop
of a promise asymmetric with the rest of the hops.

-    var realNet = window.net;
+  var realNet = window.net;
 
-    function reset() {
-      window.net = realNet;
-      NetworkSimulator._testInProgress = false;
-    }
+  function reset() {
+    window.net = realNet;
+    NetworkSimulator._testInProgress = false;
+  }
 
+  return Promise.resolve().then(function() {
     // All net.* methods should return promises. This watches all
     // promises generated by test-overridden methods.
     window.net = {};
@@ -63,23 +63,20 @@ NetworkSimulator.prototype.runTest = function(testCase) {
          };
        };
      });
-
-    try {
-      testCase();
-    } catch(e) {
-      // Make sure errors thrown in the test case don't leave window.net in a bad state.
-      reset();
-      self._assert(false, "Test case threw an error:" + e);
-    }
-
-    self.resolvePromises().then(function() {
+  }).then(function() {
+    return testCase();
+  }).catch(function(e) {
+    // Make sure errors thrown in the test case don't leave window.net in a bad state.
+    reset();
+    throw e;
+  }).then(function() {
+    return self.resolvePromises().then(function() {

[Jeffrey Yasskin, 2014/09/03 22:11:50]

I'm not sure we need the resolvePromises() step anymore, since now callers can
explicitly chain from those promises.

       reset();
       self._assert(window.net == realNet);
-      resolve();
     }).catch(function(e) {
       reset();
-      self._assert(false, "Failed to finish test: " + e);
-    });
-  });
+      self._assert(false, "Failed to finish test:" + e);

[Jeffrey Yasskin, 2014/09/03 22:11:50]

I'm not sure if we want to throw a string here or the original error. Throwing
the original error gives a much better stack trace but isn't as clear that it
happened during cleanup.

[ojan, 2014/09/04 18:26:53]

I think this is fine for now. Or, we could just do two _assert calls.

[Jeffrey Yasskin, 2014/09/04 18:48:11]

Two _assert calls doesn't help: Chai uses exceptions to report assertion
failures, so the second _assert would never run. I'll leave it like this for
now.

+    })
+  }).then(this._done, this._done);

[Jeffrey Yasskin, 2014/09/03 22:11:50]

Mocha actually supports returning promises from the it() function
(http://visionmedia.github.io/mocha/#asynchronous-code), so we could omit all
the use of _done here and have callers "return runTest(...)" instead.

[ojan, 2014/09/04 18:26:53]

sgtm

[Jeffrey Yasskin, 2014/09/04 18:48:11]

After experimenting a bit, it looks safer to keep the explicit _done calls. If
we switch to returning a Promise, and a caller forgets to propagate that Promise
out of the it() function, any delayed assertions are simply ignored, which will
be hard to debug. Making people pass in a _done function makes sure Mocha knows
the test will be asynchronous.

 };
 </script>