Skip to content

Drop FieldValue from tests #1216

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.

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

Merged
merged 4 commits into from
Feb 7, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

  • Drops the FieldValue test classes
  • wrap() returns Value and replaces valueOf() test helper
  • Moved FieldValueTest.testValueEquality(), FieldValueTest.testValueOrdering() and FieldValueTest.testCanonicalIds() to a new ProtoValuesTest files that test the ProtoValues functionality directly (via a small FieldValue-alike helper)

This is the last cleanup PR before I want to merge to master. We can discuss if we should proceed with the suggested renames as well.

@googlebot googlebot added cla: yes Override cla labels Feb 6, 2020
@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/rewritefieldvalue to master February 6, 2020 22:44
@schmidt-sebastian schmidt-sebastian changed the base branch from master to mrschmidt/rewritefieldvalue February 6, 2020 22:44
@schmidt-sebastian schmidt-sebastian changed the title Drop FieldValue Drop FieldValue from tests Feb 6, 2020
@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #1216 into mrschmidt/rewritefieldvalue will increase coverage by 0.01%.
The diff coverage is n/a.

Flag Coverage Δ Complexity Δ
#FirebaseFirestore 61.93% <ø> (+0.01%) 2226 <ø> (+1) ⬆️
#FirebaseFirestore_Ktx 41.17% <ø> (ø) 0 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
.../com/google/firebase/firestore/UserDataWriter.java 50% <0%> (+1.92%) 16% <0%> (+1%) ⬆️

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 7b0925f...1aae04d. Read the comment docs.

Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -31,9 +31,9 @@
@Config(manifest = Config.NONE)
public class ObjectValueBuilderTest {
private String fooString = "foo";
private Value fooValue = valueOf(fooString);
private Value fooValue = TestUtil.wrap(fooString);
Copy link
Contributor

Choose a reason for hiding this comment

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

Static import TestUtil.wrap (to match other usages above).

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

}

@Test
public void testEncodesNull() {
FieldValue value = wrap(null);
Value value = wrap(null);
com.google.firestore.v1.Value proto = valueBuilder().setNullValueValue(0).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace fully qualified com.google.firestore.v1.Value declarations in here with just Value?

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

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Feb 7, 2020
@schmidt-sebastian schmidt-sebastian merged commit bda327b into mrschmidt/rewritefieldvalue Feb 7, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/tests branch February 8, 2020 01:32
@firebase firebase locked and limited conversation to collaborators Mar 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants