Fix bug when asserting some valid ES6 keys #676
Conversation
meeber
commented
Apr 9, 2016
meeber
commented
- Resolution of assertKeys cannot handle some valid ES6 keys #674
- Add compareByInspect utility for use with assertKeys sorts
- Add getOwnEnumerableProperties utility
- Add getOwnEnumerablePropertySymbols utility
- Add Symbol support to the inspect utility
- Add tests to utilities, should, expect, and assert
| @@ -1172,6 +1172,11 @@ module.exports = function (chai, _) { | |||
| } else { | |||
| actual = Object.keys(obj); | |||
|
|
|||
| if (typeof Object.getOwnPropertySymbols === 'function') { | |||
| // Object.keys excludes Symbols so we need this too | |||
| actual = actual.concat(Object.getOwnPropertySymbols(obj)); | |||
There was a problem hiding this comment.
Slight concern with this - Object.getOwnPropertySymbols returns enumerable and non-enumerable keys - somewhat changing the semantics of what this assertion already does. Should we filter out the non-enumerable keys? Something like:
actual = actual.concat(Object.getOwnPropertySymbols(obj).filter(function (symbol) {
return Object.getOwnPropertyDescriptor(obj, symbol).enumerable;
}));There was a problem hiding this comment.
Whoops! Great catch. I'm on it :)
meeber
force-pushed
the
es6-keys-asserts
branch
from
7b228ae
to
7a97a67
Compare
|
Fixed the issue with the non-enumerable symbols, and also modified a couple of the keys tests to validate that non-enumerable strings and symbols aren't included. |
|
I just went through it all and it LGTM! Note for later: We may need to solve merge conflicts on #668 |
|
Great, thank you! Just noticed a typo in the comments; gonna fix it real quick. Edit: fixed |
- Resolution of chaijs#674 - Add compareByInspect utility for use with assertKeys sorts - Add getOwnEnumerableProperties utility - Add getOwnEnumerablePropertySymbols utility - Add Symbol support to the inspect utility - Add tests to utilities, should, expect, and assert
meeber
force-pushed
the
es6-keys-asserts
branch
from
7a97a67
to
e228351
Compare
|
LGTM 👍 |
