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

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

Merged
merged 10 commits into from
Oct 2, 2017

Conversation

solodynamo
Copy link
Contributor

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.

@solodynamo solodynamo requested a review from a team as a code owner September 9, 2017 20:03
@keithamus
Copy link
Member

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
Copy link
Contributor

meeber commented Sep 9, 2017

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.

@@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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 + ': ' : ''
Copy link
Contributor

@meeber meeber Sep 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@meeber
Copy link
Contributor

meeber commented Sep 10, 2017

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

@solodynamo
Copy link
Contributor Author

solodynamo commented Sep 10, 2017

@meeber incorporated more suggested changes .


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/expect.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay , so just duplicating them in both the files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor Author

@solodynamo solodynamo Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

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

test/assert.js Outdated
@@ -1,5 +1,6 @@
describe('assert', function () {
var assert = chai.assert;
var expect = chai.expect;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/should.js Outdated
@@ -1,5 +1,6 @@
describe('should', function() {
var should = chai.Should();
var expect = chai.expect;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@meeber updated PR .

test/assert.js Outdated
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');
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are using chai.expect on line 18 too.

Copy link
Member

@vieiralucas vieiralucas Sep 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@solodynamo
Copy link
Contributor Author

@meeber updated PR .

test/assert.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/assert.js Outdated

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

@vieiralucas vieiralucas Sep 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/assert.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
keithamus previously approved these changes Sep 24, 2017
Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/expect.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/expect.js Outdated

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the err helper function.

test/should.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

test/should.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@keithamus @meeber @vieiralucas

@meeber
Copy link
Contributor

meeber commented Oct 2, 2017

LGTM! @vieiralucas ?

@vieiralucas
Copy link
Member

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. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants