Use valueOf(); fixes test cases on node v0.7.7 #56

Merged
merged 3 commits into from Mar 31, 2012

Conversation

Projects
None yet
5 participants
Contributor

TooTallNate commented Mar 31, 2012

Hey TJ.

So Should.js broke on node v0.7.7 apparently. Tests were failing in a weird way. See the commit message of the first commit. Was easy enough to fix in a backwards-compatible way :)

Cheers!

TooTallNate added some commits Mar 31, 2012

@TooTallNate TooTallNate use valueOf() to get the reference the object.
It seems as though new versions of V8 (as of node v0.7.7) will return an
Object version of a primitive when it is referenced using "this", something
silly like this would fail:

  var called = false
  called.should.equal.false

  AssertionError: expected {} to be false
7872dc6
@TooTallNate TooTallNate remove 'should' from the package.json 51d845a
@TooTallNate TooTallNate fix test case for 'should' not being installed f88a3f4

tj commented on 7872dc6 Mar 31, 2012

ah interesting, yeah makes sense actually to give you the boxed primitive, interesting

Hey @TooTallNate, can you explain this change? I am really interested about why this should be better :)

Contributor

TooTallNate replied Apr 12, 2012

See #56

Contributor

lackac replied May 16, 2012

Unfortunately this breaks assertions on Date objects:

> new Date()
Wed, 16 May 2012 13:27:57 GMT
> Object(new Date()).valueOf()
1337174879014
> new Date() instanceof Date
true
> new Date().should.be.instanceof(Date)
AssertionError: expected 1337174886085 to be an instance of Date
    at Object.<anonymous> (/Users/LacKac/Code/sspinc/cartman-api/node_modules/should/lib/should.js:355:10)
    at repl:1:23
    at REPLServer.eval (repl.js:80:21)
    at Interface.<anonymous> (repl.js:182:12)
    at Interface.emit (events.js:67:17)
    at Interface._onLine (readline.js:162:10)
    at Interface._line (readline.js:426:8)
    at Interface._ttyWrite (readline.js:603:14)
    at ReadStream.<anonymous> (readline.js:82:12)
    at ReadStream.emit (events.js:88:20)

tj merged commit 9dd895a into tj:master Mar 31, 2012

Contributor

gabrielfalcao commented Apr 4, 2012

Sorry for the silly question, but I have no idea of what is the diffence between using Object(this).valueOf() or no.
Would you mind pointing out where to learn more about it?

Thanks

Owner

tj commented Apr 4, 2012

@gabrielfalcao when you invoke a method or property on a primitive like false it has to "box" it, so it wraps it with Boolean object in this case which has a Boolean.prototype etc, it's transparent to the developer though, but for example:

> false == false
true
> new Boolean(false) == new Boolean(false)
false
Contributor

gabrielfalcao commented Apr 4, 2012

Oh wow. Thank you so much for taking time for writting that explanation

Owner

tj commented Apr 4, 2012

np! actually a better example is:

> Boolean.prototype.foo = function(){ return this }
[Function]
> true.foo() == true.foo()
false
Contributor

gabrielfalcao commented Apr 4, 2012

awesome, I got it. So this pull request makes it work like this:

> Boolean.prototype.foo = function(){return Object(this).valueOf();}
[Function]
> true.foo() == true.foo()
true

I grasped, thank you man. :2: 🌈 It's a double rainbow :)

@skovalyov skovalyov added a commit to skovalyov/should.js that referenced this pull request Feb 5, 2013

@skovalyov skovalyov Fix should.be.instanceOf() failure on Date
* Improvement to #56 to address should.be.instanceOf() failure on Date
reported at #65
* More tests for should.be.instanceOf()
035b3ba

@notatestuser notatestuser pushed a commit to notatestuser/gift that referenced this pull request Feb 19, 2014

@feugy feugy - Repo.commit: send stdout/stderr to callback lile other methods
- Tests: use a working 'remotes' fixture. Old one was broken.
- Tests: use chai instead of should: same API, but work properly on Date:
  tj/should.js#56
- Tests: use rimraf instead of executing "rm -rf": allow to work on windows
- Tests: configure mocha to execute coffee-script with --compilers option
1ed5081
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment