Skip to content
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

Specific commutativity and associativity tests for BigDecimal #3325

Merged

Conversation

travisbrown
Copy link
Contributor

Before the fix in #3303 we were seeing occasional failed tests on Scala 2.13, and we haven't seen any since then, but I should have added some tests that specifically highlighted the fix, to make sure the instance don't accidentally get changed back, etc.

In this PR I've added two specific cases where commutativity and associativity didn't hold for the old |+| for BigDecimal on Scala 2.13, which simply called the standard library's +:

import java.math.MathContext

val one = BigDecimal("1", MathContext.DECIMAL32)
val small = BigDecimal("0.00001111111", MathContext.DECIMAL32)
val xs = one :: List.fill(10)(small)

And then:

scala> xs.reduceLeft(_ + _)
res0: scala.math.BigDecimal = 1.000110

scala> xs.reduceRight(_ + _)
res1: scala.math.BigDecimal = 1.000111

And for commutativity:

scala> import java.math.MathContext
import java.math.MathContext

scala> val one = BigDecimal("1")
one: scala.math.BigDecimal = 1

scala> val small = BigDecimal("1e-7", MathContext.DECIMAL32)
small: scala.math.BigDecimal = 1E-7

scala> one + small
res0: scala.math.BigDecimal = 1.0000001

scala> small + one
res1: scala.math.BigDecimal = 1.000000

@codecov-io
Copy link

Codecov Report

Merging #3325 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3325   +/-   ##
=======================================
  Coverage   93.31%   93.31%           
=======================================
  Files         378      378           
  Lines        7684     7684           
  Branches      233      233           
=======================================
  Hits         7170     7170           
  Misses        514      514
Flag Coverage Δ
#scala_version_212 93.38% <ø> (ø) ⬆️
#scala_version_213 93.09% <ø> (ø) ⬆️

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 2e12f4d...dc5d48e. Read the comment docs.

@travisbrown travisbrown added this to the 2.2.0-M1 milestone Mar 20, 2020
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Nice. I like this style of property tests and specific examples of things that have bitten us.

@travisbrown travisbrown merged commit 22fc006 into typelevel:master Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants