Enhancement: BigDecimal support for assertEquals()

[pbenedict]

Using JUnit to test BigDecimal values is always a sore point. It is because BigDecimal considers precision using equals() but ignores it in compareTo(). The best way to handle this would be a new assertEquals method that allows precision to be optionally evaluated. You can also add an alternative method that accepts a message:

public static void assertEquals(BigDecimal expected, BigDecimal actual, boolean precisionMatters) {
if (precisionMatters) {
Assert.assertEquals(expected, actual);
} else {
Assert.assertEquals(0, expected.compareTo(actual));
}
}

[geoffreywiseman]

Would be an easy thing to put together as a fork or patch; I'd be willing to do it if appropriate.

[npryce]

I'd rather have a assertNumericallyEqualTo assertion than a boolean flag passed to assertEquals.

[pbenedict]

That's a good name. I would shorten it to assertNumericallyEquals().

[aisrael]

While I agree with the sentiment, I'm not sure having another assert method with a different name will really help clear the confusion especially among people who are totally unaware of the difference or haven't really encountered the issue themselves. I mean, if I hadn't read this issue I would've gone ahead and used assertEquals() for BigDecimal. In the general case, it would, in fact, work as advertised—it would work exactly how BigDecimal#equals() works. I would even argue that this is a good thing—it forces somebody to realize that BigDecimal#equals() compares both value and scale then maybe that wasn't what they were trying to accomplish—though, in other cases, I posit it is. I mean, in some cases we want to check that the actual value returned by a method is exactly 'equal' to the expected value (not just logically/numerically equal). Put it another way: what does assertNumericallyEquals() provide (aside from brevity) that assertEquals(0, actual.compareTo(expected)) doesn't? Personally, I'd probably create a Hamcrest matcher and go assertThat(actual, isNumericallyEqualTo(expected)) if I needed it often enough—plus, can be used anywhere Hamcrest matchers can be used (data validation, among others).

[dsaff]

Sorry for the late weigh-in, but my personal opinion is close to Alistair's.

[geoffreywiseman]

Put it another way: what does assertNumericallyEquals() provide (aside from brevity) that assertEquals(0, actual.compareTo(expected)) doesn't?

Communicates intention more clearly? The hamcrest matcher approach is fine with me.

[dsaff]

Geoffrey,

Are you saying that you're fine writing that matcher for your own project, or that you wish it shipped with hamcrest? Or JUnit, but not hamcrest?

[geoffreywiseman]

Any or all of the above -- most projects I write end up having some custom hamcrest matchers. Having common cases covered by Hamcrest and/or JUnit saves a little bit of time, but this also hasn't been a very common case for me.

[neekfenwick]

A small point about less useful error messages.. I've just converted my Assert.assertEquals calls to Assert.assertTrue having hit failures in comparing BigDecimals with Assert.assertEquals. Of course now my tests fail with a kind of "boolean test failed" error, without telling me what the values were. In the same way, assertEquals(0, actual.compareTo(expected)) would log e.g. "expected 0 but got: 1" when a far more useful error message would be from assertNumericallyEquals(actual, expected) would be "expected 12.45 but got: 123" (the actual values of 'actual' and 'expected' arguments).

http://www.bssd.eu/blog/?p=113 makes interesting reading also.

[dsaff]

assertEquivalent from https://github.com/KentBeck/junit/pull/228 should resolve this.

[neekfenwick]

Thanks dsaff. That work looks great. I've not done this before, does this mean I have to fetch the junit source, merge this pull request myself, and make a custom build?

From the help at http://help.github.com/send-pull-requests/ it seems I should:

git clone https://github.com/KentBeck/junit.git
cd junit

and then add a remote for the pull request, then either fetch, merge, or do both.

Given that this seems to be a pull request i.e. not official junit, is it wise to use in an application? Any chance this will moulder outside the main junit, and I'll have issues sending my code to others, for whom it won't build?

[dsaff]

Neekfenwick,

Yep, you're on the right track about how to do it now. I intend to merge that pull into the 4.10 branch, but the original author has been having trouble getting the git status mergeable. If you have time to do the world a favor, you could fork the KentBeck repo, get everything merged, and issue a pull request against the 4.10 branch, so that it would be easier for everyone (if it's sounding popular, I could even spin up sometime a 4.10 preview jar).

[neekfenwick]

I've really never done this before :) So I've forked the repo and cloned git@github.com:neekfenwick/junit.git to my local machine (I already have a github account and ssh keys set up), and added a remote for the repo I forked it from:

git remote add upstream https://github.com/KentBeck/junit.git

Just for good measure I've branched so I can merge the pull request into somewhere other than HEAD:

git branch merge_pullreq_228
git checkout merge_pullreq_228

Now the docs I can find say "now merge the pull request into your branch". I can see the commits for pull 228 at https://github.com/KentBeck/junit/pull/228/commits but I can't merge them:

[neek junit (merge_pullreq_228)]$ git merge 57b49344
fatal: '57b49344' does not point to a commit

Since the pull request isn't for my own repo/branch I can't use the Merge tools in the github web gui (AFAIK).

Am I correct in thinking you want me to merge the 7 commits into my own branch and get it to build/pass unit tests? Could you spell out what to do to to merge one of these commits to get me on my way?

[dsaff]

Right, you can't use the web merge tools. What I believe should work is to:

git remote add leet3lite https://github.com/leet3lite/junit.git

And then from your branch, call

git pull leet3lite master

Unfortunately, I usually forget one thing when describing how to use git "over the phone", so let me know if that works for you.

[neekfenwick]

Ah I see, I don't need to merge each of those commits.. leet3lite's master branch already has them on it, and it's the act of merging that work into the original master that's the problem.

There seem to be merge conflicts between that branch and HEAD. I guess that's the whole point of this exercise.

[neek junit (merge_pullreq_228)]$ git remote add leet3lite https://github.com/leet3lite/junit.git
[neek junit (merge_pullreq_228)]$ git pull leet3lite master
remote: Counting objects: 100, done.
remote: Compressing objects: 100% (39/39), done.
remote: Total 85 (delta 34), reused 77 (delta 26)
Unpacking objects: 100% (85/85), done.
From https://github.com/leet3lite/junit
 * branch            master     -> FETCH_HEAD
Auto-merging acknowledgements.txt
CONFLICT (content): Merge conflict in acknowledgements.txt
Auto-merging src/main/java/org/junit/Assert.java
Auto-merging src/test/java/org/junit/tests/AllTests.java
CONFLICT (content): Merge conflict in src/test/java/org/junit/tests/AllTests.java
Automatic merge failed; fix conflicts and then commit the result.
[neek junit (merge_pullreq_228|MERGING)]$ 

If that look sensible to you, I'll look at carrying on with this tomorrow. Almost midnight here.

[dsaff]

That looks like you're in the right place. acknowledgements.txt and AllTests.java get touched a lot, usually just with adds, so the merging operation is probably a simple act of including all the lines added on both paths.

Thanks a lot for pushing this forward.

[dsaff]

I think this was fixed 2 years ago.

[aaugusta]

dsaff - No...?

[jkolobok]

Not in 4.11 ;( I need this feature too

[kcooney]

The discussion on assertEquivalent() moved to #228 and after that, I think it moved to #376 where we decided that really Hamcrest should solve this.

@junit-team/junit-committers any objections to me closing this?

[marcphilipp]

No objections from my side.

[dsaff]

Oops, Freudian click. :-) No objections here.

[kcooney]

Okay. Closing then.

[ptahchiev]

I don't think using the hamcrest matcher's fixes this issue. For example this code:

   assertThat(product.getRating(), is(equalTo(new BigDecimal("2.3"))));

will produce this result:

Expected: is <2.3>
     but: was <2.30000>

I think junit or hamcrest still needs isNumericEquivalent method.

[marcphilipp]

Have you tried using comparesEqualTo?

[ptahchiev]

Yes, comparesEqualTo actually works. Thanks.

[marcphilipp]

Here's a cheat sheet I created some time ago:
http://www.marcphilipp.de/blog/2013/01/02/hamcrest-quick-reference/