Adding valueOf to an object makes should fail

[moll]

And now from the top, in 4, 3, 2...:

> var Should = require("should")
> var Foo = function() {}
> Foo.prototype.valueOf = function() { return 1337 }
> new Foo().should.be.instanceOf(Foo)
AssertionError: expected 1337 to be an instance of

And now for an encore, line 75 is to be given a slap — == forces the right hand side to be valueOffed, so there's not much sense in that comparison:

> var foo = new Foo()
> foo.valueOf() == foo
true

And for the bang:

> foo.should.have.property("leetness")
AssertionError: expected 1337 to have a property 'leetness'

^ Which is just insulting.

[tvjg]

I also encountered this and was surprised by it. In brief, I have a type with two properties for accessing a buffer and its decoded string representation respectively. Like so:

function Utf8EncodedNumber (data) {
  this.rawData = data; 
  this.text = data.toString('utf8');
}
Utf8EncodedNumber.prototype.valueOf = function () { return parseInt(this.text, 10); }

Since the consumer of my library is likely concerned only with the decoded/parsed value, I had hoped to implement valueOf and allow created objects to be type-coerced for comparisons and arithmetic. When I updated my unit tests, I encountered the same issue as @moll.

While instanceOf isn't behaving as I expect, this same coercive comparison in the Object.prototype.should get accessor allows for:

var num = new Utf8EncodedNumber(dataRepr13);
num.should.eql(13);

Until I encountered this issue, I previously assumed the coercion above would occur in the eql method. But it's actually the reverse. The comparison that matters happens on line 31 of eql.js using ===. In my example, when it reaches this point, num has already been cast to 13. A preceding comment in eql.js; however, specifies the comparison be handled by ==. This guideline appears to be quoted from a CommonJS spec which describes how an assert implementation should test equality.

First, I tried simplifying should to return new Assertion(this) on line 75. This broke many unit tests. Since should is a property defined on Object.prototype, my current understanding is that this in the context actually refers to the __proto__ property of the object under test, not necessarily the object itself. When the current conditional is tripped, the valueOf call unboxes to the underlying primitive. Digging deeper, I found this adjustment first appeared in commit 7872dc6 and is refined in commit e550541 for handling Date objects. This led to a second attempt:

var textRepr  = toString.call(this);
var notDate   = (textRepr != '[object Date]');
var notObject = (textRepr != '[object Object]');
return new Assertion(notDate && notObject ? this.valueOf() : this);

This restores the expected behavior of instanceOf and all unit tests, but breaks my earlier test case num.should.eql(13). If I update, line 31 to use ==, it breaks a unit test. It seems then that this is a unit tested design choice even though it doesn't match the accompanying comment/spec. I'd like to see the behavior changed so that eql allows type-coercion, but I don't know if others depend on the strict comparison. Accordingly, I've created two separate pull requests. As long as the original issue opened here is fixed, I can always call valueOf in my unit tests to explicitly coerce types, which might be the best practice anyway.