Adding test utilities to create Value types #1158

schmidt-sebastian · 2020-01-23T16:58:43Z

This exposes a similar interface as wrap() and allows me to change the existing FieldValue test to use Value protos.

For reference: #1156

This will be used (and tested) in the follow-up CL that adds the FieldValue tpes.

This exposes a similar interface as wrap() and allows me to change the existing FieldValue test to use Value protos

codecov · 2020-01-23T17:07:18Z

Codecov Report

Merging #1158 into mrschmidt/rewritefieldvalue will increase coverage by 0.74%.
The diff coverage is 0%.

Flag	Coverage Δ	Complexity Δ
#FirebaseFirestore	`61.29% <0%> (+0.03%)`	`2279 <0> (ø)`	⬇️
#FirebaseFirestore_Ktx	`41.17% <ø> (ø)`	`0 <ø> (ø)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
...le/firebase/firestore/model/value/ProtoValues.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8d1ce8...063a256. Read the comment docs.

wilhuff · 2020-01-23T18:31:07Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

+import java.util.List;
+
+/** Test helper to create Firestore Value protos from Java types. */
+public class ValueHelper {


I'm not a huge fan of classes named with "Util" or "Helper". How about just Values? This is a convention found in Guava, where e.g. helpers for creating/manipulating lists are in Lists.

ValueHelperUtil it is :)

I renamed it to Values. FWIW, in most APIs that use this convention, the helper library and the main library sit in the same package, which we can't do here.

Lists is a great example because it's a collection of Guava utilities about java.util.List. In other cases where the library has a class and static utilities for dealing with it, it makes sense that they'd colocate them. I don't think that invalidates the static utilities as plurals convention.

See go/java-practices/utility-classes.md.

wilhuff · 2020-01-23T18:38:46Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

+    return Value.newBuilder().setMapValue(builder).build();
+  }
+
+  public static Value wrapRef(DatabaseId dbId, DocumentKey key) {


The rest of this class doesn't really have much to do with wrapping, but does create values. How about refValue or something like that?

wilhuff · 2020-01-23T18:39:18Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

+    if (o instanceof Value) {
+      return (Value) o;
+    } else if (o instanceof String) {
+      return (Value.newBuilder().setStringValue((String) o).build());


Extra parens, here and throughout.

Removed. Sorry, I was quite certain googleJavaFormat would do this for me.

wilhuff · 2020-01-23T18:40:21Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

+              com.google.protobuf.Timestamp.newBuilder()
+                  .setSeconds(timestamp.getSeconds())
+                  .setNanos(timestamp.getNanoseconds())
+                  .build())


This inner build() is unnecessary. Proto setters take messages or builders and automatically call build() for you.

Removed here and for GeoPoint.

wilhuff · 2020-01-23T18:41:23Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

+    } else if (o instanceof DocumentReference) {
+      return (Value.newBuilder()
+          .setReferenceValue(
+              "projects/projectId/databases/(default)/documents/"


projectId isn't a valid project identifier. Elsewhere we've just used "project", FWIW.

I stole this from somewhere (SQLiteSchemaTest) and fixed it there as well.

wilhuff · 2020-01-23T18:43:23Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

@@ -0,0 +1,98 @@
+// Copyright 2020 Google LLC


Note that you currently only need this for tests, but eventually, won't the UserDataConverter need this as well? Maybe put this in with regular sources?

I'd like to punt on this for now to figure out how much of this code I will need in UserDataConverter and where to put it. I added a TODO.

FWIW, I think it's nice that this is a separate class that just has these static, simple functions. Moving it into the UserDataConverter would muddy that considerably. I was proposing just moving the class wholesale into the same package, or maybe even merging with/renaming ProtoValues and putting it in model.

In any case, no action required for this PR for sure.

schmidt-sebastian · 2020-01-24T00:24:55Z

/test device-check-changed

wilhuff

LGTM

wilhuff · 2020-01-24T01:44:48Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

@@ -0,0 +1,98 @@
+// Copyright 2020 Google LLC


FWIW, I think it's nice that this is a separate class that just has these static, simple functions. Moving it into the UserDataConverter would muddy that considerably. I was proposing just moving the class wholesale into the same package, or maybe even merging with/renaming ProtoValues and putting it in model.

In any case, no action required for this PR for sure.

wilhuff · 2020-01-24T01:50:09Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/ValueHelper.java

+import java.util.List;
+
+/** Test helper to create Firestore Value protos from Java types. */
+public class ValueHelper {


Lists is a great example because it's a collection of Guava utilities about java.util.List. In other cases where the library has a class and static utilities for dealing with it, it makes sense that they'd colocate them. I don't think that invalidates the static utilities as plurals convention.

See go/java-practices/utility-classes.md.

wilhuff · 2020-01-24T01:51:58Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/Values.java

+      return Value.newBuilder().setStringValue((String) o).build();
+    } else if (o instanceof Blob) {
+      return Value.newBuilder().setBytesValue(((Blob) o).toByteString()).build();
+


nit: excess newline.

Sorry - I forgot to address this before merging. I fixed it in the follow up PR.

wilhuff · 2020-01-24T01:52:42Z

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/Values.java

-      return (Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build());
+      return Value.newBuilder().setArrayValue(list).build();
+    } else if (o instanceof Map) {
+      com.google.firestore.v1.MapValue.Builder builder =


nit: import MapValue?

google-oss-bot · 2020-01-24T05:48:49Z

@schmidt-sebastian: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests	`063a256`	link	`/test smoke-tests`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

schmidt-sebastian added 2 commits January 23, 2020 08:50

Adding Proto-based equality and comparison

5d3f44d

This will be used (and tested) in the follow-up CL that adds the FieldValue tpes.

Adding test utilities to create Value types

5d006f0

This exposes a similar interface as wrap() and allows me to change the existing FieldValue test to use Value protos

googlebot added the cla: yes Override cla label Jan 23, 2020

schmidt-sebastian requested a review from wilhuff January 23, 2020 16:58

schmidt-sebastian assigned wilhuff Jan 23, 2020

google-oss-bot added the size/M label Jan 23, 2020

schmidt-sebastian added api: firestore and removed size/M labels Jan 23, 2020

Merge branch 'mrschmidt/comparisons' into mrschmidt/testhelpers

8440ced

schmidt-sebastian changed the base branch from mrschmidt/rewritefieldvalue to mrschmidt/comparisons January 23, 2020 18:01

google-oss-bot added the size/M label Jan 23, 2020

wilhuff reviewed Jan 23, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jan 23, 2020

Address feeback

c315cdc

google-oss-bot added size/L and removed size/M labels Jan 23, 2020

schmidt-sebastian added 2 commits January 23, 2020 11:40

Address feeback

a6f880a

Merge branch 'mrschmidt/comparisons' into mrschmidt/testhelpers

529881b

google-oss-bot added size/M and removed size/L labels Jan 23, 2020

Review feedback

c8d1ce8

google-oss-bot added size/L and removed size/M labels Jan 23, 2020

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Jan 23, 2020

wilhuff approved these changes Jan 24, 2020

View reviewed changes

wilhuff removed their assignment Jan 24, 2020

wilhuff assigned schmidt-sebastian Jan 24, 2020

schmidt-sebastian changed the base branch from mrschmidt/comparisons to mrschmidt/rewritefieldvalue January 24, 2020 05:30

schmidt-sebastian added 3 commits January 23, 2020 21:31

Merge branch 'mrschmidt/rewritefieldvalue' into mrschmidt/testhelpers

02c0e78

Fix merge

e8c299a

feedback

063a256

schmidt-sebastian merged commit 94844bb into mrschmidt/rewritefieldvalue Jan 24, 2020

schmidt-sebastian deleted the mrschmidt/testhelpers branch February 7, 2020 00:22

firebase locked and limited conversation to collaborators Feb 24, 2020

Adding test utilities to create Value types #1158

Adding test utilities to create Value types #1158

Uh oh!

Conversation

schmidt-sebastian commented Jan 23, 2020

Uh oh!

codecov bot commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Jan 24, 2020

Uh oh!

wilhuff 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!

google-oss-bot commented Jan 24, 2020

Uh oh!

Uh oh!

codecov bot commented Jan 23, 2020 •

edited

Loading