Add test coverage for protobuf conversion edge cases (#2496)

Author: rfontanarosa

lib/src/firestore-to-proto.spec.ts

@@ -19,6 +19,7 @@ import { toMessage } from './firestore-to-proto';
 import { Constructor } from 'protobufjs';
 
 const {
+  AuditInfo,
   Coordinates,
   Job,
   LinearRing,
@@ -28,6 +29,7 @@ const {
   Task,
   LocationOfInterest,
 } = GroundProtos.ground.v1beta1;
+const { Timestamp } = GroundProtos.google.protobuf;
 
 describe('toMessage()', () => {
   [
@@ -138,6 +140,13 @@ describe('toMessage()', () => {
       },
       expected: new Survey(),
     },
+    {
+      desc: 'skips repeated field when value is not an array',
+      input: {
+        '1': 'not an array',
+      },
+      expected: new LinearRing(),
+    },
     {
       desc: 'converts enum value',
       input: {
@@ -152,6 +161,45 @@ describe('toMessage()', () => {
       input: {},
       expected: new Task.DateTimeQuestion(),
     },
+    {
+      desc: 'skips non-numeric enum value',
+      input: {
+        '1': 'not-a-number',
+      },
+      expected: new Task.DateTimeQuestion(),
+    },
+    {
+      desc: 'converts google.protobuf.Timestamp field',
+      input: {
+        '1': 'user-123',
+        '2': { '1': 1700000000, '2': 500 },
+      },
+      expected: new AuditInfo({
+        userId: 'user-123',
+        clientTimestamp: new Timestamp({ seconds: 1700000000, nanos: 500 }),
+      }),
+    },
+    {
+      desc: 'skips non-numeric keys',
+      input: {
+        '2': 'Survey name',
+        notANumber: 'should be ignored',
+        foo: 42,
+      },
+      expected: new Survey({
+        name: 'Survey name',
+      }),
+    },
+    {
+      desc: 'skips unrecognized field numbers',
+      input: {
+        '2': 'Survey name',
+        '999': 'unknown field should be ignored',
+      },
+      expected: new Survey({
+        name: 'Survey name',
+      }),
+    },
     {
       desc: 'converts repeated message',
       input: {
@@ -171,8 +219,42 @@ describe('toMessage()', () => {
     },
   ].forEach(({ desc, input, expected }) =>
     it(desc, () => {
-      const output = toMessage(input, expected.constructor as Constructor<any>);
+      const output = toMessage(
+        input,
+        expected.constructor as Constructor<unknown>
+      );
       expect(output).toEqual(expected);
     })
   );
+
+  it('logs and skips fields whose conversion returns an Error', () => {
+    const debugSpy = spyOn(console, 'debug');
+    const output = toMessage(
+      {
+        '2': 'Survey name',
+        '4': 'not an object', // acl map expects object → Error
+      },
+      Survey
+    );
+    expect(output).toEqual(new Survey({ name: 'Survey name' }));
+    expect(debugSpy).toHaveBeenCalledWith(jasmine.any(Error));
+  });
+
+  it('returns Error when constructor is not a protojs message', () => {
+    class NotAProtoMessage {}
+    const output = toMessage({}, NotAProtoMessage as Constructor<unknown>);
+    expect(output).toEqual(jasmine.any(Error));
+    expect((output as Error).message).toContain('is not a protojs message');
+  });
+
+  it('returns Error when message type not found in registry', () => {
+    class UnknownProtoMessage {
+      static getTypeUrl(): string {
+        return '/ground.v1beta1.DoesNotExist';
+      }
+    }
+    const output = toMessage({}, UnknownProtoMessage as Constructor<unknown>);
+    expect(output).toEqual(jasmine.any(Error));
+    expect((output as Error).message).toContain('not found in registry');
+  });
 });

lib/src/proto-to-firestore.spec.ts

@@ -1,14 +1,14 @@
 /**
  * Copyright 2024 The Ground Authors.
  *
- * Licensed under the Apache License, Version 2.0 (the 'License');
+ * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
  *
  *     https://www.apache.org/licenses/LICENSE-2.0
  *
  * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an 'AS IS' BASIS,
+ * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
@@ -146,4 +146,54 @@ describe('toDocumentData()', () => {
       expect(output).toEqual(expected);
     })
   );
+
+  it('returns empty object for message with no fields set', () => {
+    expect(toDocumentData({})).toEqual({});
+  });
+
+  it('logs and drops fields whose value has an unsupported type', () => {
+    const errorSpy = spyOn(console, 'error');
+    const survey = new Survey({ name: 'Survey name' });
+    (survey as any).description = () => 'not a string'; // function is unsupported
+    const output = toDocumentData(survey);
+    expect(output).toEqual({
+      [s.name]: 'Survey name',
+      [s.state]: Survey.State.STATE_UNSPECIFIED,
+      [s.generalAccess]: Survey.GeneralAccess.GENERAL_ACCESS_UNSPECIFIED,
+      [s.dataVisibility]: Survey.DataVisibility.DATA_VISIBILITY_UNSPECIFIED,
+    });
+    expect(errorSpy).toHaveBeenCalledWith(jasmine.any(Error));
+  });
+
+  it('drops fields whose value is empty string', () => {
+    const output = toDocumentData(
+      new Survey({ name: '', description: 'desc' })
+    );
+    expect(output).toEqual({
+      [s.description]: 'desc',
+      [s.state]: Survey.State.STATE_UNSPECIFIED,
+      [s.generalAccess]: Survey.GeneralAccess.GENERAL_ACCESS_UNSPECIFIED,
+      [s.dataVisibility]: Survey.DataVisibility.DATA_VISIBILITY_UNSPECIFIED,
+    });
+  });
+
+  it('recursively converts array input', () => {
+    const output = toDocumentData([
+      new Coordinates({ latitude: 1, longitude: 2 }),
+      new Coordinates({ latitude: 3, longitude: 4 }),
+    ]);
+    expect(output).toEqual([
+      { [c.latitude]: 1, [c.longitude]: 2 },
+      { [c.latitude]: 3, [c.longitude]: 4 },
+    ]);
+  });
+
+  it('throws when message type is not registered', () => {
+    class UnknownMessage {
+      foo = 'bar';
+    }
+    expect(() => toDocumentData(new UnknownMessage())).toThrowError(
+      /Unknown message type UnknownMessage/
+    );
+  });
 });