Fix broken assert.equal assertion when not applied on a Collection #18
Conversation
…mutable collection These tests currently fail. The result is some false positives!
You wouldn’t expect that this is necessary, however `assert.equal` and the BDD version aren’t actually the same! The BDD version uses strict equality while the “assert” version doesn’t. TIL. This fixes the bug with testing equality of an Array and Immutable collection with the "assert" style.
|
Thanks, I'll take a look at this tomorrow! Sorry it got messy :-) |
|
Np. My original fix was 💩. |
| if (actual instanceof Collection) { | ||
| return new Assertion(actual).equal(expected); | ||
| } | ||
| else return assert.equal; | ||
| else return originalEqual(actual, expected); |
There was a problem hiding this comment.
You nailed it, actually! The issue isn't so much that I didn't call the original assertion, but that I was returning a function, which never throws anything. And of course, not storing the original assertion somewhere triggers an infinite loop.
Good job!
That's one more proof we need more tests...
astorije
changed the title
Fix broken assert.equal assertion when not applied on a Collection
|
👍 Sorry it took time and 2 PRs to merge. Do you think it would be better to add a Gitter channel, or discussing over PR comments seemed enough? |
|
Yeah, sorry I didn't see it the first time. When I went back to it with a little spare time it jumped right out. IMO Gitter would be overkill. Line comments are tied to the code in question and don't require immediate response. Thanks for merging this! |
|
Voilà, it's released. I realized too late that it was a good candidate for 1.1.2, not 1.2.0, grr... Oh well. |
|
Thanks! |
This is an updated version of #16. I dug into the problem a little deeper to get a better understanding and figured out the actual root cause (essentially, not calling the original
equalfunction.