-
Notifications
You must be signed in to change notification settings - Fork 391
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
Added missing test for java conversions #334
Conversation
Codecov Report
@@ Coverage Diff @@
## master #334 +/- ##
==========================================
+ Coverage 86.55% 86.63% +0.07%
==========================================
Files 335 335
Lines 10751 10755 +4
Branches 354 565 +211
==========================================
+ Hits 9306 9318 +12
+ Misses 1445 1437 -8
Continue to review full report at Codecov.
|
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.
Try testing null values as well
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.
I meant to test for null values inside maps themselves, e.g. j.put("test-null", null: java.lang.Double)
(btw, property based testing is ideal for this kind of tests)
My assumption was that this test should cover only the basic functionality (what it does by now). By looking at the coverage you can clearly see that all lines are now covered. I don't think that exploding the scope of it makes much sense as there are also different things to focus on. |
well, the code that was introduced in #333 now clearly has a problem which I wanted you discover through the test: import com.salesforce.op.features.types._
import collection.JavaConverters._
val jm = new java.util.HashMap[String, java.lang.Boolean]()
jm.put("nullValue", null)
jm.toBinaryMap // NullPointerException ! |
I propose to add a test for null values and modify the code from #333 with following: def toBinaryMap: BinaryMap = new BinaryMap(
Option(v).map(_.asScala.collect { case (k, v) if v != null => v.booleanValue() }.toMap).getOrElse(Map.empty)
) |
I am aware of that and assumed that you are fine with this by merging my PR. |
Clearly we need to fix it as we don't want to have any NPE flying around. |
Hmmm I wouldn't say that |
Replacing with default values would be even worse. Let's have then explicit handling for null values as follows, e.g. for RealMap: def toRealMap: RealMap = new RealMap(
Option(v).map(_.asScala.map {
case (k, null) => (k, null.asInstanceOf[Double])
case (k, x) => (k, x.doubleValue())
}.toMap).getOrElse(Map.empty)
) |
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.
lgtm!
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Matthew <m***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request. |
Thanks for the contribution! Before we can merge this, we need @wsuchy to sign the Salesforce.com Contributor License Agreement. |
No description provided.