Conversation
|
@schmidt-sebastian: The following tests failed, say
DetailsInstructions 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. |
|
/test connected-check-changed |
| 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')"); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
(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)
There was a problem hiding this comment.
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.
|
Since Sebastian is in a different timezone and tests are broken without this, I'm merging this on his behalf again (I haven't learned my lesson apparently). somke-tests are failing but I'm confident it's unrelated to this change. |
5 participants
Looks like #272 broke the Firestore Integration Tests, which I noticed in the failures for #277.