-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
(complete #4858): actually change default FAIL_ON_NULL_FOR_PRIMITIVES
to true
#4890
(complete #4858): actually change default FAIL_ON_NULL_FOR_PRIMITIVES
to true
#4890
Conversation
Fixing tests there are more tests failing than I thought |
LOL. Did I actually forget to make the change. Oh my.... |
@JooHyukKim Did you verify that there were no unexpected problems w/ change, that is, all failures made sense? My concern is that with some of the changes we may cover bugs that were hidden by default settings. In this case basically quickly checking that the failure makes sense given defaults value change. |
@@ -52,7 +52,9 @@ public BooleanWrapper(@JsonProperty("ctor") Boolean foo) { | |||
public void setPrimitive(boolean v) { primitive = v; } | |||
} | |||
|
|||
private final ObjectMapper DEFAULT_MAPPER = newJsonMapper(); | |||
private final ObjectMapper DEFAULT_MAPPER = jsonMapperBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test failure for boolean[]
looks suspicious; I am not sure it should fail. Others in this package were legit, merged change in 2.19 to ensure consistent test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the only odd case, I think that's ok -- can investigate in future if necessary but good enough for now.
FAIL_ON_NULL_FOR_PRIMITIVES
to true
FAIL_ON_NULL_FOR_PRIMITIVES
to true
@JooHyukKim Thank you for doing this! I went through all changes and merged some via 2.19 (to try to unify tests for easier future merging). But I'll now merge this so defaults change actually takes effect... and we'll see how many downstream modules have test failures. I was wondering why none had: makes sense that no change had been made, in hindsight :) |
@JooHyukKim Ok, as expected some dependencies fail (actually, ones I'd expect to)
which is pretty much a set of modules that tend to show up failures on default changes. If you have time, help with these would be appreciated; I can tackle some of course. |
Yeah, will work on them tonight✌🏻 |
Excellent! |
Follow through of #4884