Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

weird operator test: assert.operator(1, '!==', '1') #386

Closed
jokeyrhyme opened this issue Mar 3, 2015 · 1 comment
Closed

weird operator test: assert.operator(1, '!==', '1') #386

jokeyrhyme opened this issue Mar 3, 2015 · 1 comment

Comments

@jokeyrhyme
Copy link

@jokeyrhyme jokeyrhyme commented Mar 3, 2015

I just noticed a weird test for assert.operator():

chai/test/assert.js

Lines 595 to 597 in 051fd95

err(function () {
assert.operator(1, '!==', '1');
}, "expected 1 to be !== \'1\'");

    err(function () {
      assert.operator(1, '!==', '1');
     }, "expected 1 to be !== \'1\'");

Basically, this test expects the assertion to throw an error, and will throw an error (failing the test) if it does not.

!== is strict inequality, which means types should matter. Is there a special reason why the Number 1 is expected to be strictly equal to the String "1" ?

@keithamus
Copy link
Member

@keithamus keithamus commented Mar 3, 2015

Congratulations @jokeyrhyme! You've just found a bug! Thanks very much for filing this issue - you're totally right - that isn't meant to behave that way.

After a bit of examination, it has to do with the way the operators are checked. assert.operator uses eval - and it concatenates the values together with the operator as a String. Given the example, the values Number(1), String('!==') and String('1') are concatenated (constructors used for emphasis). Of course, this makes a string of String('1 !== 1') - and so I suspect the tests have been malformed to fix what appeared to be breaking tests, but was actually breaking code.

Interestingly, this makes assert.operator especially useless for testing lots of primitive values; for example assert.operator([1,2,3], '===', [1,2,3]); becomes eval('1,2,3===1,2,3'); which returns 3, not false like it should.

I see there being two proposals for a fix:

1: Trick the eval function into using strings.

We want the eval function to get literal values wrapped in their correct syntax. The easiest way to do this is to simply call JSON.stringify on each value - which will give you the equivalent JS syntax for that literal, as a String. E.g. JSON.stringify(Number(1)) === '1' while JSON.stringify(String(1)) === '"1"'. So Line 1010 of assert.js becomes:

var test = new Assertion(eval(JSON.stringify(val) + operator + JSON.stringify(val2)), msg);

2: Stop using eval

Probably the better fix for this is to just stop using eval - the code will be longer but potentially more reliable and less prone to bugs like these. Of course, this means the laborious task of taking each operator and making the appropriate statement in JS. For example:

switch(operator) {
case '===':
    ok = val1 === val2;
    break;
case '!==':
    ok = val1 !== val2;
    break;
// and so on
}

The nice benefit to this is that references are kept (they wouldn't have been with eval) so asserts like: assert.operator(x, '===', x); (where x is not a primitive) also work.


Ultimately I feel like solution 2 is the correct one - if you'd like to make a PR @jokeyrhyme, I'd be happy to review and merge it. If you do so - make sure you add a good few tests, especially if you do end up implementing solution 2 - also add some tests around asserting references, as mentioned above. Getting a PR merged is basically the best feeling ever, and lands you a place on our contributors hall of fame, and if you get stuck I'll be on point to help out if needed.

christophertrml pushed a commit to christophertrml/chai that referenced this issue Mar 8, 2015
christophertrml pushed a commit to christophertrml/chai that referenced this issue Mar 8, 2015
keithamus added a commit that referenced this issue Mar 8, 2015
No longer using eval on assert operator #386
@keithamus keithamus closed this Mar 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants