clojure-expectations / expectations

A minimalist's unit testing framework ("classic" version)
https://clojure-expectations.github.io
BSD 3-Clause "New" or "Revised" License
396 stars 25 forks source link

Trouble migrating to v2 (2.0.7) #40

Closed ptaoussanis closed 9 years ago

ptaoussanis commented 10 years ago

Hi Jay,

I'm seeing an unexpected result when migrating Nippy's tests from Expectations 1.4.56 to 2.0.7.

Tests pass using 1.4.56 but are failing in 2.0.7 with:

failure in (main.clj:17) : taoensso.nippy.tests.main
(expect test-data ((comp thaw freeze) test-data))
<large map>
in expected, not actual: null
in actual, not expected: null

Where a quick check at the REPL shows that (= test-data ((comp thaw freeze) test-data)) does actually hold.

Any idea what the trouble may be?

For reference, the Nippy test data are intentionally quite elaborate: https://github.com/ptaoussanis/nippy/blob/master/src/taoensso/nippy.clj#L634

Thanks a lot! Have a great weekend, cheers :-)

jaycfields commented 10 years ago

I've seen something similar in the past where a value in one of the maps was an int and the same value was a long in the other map. The null, null error message is the result of using clojure.data/diff, which doesn't pick up that difference.

I'll check out the src and toy with it to find the specific issue. I should have an answer by Monday.

Cheers, Jay

ptaoussanis commented 10 years ago

Cool, no rush at all - was reporting in case it may be a bug. Thanks again for the great lib, cheers!

jaycfields commented 10 years ago

I think it is a bug in expectations. This is the line that causes the failure: https://github.com/ptaoussanis/nippy/blob/fb617c4df8986f33bef44616245afdcfa0aacdb7/src/taoensso/nippy.clj#L782

I'm not sure why things are no good for records, I know I have that case explicitly tested. I'll keep digging and let you know when it's sorted.

ptaoussanis commented 10 years ago

Thanks a lot Jay, much appreciated. Again, absolutely no urgency - please take your time. Cheers! :-)

jaycfields commented 9 years ago

Good news, bad news. I was able to reproduce the error. I brought nippy to the expectations project, created a record, (comp thaw freeze), and got a failure (at the command line). I ran the same code in emacs and it passed. I printed the (= x ((comp thaw freeze) x)) result while running expectations and it was false. So, something is causing it to not be equal during runs, but be equal in the repl. I'm guessing it's some Type issue that's not easy to quickly see.

However, I changed the impl of expectations https://github.com/jaycfields/expectations/commit/7951e00873ffb378eb3c9f72265dda03f6f047e0 and the problem "went away".

Version 2.0.8 is up on clojars and should work perfectly for you.

Cheers, Jay

jaycfields commented 9 years ago

Can you let me know when you test, please? I wont close the issue till you do.

ptaoussanis commented 9 years ago

Hi Jay,

Thanks a lot for your efforts on this! Would it be okay if I get back to you at the weekend? Just juggling a few urgent tasks atm.

jaycfields commented 9 years ago

Sounds good, thanks Peter.

ptaoussanis commented 9 years ago

Okay Jay - can confirm that tests are passing on my end with v2.0.8.

Can't contribute anything on the actual cause except to confirm your observations on Record equality. Thought this might have something to do with 1.6's changes to hash equality but the behaviour seems consistent with at least Clojure 1.4 through 1.6.

Will let you know if I see any other unexpected behaviour, but looks good so far.

Thanks again for taking the time to look into this. Enjoy the rest of your weekend, cheers! :-)

jaycfields commented 9 years ago

Great, thanks Peter