Fix bug and add tests.

Author: Michael Lehenbauer

packages/firestore/src/remote/remote_store.ts

@@ -248,7 +248,7 @@ export class RemoteStore implements TargetMetadataProvider {
     if (objUtils.isEmpty(this.listenTargets)) {
       if (this.watchStream.isOpen()) {
         this.watchStream.markIdle();
-      } else {
+      } else if (this.canUseNetwork()) {
         // Revert to OnlineState.Unknown if the watch stream is not open and we
         // have no listeners, since without any listens to send we cannot
         // confirm if the stream is healthy and upgrade to OnlineState.Online.

packages/firestore/test/integration/api/database.test.ts

@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+import * as chai from 'chai';
+import * as chaiAsPromised from 'chai-as-promised';
+
 import { expect } from 'chai';
 import * as firestore from '@firebase/firestore-types';
 
@@ -29,6 +32,8 @@ import {
 } from '../util/helpers';
 import { query } from '../../util/api_helpers';
 
+chai.use(chaiAsPromised);
+
 const Timestamp = firebase.firestore.Timestamp;
 
 apiDescribe('Database', persistence => {
@@ -856,26 +861,25 @@ apiDescribe('Database', persistence => {
     });
   });
 
-  it('can get documents while offline', () => {
-    return withTestDoc(persistence, docRef => {
+  it('can get documents while offline', async () => {
+    await withTestDoc(persistence, async docRef => {
       const firestore = docRef.firestore;
 
-      return firestore.disableNetwork().then(() => {
-        const writePromise = docRef.set({ foo: 'bar' });
-
-        return docRef
-          .get()
-          .then(doc => {
-            expect(doc.metadata.fromCache).to.be.true;
-            return firestore.enableNetwork();
-          })
-          .then(() => writePromise)
-          .then(() => docRef.get())
-          .then(doc => {
-            expect(doc.metadata.fromCache).to.be.false;
-            expect(doc.data()).to.deep.equal({ foo: 'bar' });
-          });
-      });
+      await firestore.disableNetwork();
+      await expect(docRef.get()).to.eventually.be.rejectedWith(
+        'Failed to get document because the client is offline.'
+      );
+
+      const writePromise = docRef.set({ foo: 'bar' });
+      const doc = await docRef.get();
+      expect(doc.metadata.fromCache).to.be.true;
+
+      await firestore.enableNetwork();
+      await writePromise;
+
+      const doc2 = await docRef.get();
+      expect(doc2.metadata.fromCache).to.be.false;
+      expect(doc2.data()).to.deep.equal({ foo: 'bar' });
     });
   });
 

packages/firestore/test/unit/specs/offline_spec.test.ts

@@ -215,4 +215,21 @@ describeSpec('Offline:', [], () => {
       );
     }
   );
+
+  specTest('Queries return from cache when network disabled', [], () => {
+    const query = Query.atPath(path('collection'));
+    return (
+      spec()
+        .disableNetwork()
+        .userListens(query)
+        .expectEvents(query, { fromCache: true })
+        .userUnlistens(query)
+
+        // There was once a bug where removing the last listener accidentally
+        // reverted us to OnlineState.Unknown, so make sure it works a second time
+        .userListens(query)
+        .expectEvents(query, { fromCache: true })
+        .userUnlistens(query)
+    );
+  });
 });

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -370,6 +370,8 @@ abstract class TestRunner {
     [targetId: number]: { query: SpecQuery; resumeToken: string };
   };
 
+  private networkEnabled = true;
+
   private datastore: Datastore;
   private localStore: LocalStore;
   private remoteStore: RemoteStore;
@@ -603,7 +605,7 @@ abstract class TestRunner {
       );
     }
 
-    if (this.isPrimaryClient) {
+    if (this.isPrimaryClient && this.networkEnabled) {
       // Open should always have happened after a listen
       await this.connection.waitForWatchOpen();
     }
@@ -847,6 +849,7 @@ abstract class TestRunner {
   }
 
   private async doDisableNetwork(): Promise<void> {
+    this.networkEnabled = false;
     // Make sure to execute all writes that are currently queued. This allows us
     // to assert on the total number of requests sent before shutdown.
     await this.remoteStore.fillWritePipeline();
@@ -858,6 +861,7 @@ abstract class TestRunner {
   }
 
   private async doEnableNetwork(): Promise<void> {
+    this.networkEnabled = true;
     await this.syncEngine.enableNetwork();
   }
 
@@ -1015,7 +1019,7 @@ abstract class TestRunner {
   }
 
   private async validateActiveTargets(): Promise<void> {
-    if (!this.isPrimaryClient) {
+    if (!this.isPrimaryClient || !this.networkEnabled) {
       expect(this.connection.activeTargets).to.be.empty;
       return;
     }