fix testEnumerables on ObjectWrap #736
Conversation
|
Also since the key length assertion was broken, no one noticed that the key length actually changed because addition instance properties were added. (in e8935bd) |
| assert(keys.includes("testGetSet")); | ||
| assert(keys.includes("testGetter")); | ||
| assert(keys.includes("testValue")); | ||
| assert(keys.includes("testMethod")); |
There was a problem hiding this comment.
Since there are now 6, there should likely be an assert for the 2 additional methods right?
There was a problem hiding this comment.
From what I see, tests for those were added in the same commit the properties were added. See e8935bd#diff-ec20a47eb5374e448ec0794a5fb6ac6eR100
Though maybe I'm missing something...
There was a problem hiding this comment.
I just though that if it validates that the number is 6, then it would be followed by 6 asserts as to what those are. It's not clear to without taking more time to check that that the earlier
assert(Object.keys(obj).length === 2);
assert(Object.keys(obj).includes('ownProperty'));
assert(Object.keys(obj).indexOf('ownPropertyT') >= 0);
covers that and if it it does it may may sense to still have the additional checks so that if the length is 6 we follow it with 6 checks.
There was a problem hiding this comment.
I added the check in 2405844
PR-URL: #736 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node-addon-api#736 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node-addon-api#736 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node-addon-api#736 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node-addon-api#736 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The assertion on key length is broken. It's an assignment instead of a bool expression.
Also changes the tests for properties to the same style as the tests for "own properties."