DoubleAssert.isEqualTo() behaviour change?

Most likely introduced by this change:

http://code.google.com/p/fest/source/diff?spec=svn1295&r=1295&format=side&path=/trunk/fest/fest-assert/src/main/java/org/fest/assertions/Assertions.java

Before, there were only assertThat() methods for primitive types, not for their wrappers.

Thus, writing assertThat((Double) null) would result in the compiler calling Assertions.assertThat(Object). This method returns ObjectAssert, which is capable of taking null as actual/expected value.

After this change, we have assertThat(Double actual), which returns a DoubleAssert. This assert class internally stores a primitive value, which gets initialized upon construction of the DoubleAssert object. Passing null to assertThat(...) will result in an assignment of null to a primitive double (java tries to performs auto-unboxing). This does not work for null, so we get the NPE observed by Stanislaw.

I'm going to try this: allow all subclasses of PrimitiveAssert to store references to the actual wrapper classes. This also helps a lot for FEST-197.

All Assert classes taking reference types as their actual value now now support the following code (will pass):

assertThat(null).isEqualTo(null);

Thanks for resolving this, Ansgar!

S.

In my opinion, the solution for this issue is to create wrapper-specific assertion classes, instead of changing the existing assertions on primitives. Changing the primitive assertions breaks existing functionality. I'm reopening this issue.

Hi Alex,

can you please show me the code/existing functionality that might potentially break? I'd like to write extensive tests to make sure each and every aspect of this "existing functionality" is covered by unit tests.

Regards
Ansgar

I switched FEST-Swing to use the latest FEST-Assert (when the fix for this bug was in place,) and I got compilation errors due to the casting that is done with primitives. If I remember correctly, this should work:

new DoubleAssert(8)

but with the fix, it doesn't, because now is treated as an Integer.

Regards,
-Alex

Actually, my comment is not correct. IMHO, the only thing that we need to add to your original fix is overloaded methods that take primitives. That will not break existing functionality.

That's also what I planned to do. Still, having tests to not break existing functionality is always an asset

So I'm going to do just that somwhen during the next few days and invite you and the other developers for a review.

BTW: can we go without PrimitiveAssert then, or should this class still be kept? Since we now base all the assert classes for primitive types on GenericAssert, we don't strictly need it any more, at least not as a base class for the primitive assert classes. Is PrimitiveAssert part of our public API, i. e. could it be that someone used it to derive his own assert classes from primitive assert?

Totally agree with you!

I think we can finally remove PrimitiveAssert and have a single hierarchy for all assertions. Yay!

Thanks!
-Alex

Oh BTW, there is a new class ComparableAssert, which is superclass of BigDecimalAssert. I think all assertions for numbers can extend this class and implement NumberAssert to minimize code duplication.

I recently found out you refactored all the unit tests. Seems like they are already structured according to certain "groups of checks". Looks very good, yet I immediately had the idea we might be able to reduce test code a lot. Here's my story...

A week ago or so, my colleagues and me were so lucky to view a presentation by J. B. Reinsberger (author of "JUnit Receipes") about testing http://www.infoq.com/presentations/integration-tests-scam. He advocated a testing style comprised of interaction tests and contract tests. Whenever two objects interact, put in interfaces between them, test each object in isolation with the other object(s) replaced by test doubles. I'll skip interaction tests. The second half of the testing strategy is what he calls "contract tests". They essentially make sure that each and every implementation in my system which claims to implement an interface actually obeys its contract..

I recognized there are probably similar patterns in FEST-Assert. In other words: I believe there are groups of assertion classes, which implement a number of interfaces, with the interfaces defining a contract.

"ComparableAssert" is, in my eyes, one of these contracts. It might also be termed an "abstract contract", or in Java terms "generic contract", because it can be applied to a number of concrete types, which of course must be comparable. I'm quite sure we can identify most of the assertions we currently support belong to a contract of some form. More examples could be: NullableAssert (isNull, isNotNull), GroupAssert (hasSize, isEmpty, isNotEmpty), ...

Last weekend, I drafted some experimental code which is intended to support systematic contract testing. It basically ensures that, whenever you define an interface (and annotate it as @Contract), all the implementors of this contract are tested to comply to the contract. This is done by creating a number of tests on dynamically (for TestNG at the moment, since we use TestNG at work), but I think it can easily be ported to JUnit; probably by defining a custom TestSuite). It aims at defining the test code once per abstract contract, and provide the relevant test data on a per-concrete-contract-basis. I. e. imagine:

interface ComparableAssert<T> as the abstract contract, and BigDecimalAssert implements ComparableAssert<BigDecimal> as an implementor of a concrete contract derived from ComparableAssert. In this setting, you'd define one (generic) test to check that each implementor obeys to ComparableAssert, and one test data factory for each concrete contract (e. g. ComparableAssert<BigDecimal>).

I'm planning to make this testing aid a github project once it's at least a bit useful. I'm not sure it works out for generic contracts, so no jumping for joy yet. What do you think?

Ansgar, I just love your idea! Really, really cool and useful!

I'm very curious to see how you generate tests dynamically...please ping me when the project is up!

Fixed in trunk. Alex is doing some cleanup as part of FEST-260, but this issue is already resolved.