Switch serializer tests to test round trips + remove deprecated grpc.load() #1699

rsgowman · 2019-04-15T15:59:31Z

Round trip testing move this port closer to the others.
Also changes the way protos in the test are constructed

Separately (though related) this PR also removes the deprecated grpc.load() method. The old tests relied on grpc.load() exposing the proto constructors (which the new protoLoader does not do.)

Rather than separate tests for encoding/decoding. Similar to the other platforms.

rsgowman · 2019-04-15T16:04:07Z

packages/firestore/test/unit/remote/node/serializer.test.ts

        );
+        const proto: api.Value = {
+          timestampValue: (expectedJson[i] as unknown) as string


I'm not sure why the proto's timestampValue is typed as a string? But I also have no idea how protobufjs works.

I also needed the same trick down below in the bytes section, though at least in that case, it sort of makes sense... since at least some of the time this is actually a base64 encoded string.

I think the backend accepts either a timestamp proto or a UTC string and our proto .d.ts file is currently typed as a string (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/protos/firestore_proto_api.d.ts#L356). Per the conversation in chat, I think we can probably go ahead and just fix the d.ts file (so it accepts either format I guess).

rsgowman · 2019-04-15T16:07:42Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-      // clang-format on
-    });
-  });
+      const proto: api.Value = expectedJson;


Regarding this comment which I deleted: "Compare the raw object ... in the event of an inequality".

After my change (I didn't test it before my change), I found this to be incorrect. If I introduced an inequality and ran the tests a few times, the tests always completed with the error string properly generated. As a result, I eliminated the special casing here.

We don't use jasmine anymore, so this is very likely fine. :) Thanks!

mikelehen

I haven't done a fully detailed review yet but at a high-level I think I'm mostly on board with this (thanks a ton for tackling all of this). I left a few comments and I do have a bit of reservation about no longer constructing actual proto objects. As-is if you make a mistake in serializer.ts with the JSON we generate such that the JSON is wrong, the serializer tests won't notice as long as you make the mistake in both the serialization and deserialization methods. In theory the JSON representation of the protos is strongly typed based on https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/protos/firestore_proto_api.d.ts#L1, but there are some nuances / hacks because grpc and webchannel speak slightly different dialects of JSON protos so it's still conceivable we could screw something up... though presumably integration tests would catch it.

If we wanted to round-trip through the actual proto object that gRPC uses (similar to the way the tests did before), I think we could just load the protos directly in our tests via protobufjs instead of going through @grpc/proto-loader and then we'd get access to the constructors as before. Basically just copy the gprc/proto-loader loadSync() code: https://github.com/grpc/grpc-node/blob/073dc563858a2d74bd114e5cc50d02203123078f/packages/proto-loader/src/index.ts#L314

I'd be a fan of that change, but if you don't feel like taking it on, I can probably get behind this PR as-is or else I might take a stab at it myself...

mikelehen · 2019-04-15T20:45:00Z

packages/firestore/src/platform_node/load_protos.ts

+
+  // Beware that converting fields to camel case does not convert the tag fields
+  // in oneof groups (!!!). This will likely be fixed when we upgrade to
+  // protobufjs 6.x


I think this comment is probably stale since:

gprc/proto-loader pulls in protobufjs 6.8.6.

This comment used to be referring to our usage of 'convertFieldsToCamelCase' which no longer exists.

I think this comment is referring to the fact that the tag fields are named value_type, response_type, etc. instead of valueType, responseType, etc. And I guess this is not going to change (since you didn't have to change that code in serializer.ts), so I would just drop the comment.

mikelehen · 2019-04-15T20:52:39Z

packages/firestore/src/platform_node/load_protos.ts

+    defaults: true,
+    oneofs: true,
+    includeDirs: [root]
+  });


I wonder if it's worth commenting these options a bit so somebody (future me) doesn't have to go look up the grpc docs to figure out what they mean...

mikelehen · 2019-04-15T20:56:06Z

packages/firestore/src/protos/google/protobuf/descriptor.proto

@@ -0,0 +1,882 @@
+// Protocol Buffers - Google's data interchange format


Just to verify (I'm too lazy to diff), did you copy this from https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/descriptor.proto ?

Can you update https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/protos/update.sh to automatically copy it?

mikelehen · 2019-04-15T21:46:02Z

packages/firestore/test/unit/remote/node/serializer.test.ts

+      // as optional fields. To stay consistent, we'll still test for the given
+      // tag's presence here, though we won't test for absence of all other
+      // fields.
+      expect(actual[tag]).to.not.be.undefined;


Hrm...

Since the following expectation fully verifies the contents of actual, it seems like this must be redundant, no? Or is this meant to verify the test is written correctly? If so, consider using an assert() instead.

The old code verified that the serializer generated protos that contain value_type, which I think was important. If I'm understanding correctly from

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

Line 1369 in eaa6aa1

* Checks for a specific oneof tag in a protocol buffer message.

, gRPC would produce output messages without value_type but require input messages to have value_type. But I wonder if that asymmetry still exists (or if we can avoid it by tweaking the options we pass to loadSync()).

So I think we need to rework the test to verify the serializer generates (and accepts) value_type or else rework the serializer so we don't need the hasTag() mess (which would be amazing if it works).

mikelehen · 2019-04-15T22:21:19Z

packages/firestore/test/unit/remote/node/serializer.test.ts

        );
+        const proto: api.Value = {
+          timestampValue: (expectedJson[i] as unknown) as string


I think the backend accepts either a timestamp proto or a UTC string and our proto .d.ts file is currently typed as a string (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/protos/firestore_proto_api.d.ts#L356). Per the conversation in chat, I think we can probably go ahead and just fix the d.ts file (so it accepts either format I guess).

mikelehen · 2019-04-15T22:42:47Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-      // clang-format on
-    });
-  });
+      const proto: api.Value = expectedJson;


We don't use jasmine anymore, so this is very likely fine. :) Thanks!

mikelehen · 2019-04-15T22:48:00Z

packages/firestore/package.json

@@ -31,6 +31,7 @@
    "@firebase/logger": "0.1.11",
    "@firebase/webchannel-wrapper": "0.2.17",
    "grpc": "1.19.0",
+    "@grpc/proto-loader": "^0.4.0",


You need to run yarn or yarn install from the root and check in the resulting yarn.lock file.

mikelehen · 2019-04-16T01:32:08Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-        new fieldValue.BlobValue(blob)
-      );
+      expectRoundTrip(objValue, proto, 'mapValue');
+      // clang-format on


what is this?

rsgowman · 2019-04-16T14:49:09Z

I haven't done a fully detailed review yet but at a high-level I think I'm mostly on board with this (thanks a ton for tackling all of this). I left a few comments and I do have a bit of reservation about no longer constructing actual proto objects. As-is if you make a mistake in serializer.ts with the JSON we generate such that the JSON is wrong, the serializer tests won't notice as long as you make the mistake in both the serialization and deserialization methods. In theory the JSON representation of the protos is strongly typed based on https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/protos/firestore_proto_api.d.ts#L1, but there are some nuances / hacks because grpc and webchannel speak slightly different dialects of JSON protos so it's still conceivable we could screw something up... though presumably integration tests would catch it.

If we wanted to round-trip through the actual proto object that gRPC uses (similar to the way the tests did before), I think we could just load the protos directly in our tests via protobufjs instead of going through @grpc/proto-loader and then we'd get access to the constructors as before. Basically just copy the gprc/proto-loader loadSync() code: https://github.com/grpc/grpc-node/blob/073dc563858a2d74bd114e5cc50d02203123078f/packages/proto-loader/src/index.ts#L314

I'd be a fan of that change, but if you don't feel like taking it on, I can probably get behind this PR as-is or else I might take a stab at it myself...

Take a look at this: #1669. It's my first attempt; I don't like it as much, but I think it does what you're suggesting. I think it may be possible to meld these two together, so that we still get access to the underlying ctors, but also can eliminate all the fromFoo tests. If that's what you had in mind, then I'll tackle it.

mikelehen · 2019-04-16T14:59:11Z

Oh! Yes, #1669 is very much what I had in mind, but yeah, I think combining that with all the cleanup you did here would be great. Basically make your "roundtrip" methods also construct the real proto instead of just the JSON representation (maybe even encode to bytes and back to JSON?). That would make these tests really nice.

wilhuff · 2019-04-16T14:59:47Z

Could we meld these two approaches together by essentially running the round trip twice? Once directly and once through protos? This would have the same benefit of reducing the volume of tests, while giving us the same net coverage.

Note that this is at least conceptually similar to what we're doing in the C++ serialization tests, where we're validating that our serialization generates bytes that libprotobuf can parse. There we modified the round trip validation to check both paths. Though the particulars are different here, could the same approach work?

wilhuff · 2019-04-16T15:00:06Z

LOL. I wish I refreshed before posting.

rsgowman · 2019-05-02T14:12:38Z

Closing in favour of #1669 (which contains the relevant bits from this pr)

rsgowman added 3 commits April 15, 2019 11:39

Switch serializer tests to test round trips

e9baed4

Rather than separate tests for encoding/decoding. Similar to the other platforms.

Remove use of deprecated grpc.load()

98dc7f4

[AUTOMATED]: Prettier Code Styling

634b0c0

rsgowman self-assigned this Apr 15, 2019

fixups

93ea0c9

rsgowman commented Apr 15, 2019

View reviewed changes

rsgowman mentioned this pull request Apr 15, 2019

grpc deprecated error on v5.3.1 #1112

Closed

lint

b3a1f6f

rsgowman added the api: firestore label Apr 15, 2019

rsgowman assigned mikelehen and unassigned rsgowman Apr 15, 2019

rsgowman requested a review from mikelehen April 15, 2019 18:28

mikelehen suggested changes Apr 16, 2019

View reviewed changes

mikelehen assigned rsgowman and unassigned mikelehen Apr 16, 2019

rsgowman mentioned this pull request Apr 24, 2019

Remove use of deprecated grpc.load() method #1669

Merged

rsgowman closed this May 2, 2019

firebase locked and limited conversation to collaborators Oct 12, 2019

wilhuff deleted the rsgowman/grpc_load_2 branch March 13, 2020 16:26

		@@ -0,0 +1,882 @@
		// Protocol Buffers - Google's data interchange format

Switch serializer tests to test round trips + remove deprecated grpc.load() #1699

Switch serializer tests to test round trips + remove deprecated grpc.load() #1699

Uh oh!

Conversation

rsgowman commented Apr 15, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsgowman commented Apr 16, 2019

Uh oh!

mikelehen commented Apr 16, 2019

Uh oh!

wilhuff commented Apr 16, 2019

Uh oh!

wilhuff commented Apr 16, 2019

Uh oh!

rsgowman commented May 2, 2019

Uh oh!

Uh oh!