Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add totalDocuments and totalBytes to bundle metadata. #1085

Merged
merged 4 commits into from
May 26, 2020
Merged

feat: Add totalDocuments and totalBytes to bundle metadata. #1085

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be577dd
feat: Add totalDocuments and totalBytes to bundle metadata.
wu-hui May 19, 2020
095e12d
fix: Better comment
wu-hui May 19, 2020
38abf77
fix: Better testing.
wu-hui May 21, 2020
6905f59
fix: Improve metadata testing.
wu-hui May 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dev/src/bundle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export class BundleBuilder {
totalDocuments: this.documents.size,
totalBytes: bundleBuffer.length,
};
// Prepends the metadata element to the bundleBuffer, note `bundleBuffer` is the second argument to `Buffer.concat`.
// Prepends the metadata element to the bundleBuffer: `bundleBuffer` is the second argument to `Buffer.concat`.
bundleBuffer = Buffer.concat([
this.elementToLengthPrefixedBuffer({metadata}),
bundleBuffer,
Expand Down
13 changes: 4 additions & 9 deletions dev/system-test/firestore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
WriteResult,
} from '../src';
import {autoId, Deferred} from '../src/util';
import {verifyMetadata} from '../test/bundle';
import {
bundleToElementArray,
Post,
Expand Down Expand Up @@ -2416,9 +2417,7 @@ describe('Bundle building', () => {
const elements = await bundleToElementArray(bundle.build());

const meta = (elements[0] as IBundleElement).metadata;
expect(meta!.totalBytes).greaterThan(0);
delete meta!.totalBytes;
expect(meta).to.deep.equal({
verifyMetadata(meta!, {
id: 'test-bundle',
createTime: snap.readTime.toProto().timestampValue,
version: 1,
Expand Down Expand Up @@ -2452,9 +2451,7 @@ describe('Bundle building', () => {
expect(elements.length).to.equal(2);

const meta = (elements[0] as IBundleElement).metadata;
expect(meta!.totalBytes).greaterThan(0);
delete meta!.totalBytes;
expect(meta).to.deep.equal({
verifyMetadata(meta!, {
id: 'test-bundle',
createTime: snap.readTime.toProto().timestampValue,
version: 1,
Expand Down Expand Up @@ -2482,9 +2479,7 @@ describe('Bundle building', () => {
const elements = await bundleToElementArray(await bundle.build());

const meta = (elements[0] as IBundleElement).metadata;
expect(meta!.totalBytes).greaterThan(0);
delete meta!.totalBytes;
expect(meta).to.deep.equal({
verifyMetadata(meta!, {
id: 'test-bundle',
createTime: limitToLastSnap.readTime.toProto().timestampValue,
version: 1,
Expand Down
26 changes: 14 additions & 12 deletions dev/test/bundle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ import {
verifyInstance,
} from './util/helpers';
import IBundleElement = firestore.IBundleElement;
import IBundleMetadata = firestore.IBundleMetadata;

export function verifyMetadata(
meta: IBundleMetadata,
expected: IBundleMetadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping you would pass in the expected state one at a time:

export function verifyMetadata(
  meta: IBundleMetadata,
  id: string,
  createTime: ReadTime,
  version: number,
  totalDocuments: 0
)

From that, you can simplify further. Extract a constant for the bundle ID, and use it everywhere. The version is also always a constant. So in the end you have:

export function verifyMetadata(
  meta: IBundleMetadata,
  createTime: ReadTime,
  totalDocuments: 0
)

You can then also compare each field directly:

  expect(meta.totalBytes).greaterThan(0);
  expect(meta.id).to.equal(BUNDLE_ID);
  expect(meta.version).to.equal(1);
  expect(meta.createTime).to.deep.equal(readTime.toProto());
  expect(meta.totalDocuments).to.equal(totalDocuments);

This removes the need to delete the field and ruins nasty surprises in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

): void {
expect(meta!.totalBytes).greaterThan(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

delete meta!.totalBytes;
expect(meta).to.deep.equal(expected);
}

describe('Bundle Buidler', () => {
let firestore: Firestore;
Expand Down Expand Up @@ -80,9 +90,7 @@ describe('Bundle Buidler', () => {
expect(elements.length).to.equal(3);

const meta = (elements[0] as IBundleElement).metadata;
expect(meta!.totalBytes).greaterThan(0);
delete meta!.totalBytes;
expect(meta).to.deep.equal({
verifyMetadata(meta!, {
id: 'test-bundle',
// `snap1.readTime` is the bundle createTime, because it is larger than `snap2.readTime`.
createTime: snap1.readTime.toProto().timestampValue,
Expand Down Expand Up @@ -131,9 +139,7 @@ describe('Bundle Buidler', () => {
expect(elements.length).to.equal(4);

const meta = (elements[0] as IBundleElement).metadata;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind adding a helper for the metadata comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

expect(meta!.totalBytes).greaterThan(0);
delete meta!.totalBytes;
expect(meta).to.deep.equal({
verifyMetadata(meta!, {
id: 'test-bundle',
// `snap.readTime` is the bundle createTime, because it is larger than `snap2.readTime`.
createTime: snap.readTime.toProto().timestampValue,
Expand Down Expand Up @@ -187,9 +193,7 @@ describe('Bundle Buidler', () => {
expect(elements.length).to.equal(3);

const meta = (elements[0] as IBundleElement).metadata;
expect(meta!.totalBytes).greaterThan(0);
delete meta!.totalBytes;
expect(meta).to.deep.equal({
verifyMetadata(meta!, {
id: 'test-bundle',
// `snap1.readTime` is the bundle createTime, because it is larger than `snap2.readTime`.
createTime: snap1.readTime.toProto().timestampValue,
Expand Down Expand Up @@ -225,9 +229,7 @@ describe('Bundle Buidler', () => {

expect(newElements.length).to.equal(5);
const newMeta = (newElements[0] as IBundleElement).metadata;
expect(newMeta!.totalBytes).greaterThan(0);
delete newMeta!.totalBytes;
expect(newMeta).to.deep.equal({
verifyMetadata(newMeta!, {
id: 'test-bundle',
// `snap1.readTime` is the bundle createTime, because it is larger than `snap2.readTime`.
createTime: snap1.readTime.toProto().timestampValue,
Expand Down