Add bundle conformance tests to verify documents are loaded correctly. #4337
Conversation
|
Size Analysis ReportAffected Products
|
| @@ -207,7 +207,7 @@ export function bundleInitialProgress( | |||
| documentsLoaded: 0, | |||
| bytesLoaded: 0, | |||
| totalDocuments: metadata.totalDocuments!, | |||
| totalBytes: metadata.totalBytes! | |||
| totalBytes: parseInt(metadata.totalBytes!.toString(), 10) | |||
There was a problem hiding this comment.
Please use normailizeNumber for this: https://osscs.corp.google.com/firebase-sdk/firebase-js-sdk/+/master:packages/firestore/src/model/normalize.ts;l=73?q=normalizeNumber&ss=firebase-sdk%2Ffirebase-js-sdk
This is the function that we use in other places to deserialize large numbers.
| ]; | ||
|
|
||
| // This is built by Java(proto3 json formatter) with above document and one additional field of `pathValue: db.doc('col1/ref1')`. | ||
| const BUNDLES_PROTOB3_CONFORMANCE_TEMPLATE = [ |
There was a problem hiding this comment.
What does the B stand for?
There was a problem hiding this comment.
Stands for I-cannot-type i guess.
There was a problem hiding this comment.
Please fix though :)
| bytesValue: Blob.fromUint8Array(new Uint8Array([0x01, 0x02])) | ||
| }; | ||
|
|
||
| // This is built by Node(protobuf.js) with above document and one additional field of `pathValue: db.doc('col1/ref1')`. |
There was a problem hiding this comment.
Note that this is now a strange mix of ProtobufJS and Proto3 JSON.
| @@ -252,3 +264,178 @@ apiDescribe('Bundles', (persistence: boolean) => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| apiDescribe('Bundles conformance', (persistence: boolean) => { | |||
| const CONFORMANCE_DOC = { | |||
There was a problem hiding this comment.
The approach of "hardcoding" the JSON strings from Node and Java has some merits, but since it is Monday morning and we have a full week ahead of us, I would like to point out that you could also use the Web serializer to built both Proto3 JSON and ProtobufJS documents in the tests themselves. This approach has some benefits (less code to copy/paste, less chance that the code will go out of sync) and some downsides (less portable, may not catch bugs in the Node/Java clients) so I will leave the decision to you.
If we do decide to only support Proto3 JSON, we can also get rid of one of the tests here.
There was a problem hiding this comment.
I added a todo instead, my 3 minute research did not tell me clearly how to do it..
There was a problem hiding this comment.
We might want to spend some more time on this since adding this and then removing it right away just adds code churn. You should be able to call wrap() with CONFORMANCE_DOC and get a JSON encoded Value proto. You may have to add an overload so you can control the useProto3JSON flag (and as discussed, this only works in api_internal)
There was a problem hiding this comment.
As discussed offline, this is harder than I expected, and does not bring much value because we still need to hard code elements that are not documents. Let's not do this for now?
| @@ -252,3 +264,178 @@ apiDescribe('Bundles', (persistence: boolean) => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| apiDescribe('Bundles conformance', (persistence: boolean) => { | |||
| const CONFORMANCE_DOC = { | |||
There was a problem hiding this comment.
We might want to spend some more time on this since adding this and then removing it right away just adds code churn. You should be able to call wrap() with CONFORMANCE_DOC and get a JSON encoded Value proto. You may have to add an overload so you can control the useProto3JSON flag (and as discussed, this only works in api_internal)
| ]; | ||
|
|
||
| // This is built by Java(proto3 json formatter) with above document and one additional field of `pathValue: db.doc('col1/ref1')`. | ||
| const BUNDLES_PROTOB3_CONFORMANCE_TEMPLATE = [ |
There was a problem hiding this comment.
Please fix though :)
Co-authored-by: Sebastian Schmidt <[email protected]>
No description provided.