Creating Stream Tests for the Web Client #260

schmidt-sebastian · 2017-10-24T21:32:06Z

This is a port of firebase/firebase-ios-sdk#391

mikelehen · 2017-10-25T14:53:47Z

packages/firestore/test/integration/remote/stream.test.ts

+   * Fails the test if the expected callback does not match the name of the
+   * actual callback function.
+   */
+  awaitCallback(expectedCallback: StreamStatusCallback): Promise<void> {


I'd normally put the public surface area of the class (awaitCallback / verifyNoPendingCallbacks) at the top, but feel free to leave as-is for symmetry with iOS.

Moved to top.

mikelehen · 2017-10-25T14:58:58Z

packages/firestore/test/integration/remote/stream.test.ts

+import { getDefaultDatabaseInfo } from '../util/helpers';
+import FirestoreError = firestore.FirestoreError;
+
+type StreamStatusCallback =


Perhaps StreamCallbackType or StreameEvent? (not all of these are "status" events, and I'd expect an actual "callback" type to be a function, not a string)

I think the slightly strange thing here is that we're including 'on' in the names, which makes them refer to the method you call to register an event callback rather than the event itself. So it's also tempting to drop the 'on's ?

Also, is it worth adding a comment calling out that this is a merging of the watch stream and write stream callback types?

I renamed it to StreameEventType, which indeed suggests to remove the 'on' from the type definition.

mikelehen · 2017-10-25T15:02:57Z

packages/firestore/test/integration/remote/stream.test.ts

+  | 'onClose';
+
+class StreamStatusListener implements WatchStreamListener, WriteStreamListener {
+  private pendingStates: StreamStatusCallback[] = [];


'state' seems out-of-place. pendingCallbacks?

mikelehen · 2017-10-25T15:03:43Z

packages/firestore/test/integration/remote/stream.test.ts

+  private pendingStates: StreamStatusCallback[] = [];
+  private pendingPromises: Deferred<StreamStatusCallback>[] = [];
+
+  private resolvePending(actualCallback: StreamStatusCallback) {


can you declare the return type?

mikelehen · 2017-10-25T15:05:56Z

packages/firestore/test/integration/remote/stream.test.ts

+  private pendingPromises: Deferred<StreamStatusCallback>[] = [];
+
+  private resolvePending(actualCallback: StreamStatusCallback) {
+    let pendingPromise = this.pendingPromises.shift();


nit: Here you're calling shift() and checking for undefined. Below (for pendingStates) you check the length > 0 before calling shift. I prefer the latter, but regardless I'd try to be consistent.

I went with the length check, but left the intermediate variable since it makes the control flow easier to follow.

mikelehen · 2017-10-25T15:15:07Z

packages/firestore/test/integration/remote/stream.test.ts

+      return deferred.promise.then(actualCallback => {
+        expect(actualCallback).to.equal(expectedCallback);
+      });
+    }


This code is a bit wonky in that on failure it might throw an exception or might return a failed promise, depending on if we have pendingStates (and that the expectedCallback expectation is repeated). Perhaps do something like:

let promise: Promise<StreamStatusCallback>; if (this.pendingStates.length > 0) { promise = Promise.resolve(this.pendingStates.shift()); } else { const deferred = new Deferred<StreamStatusCallback>(); this.pendingPromises.push(deferred); promise = deferred.promise; } return promise.then(actualCallback => { expect(actualCallback).to.equal(expectedCallback); });

I changed the code to always return a Promise, and to reject it if the callback type doesn't match.

mikelehen · 2017-10-25T15:17:04Z

packages/firestore/test/integration/remote/stream.test.ts

+          new EmptyCredentialsProvider(),
+          serializer
+        );
+      })


Does it make sense to factor out an initializeDatastore() method to avoid 90% of this repeated code?

mikelehen · 2017-10-25T15:20:17Z

packages/firestore/test/integration/remote/stream.test.ts

+      })
+      .then(() => {
+        writeStream.stop();
+      });


Why does the web test not verify that attempting to write before handshake throws but works after? If we're going to the effort of porting the tests, we should make sure they're at parity (or at least add a comment for why they're not). :-)

I totally forgot to port this part :(

mikelehen · 2017-10-25T15:23:04Z

packages/firestore/test/integration/remote/stream.test.ts

+      });
+  }
+
+  asyncIt('can be stopped before handshake', () => {


same comment as above about not testing we don't get extra onClose event after stop().

mikelehen · 2017-10-25T15:24:11Z

packages/firestore/test/integration/remote/stream.test.ts

+      });
+  }
+
+  asyncIt('can be stopped before handshake', () => {


The iOS test has a comment:

/** Verifies that the watch stream does not issue an onClose callback after a call to stop(). */

and does indeed trigger a GRPC close event (to make sure that our stream doesn't trigger a close event in response). Why aren't we testing that on web?

I went back and forth on this just now, but decided to ultimately leave it as is. The handleStreamClose callback on the Web only ever gets called when the stream is still open and asserts that this is the case. To bring it closer to the other platforms, I would have to break this invariant.

If you prefer, I can also assert here that 'handleStreamClose' throws an error at this point. Just making it public for that doesn't seem worth it - especially since it invites other consumers to trip over this assertion.

I see. To make web a more direct port, I think we'd refactor

firebase-js-sdk/packages/firestore/src/remote/persistent_stream.ts

Line 339 in fba3c06

this.stream.onClose((error: FirestoreError) => {

so that it delegates to a function that could then be called from this test. But going to that much trouble probably isn't warranted.

mikelehen

Thanks! Nice withTestDatastore() cleanup.

mikelehen · 2017-10-25T18:31:59Z

packages/firestore/test/integration/remote/stream.test.ts

+        return Promise.reject(
+          `Unexpected callback encountered. Expected '${expectedCallback}', but got '${actualCallback}'.`
+        );
+      }


I don't have any strong preference, but FYI I think using expect(...).to.equal(...) here was probably fine.

mikelehen · 2017-10-25T18:35:18Z

packages/firestore/test/integration/remote/stream.test.ts

+      watchStream.start();
+
+      return streamListener.awaitCallback('open').then(() => {
+        // Stop must not call onClose because the full implementation of the delegate could


delegate => callback ?

mikelehen · 2017-10-25T18:36:08Z

packages/firestore/test/integration/remote/stream.test.ts

+      // Don't start the handshake.
+
+      // Stop must not call onClose because the full implementation of the delegate could
+      // attempt to restart the stream in the event it had pending watches.


delagate => callback and while you're here maybe fix "pending watches" to "pending writes" (even though Obj-C is wrong too :-))

schmidt-sebastian requested review from mikelehen and wilhuff as code owners October 24, 2017 21:32

google-oss-bot added the needs-triage label Oct 24, 2017

Creating Stream Tests for the Web Client

a30f63b

schmidt-sebastian force-pushed the mrschmidt-closewhenidle branch from a37c4bd to a30f63b Compare October 24, 2017 21:34

Updating comment

d47f81f

schmidt-sebastian assigned mikelehen Oct 24, 2017

Using equal everywhere

5d4fa69

mikelehen suggested changes Oct 25, 2017

View reviewed changes

schmidt-sebastian added 2 commits October 25, 2017 11:03

Addressing review feedback

d0d40ef

[AUTOMATED]: Prettier Code Styling

4dde0a2

mikelehen approved these changes Oct 25, 2017

View reviewed changes

Addressing feedback

68b04ab

schmidt-sebastian merged commit e1dceac into master Oct 25, 2017

firebase locked and limited conversation to collaborators Oct 25, 2019

Creating Stream Tests for the Web Client #260

Creating Stream Tests for the Web Client #260

Uh oh!

Conversation

schmidt-sebastian commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian Oct 25, 2017 •

edited

Loading