Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Assertions for unexpected values (undefined and NaN) #110

Closed
wants to merge 3 commits into from

3 participants

@lfborjas

As I understood the semantics of assert.propertyVal,  it should simply check if a given key has a given value associated. But it doesn't behave that way when the value is undefined or NaN.

That is, the following assertions fail:

assert.propertyVal({evil: undefined}, 'evil', undefined)
assert.propertyVal({camus: NaN}, 'camus', NaN)

But they shouldn't. This PR addresses that (or, rather, tries to :) )

lfborjas added some commits
@lfborjas lfborjas Allows assertion of keys with expected undef vals
The following

  assert.propertyVal({evil: undefined}, 'evil', undefined)

Should work instead of saying that the object doesn't have an `evil`
key.
12c3a20
@lfborjas lfborjas Allows to check for keys associated to NaN
The following should not raise an assertion exception

    assert.propertyVal({camus: NaN}, 'camus', NaN)

But, since [NaN is not equal to itself as it ought to
be](http://stackoverflow.com/questions/1565<t_co>4/what-is-the-rationale-for-all-comparisons-returning-false-for-ieee754-nan-values)
the previous assertion wasn't behaving as expected.

Now, I'm not sure that the weird semantics of `NaN` should allow for
such a thing, but I'm inclined to believe that, if anyone wants to
assert that they're expecting `NaN` to be associated to a value, its
startling nature shouldn't get in the way.

 
4e2ca5f
@domenic
Owner

Hmm, so this would be a much more insidious problem throughout Chai; e.g. assert.equal. We also have the problem that -0 === +0. See harmony:egal for some info here.

@logicalparadox, thoughts?

Hmm, I see, so it boils down to deciding what this and other assertions for equality in Chai mean: if they ought to represent that two objects are not "observably distinguishable" or to have the same meaning we expect from the strict equality operator. I, for one, advocate for the principle of least astonishment, but, then again, I'm a javascript amateur so other people might not be as astonished as me.

Firstly, there is a scoping problem with your double nested eql -> isNaN. It looks like you are always testing a.

But that may not matter as I feel the egal approach is more appropriate as it covers all our bases. Here is my demo:

/**
 * test.js
 */

function eql (x, y) {
  if (x === y) {
    // 0 === -0, but they are not identical
    return x !== 0 || 1 / x === 1 / y;
  }

  // NaN !== NaN, but they are identical.
  // NaNs are the only non-reflexive value, i.e., if x !== x,
  // then x is a NaN.
  // isNaN is broken: it converts its argument to number, so
  // isNaN("foo") => true
  return x !== x && y !== y;
}

console.log('"x", "y"              ', eql('x', 'y'));
console.log('"x", "x"              ', eql('x', 'x'));
console.log('+0, -0                ', eql(+0, -0));
console.log('-0, -0                ', eql(-0, -0));
console.log('+0, +0                ', eql(+0, +0));
console.log('0, 0                  ', eql(0, 0));
console.log('1, -1                 ', eql(1, -1));
console.log('1, 1                  ', eql(1, 1));
console.log('NaN, NaN              ', eql(NaN, NaN));
console.log('undefined, undefined  ', eql(undefined, undefined));
console.log('null, null            ', eql(null, null));
console.log('undefined, null       ', eql(undefined, null));

Output:

λ ~ → node test.js 
"x", "y"               false
"x", "x"               true
+0, -0                 false
-0, -0                 true
+0, +0                 true
0, 0                   true
1, -1                  false
1, 1                   true
NaN, NaN               true
undefined, undefined   true
null, null             true
undefined, null        false

@domenic - This seem right to you?

Also, as a small reminder please follow Chai's Coding Style Guide.

Thanks for the coding style guide! :)

Regarding the scoping problem, indeed, that thing I wrote is totally incorrect, I should have added the following cases

    err(function () {
      assert.propertyVal(simpleObj, 'foo', NaN);
    }, "expected { foo: 'bar' } to have a property 'foo' of NaN, but got 'bar'");

    err(function () {
      assert.propertyVal(evilObj, 'dubious', 42);
    }, "expected { gone: undefined, dubious: NaN } to have a property 'foo' of 42 but got NaN");

to realize that, with that faulty implementation, the first one fails.

Regarding the egal issue, I agree with you and @domenic that that's a good approach; I was confused for a bit about the -0 shouldn't be egal to 0, but it makes sense, as a developer might expect a property to be -0 and not +0 (even when they are, in fact, the same number, right?).

Moreover, do you think it correct/desirable that the semantics of all equality assertions in Chai (I can only think of property and equal, perhaps eql too?) should follow those of egal as opposed to strict javascript equality? (if so, I think a separate pull request that deals with that issue would be better, no?).

Sure, that's fine.

Neat, I think I'll revert this commit in this pull request and squash the other two together to make this one only deal with the undefined issue, and then I'll submit a separate PR for the egal stuff.

I'm just dithering as to what'll happen to this thread if I revert + squash + force-push, though.

@domenic
Owner

I think this misses the following case:

err(function () {
  assert.propertyVal(evilObj, 'foo', undefined);
}, "expected { gone: undefined } to have a property 'foo'");

I see an in operator in your future.

Ha, it does! Good catch! Checking it out.

Addressed in 41730a6 , steering away from in in favor of the old and reliable hasOwnProperty to avoid cases where we Cannot use 'in' operator to search for 'length' (quoth my test suite output, lol)

Owner

Wait, now what about the case

var evilObj2 = Object.create(evilObj);
assert.propertyVal(evilObj2, 'gone', undefined);

Dang it, true, I didn't even remember Object.create was a thing we were allowed to use now!

Hmm, I'm not sure about doing

var wants_undefined_val = arguments.length >= 2 && val === undefined && name in obj;

because I'm actually not privy as to when it's safe to use in. If I check for name in obj before undefined === val, all hell breaks loose in various tests that are looking for the property length to have a certain value. Thoughts?    

Owner

I don't really get the "length" issue; can you explain it a bit more?

Ok, what I saw when running the test suite, and having that statement with the clause order I told you about, the tests for lengthOf started to fail because the lengthOf assertion uses property to check if an object has a length property, and that in turn meant me doing stuff like "length" in 'foo' .

Owner

Ahh the issue is that you can't use in for non-objects. Interesting. Easy fix: name in Object(obj).

:heart: that solution. I rather do that than have a weird order of logical clauses, haha.

Ok, have a look at c5aef16 , sorry for so many commits dealing with the same thing!

Owner

Love it :D. We'll wait for @logicalparadox to comment on the NaN issue before merging. In the meantime feel free to interactive-rebase and force-push to this branch in order to get the undefined stuff down to a single commit.

On it! :D

the phrase "interactive-rebase and force-push" made me think of "just attackclone the grit repo pushmerge, then rubygem the lymphnode js shawarma module – and presto!", lol, I'm the worst.

Done it; squashed all three errata-commits into one, didn't want to reorder the original two and squash further to not loose these threads, though.

@domenic

Nit: camelCase in JavaScript.

I like my snakes to scream thank you.

(bad joke, that's not even screaming snake case :( )

@lfborjas lfborjas Checks for an undefined val iff the key is present
So we don't go around doing weird stuff like

    assert.propertyVal({foo: 'bar'}, 'not_there', undefined)

And having it pass!
6db3335
@logicalparadox

Any word on this?

@lfborjas

Yup, I haven't had the chance to come back to this since last week, the last I wrote was this commit lfborjas@f2f8111 and for some reason this:

assert.equal(NaN, NaN);

crashes silently: it doesn't even reach the line where _.sameValue is invoked; any pointers as to what it is and why that might be happening?  

@logicalparadox

What do you mean by "crashes silently"? That should pass. Does it fail?

@lfborjas

Hence my confusion then: it doesn't pass nor fail, it doesn't even invoke the assertion in the assertEqual function; that is, it doesn't reach this line upon execution: https://github.com/lfborjas/chai/blob/f2f811138cb25d90677aa5f78a30032e7374dfd3/lib/chai/core/assertions.js#L375 (other assertions do, btw)

@logicalparadox

I think you mean to call assert.strictEqual.

Edit: explanation via code

@lfborjas

Sorry, not following (my bad, I'm changing contexts between js-in-chai and python-at-work so I have snakes on the head, lol). Do you mean that it is assert.strictEqual instead of assert.equal the one that should use _.sameValue for comparisons of the actual and expected values?

@logicalparadox

You should be able to change your tests to assert.strictEqual(NaN, NaN) and they will function as expected.

Basically, to maintain feature parity with node.js' include assertions, assert.equal must do only == testing with no special sauce. If you do assert.strictEqual() that is the equivalent of using expect(NaN).to.equal(NaN), which is where your _.sameValue comes into play.

@lfborjas

Aha, gotcha, will look into that then; thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 22, 2012
  1. @lfborjas

    Allows assertion of keys with expected undef vals

    lfborjas authored
    The following
    
      assert.propertyVal({evil: undefined}, 'evil', undefined)
    
    Should work instead of saying that the object doesn't have an `evil`
    key.
  2. @lfborjas

    Allows to check for keys associated to NaN

    lfborjas authored
    The following should not raise an assertion exception
    
        assert.propertyVal({camus: NaN}, 'camus', NaN)
    
    But, since [NaN is not equal to itself as it ought to
    be](http://stackoverflow.com/questions/1565<t_co>4/what-is-the-rationale-for-all-comparisons-returning-false-for-ieee754-nan-values)
    the previous assertion wasn't behaving as expected.
    
    Now, I'm not sure that the weird semantics of `NaN` should allow for
    such a thing, but I'm inclined to believe that, if anyone wants to
    assert that they're expecting `NaN` to be associated to a value, its
    startling nature shouldn't get in the way.
    
     
  3. @lfborjas

    Checks for an undefined val iff the key is present

    lfborjas authored
    So we don't go around doing weird stuff like
    
        assert.propertyVal({foo: 'bar'}, 'not_there', undefined)
    
    And having it pass!
This page is out of date. Refresh to see the latest.
Showing with 27 additions and 2 deletions.
  1. +16 −2 lib/chai/core/assertions.js
  2. +11 −0 test/assert.js
View
18 lib/chai/core/assertions.js
@@ -647,6 +647,20 @@ module.exports = function (chai, _) {
? _.getPathValue(name, obj)
: obj[name];
+ var wantsUndefinedVal = arguments.length >= 2
+ && name in Object(obj)
+ && val === undefined;
+
+ var eql = function( a, b ){
+ //from underscore:
+ //https://github.com/documentcloud/underscore/blob/0a2adcb0d19b42b61e8ab63d3a5dee6ef2bcce48/underscore.js#L961
+ //NaN is not equal to itself!
+ var isNaN = function(){ return '[object Number]' === toString.call(a) && +a != +a }
+
+ if( isNaN(a) ) return isNaN(b);
+ return a === b;
+ };
+
if (negate && undefined !== val) {
if (undefined === value) {
msg = (msg != null) ? msg + ': ' : '';
@@ -654,14 +668,14 @@ module.exports = function (chai, _) {
}
} else {
this.assert(
- undefined !== value
+ undefined !== value || wantsUndefinedVal
, 'expected #{this} to have a ' + descriptor + _.inspect(name)
, 'expected #{this} to not have ' + descriptor + _.inspect(name));
}
if (undefined !== val) {
this.assert(
- val === value
+ eql( val, value )
, 'expected #{this} to have a ' + descriptor + _.inspect(name) + ' of #{exp}, but got #{act}'
, 'expected #{this} to not have a ' + descriptor + _.inspect(name) + ' of #{act}'
, val
View
11 test/assert.js
@@ -390,7 +390,13 @@ suite('assert', function () {
test('property', function () {
var obj = { foo: { bar: 'baz' } };
var simpleObj = { foo: 'bar' };
+ var evilObj = { gone: undefined, dubious: NaN };
+ var evilObj2 = Object.create(evilObj);
+
assert.property(obj, 'foo');
+ assert.propertyVal(evilObj, 'gone', undefined);
+ assert.propertyVal(evilObj2, 'gone', undefined);
+ assert.propertyVal(evilObj, 'dubious', NaN);
assert.deepProperty(obj, 'foo.bar');
assert.notProperty(obj, 'baz');
assert.notProperty(obj, 'foo.bar');
@@ -419,9 +425,14 @@ suite('assert', function () {
}, "expected { foo: 'bar' } to have a property 'foo' of 'ball', but got 'bar'");
err(function () {
+ assert.propertyVal(evilObj, 'foo', undefined);
+ }, "expected { gone: undefined, dubious: NaN } to have a property 'foo'");
+
+ err(function () {
assert.deepPropertyVal(obj, 'foo.bar', 'ball');
}, "expected { foo: { bar: 'baz' } } to have a deep property 'foo.bar' of 'ball', but got 'baz'");
+
err(function () {
assert.propertyNotVal(simpleObj, 'foo', 'bar');
}, "expected { foo: 'bar' } to not have a property 'foo' of 'bar'");
Something went wrong with that request. Please try again.