Added missing test for java conversions

[wsuchy]

No description provided.

[codecov]

Codecov Report

Merging #334 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
...ala/com/salesforce/op/features/types/package.scala	57.93% <100%> (+6.86%)	⬆️

---

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 9fee4d4...40b46bb. Read the comment docs.

[tovbinm]

Try testing null values as well

[tovbinm]

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)

[wsuchy]

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.

[tovbinm]

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 !

[tovbinm]

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)
)

[wsuchy]

I am aware of that and assumed that you are fine with this by merging my PR.

[tovbinm]

Clearly we need to fix it as we don't want to have any NPE flying around.

[wsuchy]

Hmmm I wouldn't say that nulls must be implicitly omitted. Why not convert them to false in Boolean and 0 in Integer?
Or maybe leave exceptions as a suggestion for the user that there is a problem with the data?

[tovbinm]

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)
)

[tovbinm]

lgtm!

[salesforce-cla]

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.

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @wsuchy to sign the Salesforce.com Contributor License Agreement.