tj / should.js

BDD style assertions for node.js -- test framework agnostic
MIT License
2.75k stars 195 forks source link

instanceof(Date) fails for dates #65

Closed mattness closed 11 years ago

mattness commented 12 years ago

Problem

Using should v0.6.3, the instanceof() method returns an incorrect result when called on a Date object and passed Date as its argument.

Expected behavior

Calling instanceof(Date) on a Date object should not throw an AssertionError.

Steps to reproduce

$ node
> require('should');
{ [Function: ok]
  AssertionError: 
   { [Function: AssertionError]
     super_: 
      { [Function: Error]
        captureStackTrace: [Function: captureStackTrace],
        stackTraceLimit: 10 } },
  fail: [Function: fail],
  ok: [Circular],
  equal: [Function: equal],
  notEqual: [Function: notEqual],
  deepEqual: [Function: deepEqual],
  notDeepEqual: [Function: notDeepEqual],
  strictEqual: [Function: strictEqual],
  notStrictEqual: [Function: notStrictEqual],
  throws: [Function],
  doesNotThrow: [Function],
  ifError: [Function],
  version: '0.6.3',
  exist: [Function],
  not: { exist: [Function] },
  Assertion: [Function: Assertion] }
> (new Date()).should.be.an.instanceof(Date);
AssertionError: expected 1335798857282 to be an instance of Date
    at Object.<anonymous> (/home/gollom/projects/surveycalc-node/node_modules/should/lib/should.js:355:10)
    at repl:1:27
    at REPLServer.eval (repl.js:80:21)
    at repl.js:190:20
    at REPLServer.eval (repl.js:87:5)
    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)

Work-around

$ node
> require('should')
{ [Function: ok]
  AssertionError: 
   { [Function: AssertionError]
     super_: 
      { [Function: Error]
        captureStackTrace: [Function: captureStackTrace],
        stackTraceLimit: 10 } },
  fail: [Function: fail],
  ok: [Circular],
  equal: [Function: equal],
  notEqual: [Function: notEqual],
  deepEqual: [Function: deepEqual],
  notDeepEqual: [Function: notDeepEqual],
  strictEqual: [Function: strictEqual],
  notStrictEqual: [Function: notStrictEqual],
  throws: [Function],
  doesNotThrow: [Function],
  ifError: [Function],
  version: '0.6.3',
  exist: [Function],
  not: { exist: [Function] },
  Assertion: [Function: Assertion] }
> (new Date() instanceof Date).should.be.true;
{ obj: true }
TooTallNate commented 12 years ago

This was probably my .valueOf() patch that broke it. I guess the solution is gonna be to revert that and only use valueOf() where explicitly needed (like equal()).

kivikakk commented 12 years ago

+1, this breaks commander.js's tests.

Even so, using it in equal(), true() and false() (which appear to be the candidates for use) still cause existing test cases to break on node ~0.6 (and I guess anything < 0.7.7).

kivikakk commented 12 years ago

V8 is doing my head in with this.

It's very tricky to distinguish whether or not a given object has been wrapped by Object in the newer versions or not; this.constructor still looks the same as it normally would, only typeof this returns "object" instead of e.g. "string" for a string.

So far the only way to "reliably" detect this appears to be a hack of checking that the constructor of the object appears to be a primitive type that might be wrapped, and then seeing if it was wrapped by looking at the typeof.

66 is a pull request with this change; I'm not suggesting you definitely should take this route, but at least we have all tests passing on node ~0.6 and 0.7.8 with this.

leecookson commented 12 years ago

Need to fix this...more packages will fail when upgrading to the latest version. Found a unit test in sentientwaffle/gift that worked with should.js 0.3.0), but fails with 0.6.3 I had to use the workaround for my fork.

scott2449 commented 12 years ago

It is most definitely caused by the valueOf when should is defined and is still in all versions after 0.6.something

At the repl:

test = new Date() Wed Jul 18 2012 11:00:31 GMT-0400 (EDT) test instanceof Date true Object(test).valueOf() 1342623631108

it should be returning 'Date'

TooTallNate commented 12 years ago

+1

Guuz commented 12 years ago

I'm using the latest version of Should and Node.js and this is still an issue. Any updates?

missinglink commented 11 years ago

+1

NatalieWolfe commented 11 years ago

+1

skovalyov commented 11 years ago

+1

tj commented 11 years ago

+5

fakewaffle commented 11 years ago

+5

TooTallNate commented 11 years ago

I don't get it... if you guys want this feature, somebody write a patch.

IMO though you shouldn't be testing for instanceof ever though (except for in constructors). Consider a Date object returned from another VM context, it would fail this check.