From f7dcb37c6c03b1f92e5a75981b1d30619b8c517e Mon Sep 17 00:00:00 2001
From: Alec Gibson <aleckgibson@gmail.com>
Date: Thu, 30 Aug 2018 10:05:13 +0100
Subject: [PATCH 1/2] Improve test coverage for
 `SnapshotRequest.onConnectionStateChanged`

This change improves `SnapshotRequest` test coverage to 100%.

Our current test for checking that a dropped connection works didn't
actually fail properly when removing `this.sent = false` from
`SnapshotRequest._onConnectionStateChanged`, so an improved test has
been written that use middleware to wait until the request has
definitely been sent, before dropping the connection and reconnecting.

It also adds a previously uncovered test case for checking that the
connection cannot send the same request multiple times, by using a
similar setup to the rewritten dropped connection case.

This change also adds some documentation recording why we might want to
reset `this.sent = false`, because just looking at the code it's
unclear why we would ever want to do that.
---
 lib/client/snapshot-request.js  |  9 ++++++---
 test/client/snapshot-request.js | 31 ++++++++++++++++++++++++++-----
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/lib/client/snapshot-request.js b/lib/client/snapshot-request.js
index 54679d0cd..aa22b272b 100644
--- a/lib/client/snapshot-request.js
+++ b/lib/client/snapshot-request.js
@@ -56,9 +56,12 @@ SnapshotRequest.prototype.send = function () {
 };
 
 SnapshotRequest.prototype._onConnectionStateChanged = function () {
-  if (this.connection.canSend && !this.sent) {
-    this.send();
-  } else if (!this.connection.canSend) {
+  if (this.connection.canSend) {
+    if (!this.sent) this.send();
+  } else {
+    // If the connection can't send, then we've had a disconnection, and even if we've already sent
+    // the request previously, we need to re-send it over this reconnected client, so reset the
+    // sent flag to false.
     this.sent = false;
   }
 };
diff --git a/test/client/snapshot-request.js b/test/client/snapshot-request.js
index 2ee4fd0b5..80adc113e 100644
--- a/test/client/snapshot-request.js
+++ b/test/client/snapshot-request.js
@@ -214,13 +214,34 @@ describe('SnapshotRequest', function () {
     it('can drop its connection and reconnect, and the callback is just called once', function (done) {
       var connection = backend.connect();
 
-      connection.fetchSnapshot('books', 'don-quixote', function (error) {
-        if (error) return done(error);
-        done();
+      var connectionInterrupted = false;
+      backend.use(backend.MIDDLEWARE_ACTIONS.readSnapshots, function (request, callback) {
+        if (!connectionInterrupted) {
+          connection.close();
+          backend.connect(connection);
+          connectionInterrupted = true;
+        }
+
+        callback();
+      });
+
+      connection.fetchSnapshot('books', 'don-quixote', done);
+    });
+
+    it('cannot send the same request twice over a connection', function (done) {
+      var connection = backend.connect();
+
+      var hasResent = false;
+      backend.use(backend.MIDDLEWARE_ACTIONS.readSnapshots, function (request, callback) {
+        if (!hasResent) {
+          connection._snapshotRequests[1]._onConnectionStateChanged();
+          hasResent = true;
+        }
+
+        callback();
       });
 
-      connection.close();
-      backend.connect(connection);
+      connection.fetchSnapshot('books', 'don-quixote', done);
     });
 
     describe('readSnapshots middleware', function () {

From 70cb681fcc78c96306a47d0fdf0aba074cc3704e Mon Sep 17 00:00:00 2001
From: Alec Gibson <aleckgibson@gmail.com>
Date: Thu, 6 Sep 2018 08:48:51 +0100
Subject: [PATCH 2/2] Document complicated snapshot request testing logic

---
 test/client/snapshot-request.js | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/client/snapshot-request.js b/test/client/snapshot-request.js
index 80adc113e..8711bf7b5 100644
--- a/test/client/snapshot-request.js
+++ b/test/client/snapshot-request.js
@@ -214,6 +214,15 @@ describe('SnapshotRequest', function () {
     it('can drop its connection and reconnect, and the callback is just called once', function (done) {
       var connection = backend.connect();
 
+      // Here we hook into middleware to make sure that we get the following flow:
+      // - Connection established
+      // - Connection attempts to fetch a snapshot
+      // - Snapshot is about to be returned
+      // - Connection is dropped before the snapshot is returned
+      // - Connection is re-established
+      // - Connection re-requests the snapshot
+      // - This time the fetch operation is allowed to complete (because of the connectionInterrupted flag)
+      // - The done callback is called just once (if it's called twice, then mocha will complain)
       var connectionInterrupted = false;
       backend.use(backend.MIDDLEWARE_ACTIONS.readSnapshots, function (request, callback) {
         if (!connectionInterrupted) {
@@ -231,6 +240,13 @@ describe('SnapshotRequest', function () {
     it('cannot send the same request twice over a connection', function (done) {
       var connection = backend.connect();
 
+      // Here we hook into the middleware to make sure that we get the following flow:
+      // - Attempt to fetch a snapshot
+      // - The snapshot request is temporarily stored on the Connection
+      // - Snapshot is about to be returned (ie the request was already successfully sent)
+      // - We attempt to resend the request again
+      // - The done callback is call just once, because the second request does not get sent
+      //   (if the done callback is called twice, then mocha will complain)
       var hasResent = false;
       backend.use(backend.MIDDLEWARE_ACTIONS.readSnapshots, function (request, callback) {
         if (!hasResent) {