Skip to content

Fix Firestore POJO tests #281

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

Merged
mikelehen merged 1 commit into from
Mar 14, 2019
Merged

Fix Firestore POJO tests #281

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions firebase-firestore/src/androidTest/java/com/google/firebase/firestore/POJOTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,19 +301,19 @@ public void testCantWriteNonStandardNumberTypes() {
pojo.bigIntegerValue = new BigInteger("0");
expectedErrorMessages.put(
pojo,
"Could not serialize object. BigInteger is not supported, please use an int, long, float or double (found in field 'bigIntegerValue')");
"Could not serialize object. Numbers of type BigInteger are not supported, please use an int, long, float or double (found in field 'bigIntegerValue')");
Copy link
Member

@rsgowman rsgowman Mar 14, 2019

Choose a reason for hiding this comment

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

So remember in that other PR? Where I was talking about over-specified error message checking in tests? :)

(This is still fine; no action required.)

Copy link
Contributor

@mikelehen mikelehen Mar 14, 2019

Choose a reason for hiding this comment

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

FWIW- As a reviewer, I actually like that I can just look at the tests to see the exact error message that comes out without worrying that we're missing a space somewhere or that some generated part of the message is coming out goofy.

Also, as a reviewer, I should actually check if the tests pass before merging PRs.
box_of_shame

(in any case, there's definitely a balance to be struck with these sorts of tests... if we feel they're too fragile, I don't mind revisiting)

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Mar 15, 2019

Choose a reason for hiding this comment

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

I actually am a big fan of having at least one place that checks the full error message. A lot of our error messages are composed of different parts, and it is very easy to omit words/spaces or put extra spaces when we do this.


pojo = new InvalidPOJO();
pojo.byteValue = 0;
expectedErrorMessages.put(
pojo,
"Could not serialize object. Byte is not supported, please use an int, long, float or double (found in field 'byteValue')");
"Could not serialize object. Numbers of type Byte are not supported, please use an int, long, float or double (found in field 'byteValue')");

pojo = new InvalidPOJO();
pojo.shortValue = 0;
expectedErrorMessages.put(
pojo,
"Could not serialize object. Short is not supported, please use an int, long, float or double (found in field 'shortValue')");
"Could not serialize object. Numbers of type Short are not supported, please use an int, long, float or double (found in field 'shortValue')");

for (Map.Entry<InvalidPOJO, String> testCase : expectedErrorMessages.entrySet()) {
expectError(() -> ref.set(testCase.getKey()), testCase.getValue());
Expand All @@ -337,14 +337,14 @@ public void testCantReadBigInteger() {

expectError(
() -> snap.get("bigIntegerValue", InvalidPOJO.class),
"Could not deserialize object. Deserializing to BigInteger is not supported (found in field 'bigIntegerValue')");
"Could not deserialize object. Deserializing values to BigInteger is not supported (found in field 'bigIntegerValue')");

expectError(
() -> snap.get("byteValue", InvalidPOJO.class),
"Could not deserialize object. Deserializing to Byte is not supported (found in field 'byteValue')");
"Could not deserialize object. Deserializing values to Byte is not supported (found in field 'byteValue')");

expectError(
() -> snap.get("shortValue", InvalidPOJO.class),
"Could not deserialize object. Deserializing to Short is not supported (found in field 'shortValue')");
"Could not deserialize object. Deserializing values to Short is not supported (found in field 'shortValue')");
}
}