property assertion should only accept strings if nested, fixes #1043 #1044

solodynamo · 2017-09-09T20:03:17Z

Description of the Change

Adds case check when property assertion is not string and nested flag is set.

Why should this be in core?

Adds required check case when non-string param is passed with nested flag set.

Benefits

It will provide more descriptive error to the devs on invalid passed params.

…#1043

keithamus · 2017-09-09T21:56:58Z

Great work thanks for this @solodynamo. Do you think you could also add a test case to verify this behaviour? The property tests can be found here:

chai/test/expect.js

Lines 1416 to 1465 in dbeae08

    
           it('property(name)', function(){ 
        
             expect('test').to.have.property('length'); 
        
             expect({a: 1}).to.have.property('toString'); 
        
             expect(4).to.not.have.property('length'); 
        
             expect({ 'foo.bar': 'baz' }) 
        
               .to.have.property('foo.bar'); 
        
             expect({ foo: { bar: 'baz' } }) 
        
               .to.not.have.property('foo.bar'); 
        
             // Properties with the value 'undefined' are still properties 
        
             var obj = { foo: undefined }; 
        
             Object.defineProperty(obj, 'bar', { 
        
               get: function() { } 
        
             }); 
        
             expect(obj).to.have.property('foo'); 
        
             expect(obj).to.have.property('bar'); 
        
             expect({ 'foo.bar[]': 'baz'}) 
        
               .to.have.property('foo.bar[]'); 
        
             err(function(){ 
        
               expect('asd').to.have.property('foo'); 
        
             }, "expected 'asd' to have property 'foo'"); 
        
             err(function(){ 
        
               expect('asd', 'blah').to.have.property('foo'); 
        
             }, "blah: expected 'asd' to have property 'foo'"); 
        
             err(function(){ 
        
               expect({ foo: { bar: 'baz' } }) 
        
                 .to.have.property('foo.bar'); 
        
             }, "expected { foo: { bar: 'baz' } } to have property 'foo.bar'"); 
        
             err(function() { 
        
               expect({a: {b: 1}}).to.have.own.nested.property("a.b"); 
        
             }, "The \"nested\" and \"own\" flags cannot be combined."); 
        
             err(function() { 
        
               expect({a: {b: 1}}, 'blah').to.have.own.nested.property("a.b"); 
        
             }, "blah: The \"nested\" and \"own\" flags cannot be combined."); 
        
             err(function () { 
        
               expect(null, 'blah').to.have.property("a"); 
        
             }, "blah: Target cannot be null or undefined."); 
        
             err(function () { 
        
               expect(undefined, 'blah').to.have.property("a"); 
        
             }, "blah: Target cannot be null or undefined."); 
        
           });

meeber · 2017-09-09T22:27:05Z

Thanks for working on this @solodynamo. I think we need to expand this a bit further to also perform a type check if not isNested. For example:

var nameType = typeof name;
if (isNested) {
  if (nameType !== 'string') {
    <error>
  }
} else {
  if (nameType !== 'string' && nameType !== 'number' && nameType !== 'symbol') {
    <error>
  }
}

As @keithamus mentioned, it would be good to have tests confirming the new behavior. Whatever tests that are added to chai/test/expect.js should be replicated for the other interfaces in chai/test/should.js and chai/test/assert.js.

meeber · 2017-09-09T22:27:51Z

lib/chai/core/assertions.js

@@ -1765,6 +1765,12 @@ module.exports = function (chai, _) {
      , obj = flag(this, 'object')
      , ssfi = flag(this, 'ssfi');

+    if (typeof name !== 'string' && isNested) {
+      var msgPrefix = flag(this, 'message')


We don't need to define msgPrefix here; the value of flag(this, 'message') is already stored in the flagMsg variable from line 1764.

meeber · 2017-09-09T22:30:04Z

lib/chai/core/assertions.js

@@ -1765,6 +1765,12 @@ module.exports = function (chai, _) {
      , obj = flag(this, 'object')
      , ssfi = flag(this, 'ssfi');

+    if (typeof name !== 'string' && isNested) {
+      var msgPrefix = flag(this, 'message')
+      msgPrefix = msgPrefix ? msgPrefix + ': ' : ''


The logic here on line 1770 is repeated on lines 1769 and 1778 (except with flagMsg); I'm thinking we can reduce duplication by removing all three of these and instead perform the operation a single time near the top after line 1766 where the variable declarations take place.

Yes, there is little duplication of logic here . We can remove msgPrefix and use flagMsg variable in its place and move the flagMsg = flagMsg ? flagMsg + ': ' : '' near the top as you suggested.

solodynamo · 2017-09-10T11:15:48Z

Hey @keithamus @meeber updated the PR with suggested changes .

meeber · 2017-09-10T13:20:28Z

@solodynamo Thanks for your continued work on this! Please see this comment for a couple more suggested changes.

solodynamo · 2017-09-10T16:34:31Z

@meeber incorporated more suggested changes .

vieiralucas · 2017-09-10T16:53:49Z

lib/chai/core/assertions.js

+
+    if (isNested) {
+      if (nameType !== 'string') {
+        throw new AssertionError(flagMsg + 'the argument to `property` must be a string');


What about telling that it must be string because the nested flag is on?

Something like:

throw new AssertionError(flagMsg + 'the argument to `property` with nested flag must be a string');

I'm afraid that it will be confusing to sometimes say that it can be a string, number or symbol and sometimes just saying that it must be a string

Thoughts? @keithamus @solodynamo @meeber

But rather than writing nested flag we should think something else as end user may not know about internal logic of isNested. Suggestions ? @vieiralucas @meeber @keithamus

Agreed, we can safely assume to get here they've written .nested.propery() so we can use that instead.

A user could reach this error via the assert interface too, though, so nested.property wouldn't always be entirely accurate.

I'm having a hard time thinking on a good message

the argument to `property` when nesting must be a `string` the argument to `property` for a nested assertion must be a `string`

How about:

Normal: the argument to property must be a string, number, or symbol
Nested: the argument to property must be a string when using nested syntax

meeber · 2017-09-11T10:40:49Z

test/expect.js

+
+    expect(function () {
+      expect({a:1}).to.have.property(null);
+    }).to.throw('the argument to `property` must be either of type string, number or symbol');


Let's repeat these three new error tests in the should and assert interface test files.

Okay , so just duplicating them in both the files.

@keithamus @meeber isNested is true when name is undefined , Shouldn't this be false

@solodynamo I'm not sure what you mean.

On a separate note, I just noticed a couple of things:

Let's remove the .not.throw test on lines 1466-1468; we already test successful .nested.property assertions in the section that starts on line 1754.

Let's relocate the test on lines 1470-1472 to the end of the nested.property section (that starts on line 1754).

@meeber I meant to ask
expect(function () { ({a:1}).should.have.nested.property(undefined); }).to.throw('the argument toproperty must be either of type string, number or symbol');
Above test is throwing error 'the argument to property must be a string'. I debugged and found isNested is true in above case , so i meant isNested should be false ? Please correct me if i misunderstood something.

solodynamo · 2017-09-14T05:47:05Z

@meeber @keithamus @vieiralucas look through updated changes, if looks fine please merge.

meeber · 2017-09-16T12:37:43Z

test/assert.js

@@ -1,5 +1,6 @@
 describe('assert', function () {
  var assert = chai.assert;
+  var expect = chai.expect;


Let's not add expect to the assert interface tests. It should be possible to test errors without expect by using the same style that's used elsewhere in this file.

meeber · 2017-09-16T12:39:00Z

test/should.js

@@ -1,5 +1,6 @@
 describe('should', function() {
  var should = chai.Should();
+  var expect = chai.expect;


Same thing here. Let's not add expect to the should interface tests.

meeber · 2017-09-16T12:46:06Z

lib/chai/core/assertions.js

+
+    if (isNested) {
+      if (nameType !== 'string') {
+        throw new AssertionError(flagMsg + 'the argument to `property` must be a string');


How about:

Normal: the argument to property must be a string, number, or symbol
Nested: the argument to property must be a string when using nested syntax

solodynamo · 2017-09-16T17:32:19Z

@meeber updated PR .

solodynamo · 2017-09-16T18:03:49Z

test/assert.js

      assert.nestedProperty({a:1}, {'a':'1'});
-    }).to.throw('the argument to `property` must be a string');
+    }).to.throw('the argument to property must be a string when using nested syntax');
  });



we are using chai.expect on line 18 too.

It's shouldn't be wrong to use expect in this test, because here we are actually using chai to assert, we are not calling assertions and expecting them to work, we are testing that the assertion throws.

But I agree with @meeber that we must try and be able to use own interface to test it.

You can replace the ones you added on this PR with assert.throws and assert.doesNotThrows and maybe remove the others in another PR.

Sure @vieiralucas . I have replaced them and also surely will open a seperate PR in future to replace the remaining.

solodynamo · 2017-09-17T10:05:48Z

@meeber updated PR .

vieiralucas · 2017-09-17T18:09:17Z

test/assert.js

+
+    assert.throws(function () {
+      assert.nestedProperty({a:1}, {'a':'1'});
+    }, Error, 'the argument to property must be a string when using nested syntax');


@solodynamo I'm sorry for making a mess, but I realized that what @meeber meant to suggest is for you to use the err function just like on the tests above.

Notice that when you do that it will start to throw implementation frames not properly filtered from stack trace:.
Thats because you forgot to pass the ssfi argument to the constructor of the AssertionError
You are doing this:

throw new AssertionError(flagMsg + 'the argument to property must be a string when using nested syntax');

But you need to do this:

throw new AssertionError( flagMsg + 'the argument to property must be a string when using nested syntax', undefined, ssfi );

I'm sorry again for the confusion I made.

Also, please pass a third argument to test if the custom message works.
Your test should look like this:

err(function () { assert.nestedProperty({a:1}, {'a':'1'}, 'blah'); }, 'blah: the argument to property must be a string when using nested syntax');

vieiralucas · 2017-09-17T18:11:11Z

lib/chai/core/assertions.js

+      }
+    } else {
+      if (nameType !== 'string' && nameType !== 'number' && nameType !== 'symbol') {
+        throw new AssertionError(flagMsg + ' the argument to property must be a string, number, or symbol');


It looks like this has an extra space. flagMsg variable already has a trailing space

vieiralucas · 2017-09-17T18:16:29Z

test/assert.js

+
+    assert.doesNotThrow(function () {
+      assert.nestedProperty({a:1}, '{a:1}');
+    }, Error, 'the argument to property must be a string when using nested syntax');


I don't think you need to add this test actually.
There is already a test above on line 1404 for the assert.nestedProperty passing a String.

vieiralucas · 2017-09-17T18:20:52Z

test/assert.js

+
+    assert.throws(function () {
+      assert.nestedProperty({a:1}, {'a':'1'});
+    }, Error, 'the argument to property must be a string when using nested syntax');


Also, please pass a third argument to test if the custom message works.
Your test should look like this:

err(function () { assert.nestedProperty({a:1}, {'a':'1'}, 'blah'); }, 'blah: the argument to property must be a string when using nested syntax');

keithamus

Thanks for your patience o this @solodynamo. This LGTM 👍 good work. @meeber @vieiralucas everything okay for you both?

meeber

The assertion code looks good, but I still have some issues with the tests. Thank you for your continued work on this!

meeber · 2017-09-24T19:10:59Z

test/expect.js

+
+    expect(function () {
+      expect({a:1}).to.have.property(null);
+    }).to.throw('the argument to property must be a string, number, or symbol');


This should use the err helper function, just like the test above it.

meeber · 2017-09-24T19:11:21Z

test/expect.js

+
+    expect(function () {
+      expect({a:1}).to.have.nested.property({'a':'1'});
+    }).to.throw('the argument to property must be a string when using nested syntax');


This should use the err helper function.

meeber · 2017-09-24T19:12:03Z

test/should.js

+
+    (function () {
+      ({a:1}).should.have.nested.property({'a':'1'});
+    }).should.throw('the argument to property must be a string when using nested syntax');


This should use the err helper function, just like the test below it.

meeber · 2017-09-24T19:14:58Z

test/assert.js

+
+    err(function () {
+      assert.nestedProperty({a:1}, {'a':'1'}, 'blah');
+    }, 'blah: the argument to property must be a string when using nested syntax');


One more test should be added here to the assert interface tests in order to verify the error message "the argument to property must be a string, number, or symbol". This test can closely resemble the expect({a:1}).to.have.property(null); test that you added on the expect interface, except using the assert interface instead of expect. (And using the err helper function, just like you've done in the previous two tests on the assert interface).

meeber · 2017-09-24T19:17:55Z

test/should.js

+
+    (function () {
+      ({a:1}).should.have.nested.property('{a:1}');
+    }).should.not.throw('the argument to property must be a string when using nested syntax');


Let's remove this test. We already test that .nested.property works properly elsewhere in the suite. Adding this test with ".should.not.throw` doesn't add any value.

meeber · 2017-09-24T19:19:24Z

test/should.js

+    (function () {
+      ({a:1}).should.have.nested.property({'a':'1'});
+    }).should.throw('the argument to property must be a string when using nested syntax');
+


One more test should be added here to the should interface tests in order to verify the error message "the argument to property must be a string, number, or symbol". This test can closely resemble the expect({a:1}).to.have.property(null); test that you added on the expect interface, except using the should interface this time instead of expect. (And using the err helper function).

solodynamo · 2017-10-01T14:54:09Z

@keithamus @meeber @vieiralucas

meeber · 2017-10-02T01:01:15Z

LGTM! @vieiralucas ?

vieiralucas · 2017-10-02T20:43:52Z

LGTM! Thank you so much for this PR @solodynamo
I'm sorry that it took so long, but you did it! I hope you had a good experience contributing to chai. 😄

property assertion should only accept strings if nested, fixes chaijs…

e167009

…#1043

solodynamo requested a review from a team as a code owner September 9, 2017 20:03

meeber reviewed Sep 9, 2017

View reviewed changes

solodynamo added 2 commits September 10, 2017 16:35

similar logic seperated out

3f2ce29

test cases for fix chaijs#1043

335fd33

type check if not isNested with tests

5fd4fa2

vieiralucas reviewed Sep 10, 2017

View reviewed changes

meeber reviewed Sep 11, 2017

View reviewed changes

Tests for assert and should inteface

8ff7cb7

meeber requested changes Sep 16, 2017

View reviewed changes

solodynamo added 2 commits September 16, 2017 20:06

Error message change

a078a6d

changes in should and assert tests as per review

e1442d0

solodynamo commented Sep 16, 2017

View reviewed changes

assert tests modified

d58d2bf

vieiralucas requested changes Sep 17, 2017

View reviewed changes

Review comments changes

84062b1

keithamus previously approved these changes Sep 24, 2017

View reviewed changes

meeber requested changes Sep 24, 2017

View reviewed changes

solodynamo dismissed keithamus’s stale review via f9e5a68 September 29, 2017 15:19

review comments fixes

f9e5a68

meeber approved these changes Oct 2, 2017

View reviewed changes

vieiralucas approved these changes Oct 2, 2017

View reviewed changes

vieiralucas merged commit 3ace4a0 into chaijs:master Oct 2, 2017

straker mentioned this pull request Jul 29, 2019

chore: update dependencies dequelabs/axe-core#1726

Merged

4 tasks

snyk-bot mentioned this pull request Jan 29, 2020

[Snyk] Upgrade chai from 4.1.2 to 4.2.0 davemenninger/socket.io-chat-fargate#3

Open

snyk-bot mentioned this pull request Mar 15, 2020

[Snyk] Upgrade chai from 4.0.2 to 4.2.0 postmanlabs/sails-env-switch#15

Open

property assertion should only accept strings if nested, fixes #1043 #1044

property assertion should only accept strings if nested, fixes #1043 #1044

Conversation

solodynamo commented Sep 9, 2017

Description of the Change

Why should this be in core?

Benefits

keithamus commented Sep 9, 2017

meeber commented Sep 9, 2017

Choose a reason for hiding this comment

meeber Sep 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solodynamo commented Sep 10, 2017

meeber commented Sep 10, 2017

solodynamo commented Sep 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solodynamo Sep 12, 2017 • edited

Choose a reason for hiding this comment

solodynamo commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solodynamo commented Sep 16, 2017

Choose a reason for hiding this comment

vieiralucas Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solodynamo commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vieiralucas Sep 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithamus left a comment

Choose a reason for hiding this comment

meeber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solodynamo commented Oct 1, 2017

meeber commented Oct 2, 2017

vieiralucas commented Oct 2, 2017

meeber Sep 9, 2017 •

edited

solodynamo commented Sep 10, 2017 •

edited

solodynamo Sep 12, 2017 •

edited

vieiralucas Sep 16, 2017 •

edited

vieiralucas Sep 17, 2017 •

edited