-
Notifications
You must be signed in to change notification settings - Fork 465
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
Add Object missing tests #923
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoseExposito thanks for wirking on this. Just some comments for now.
In my opinion you should create a test for |
Hi @NickNaso Thanks for you code review
Added in 61a0c76 EDIT: I'm seeing the same crash that was present in #928 here now:
It looks like this problem is affecting other PRs, like #927 or #909. Maybe something is going on with the CI system? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In terms of:
I'd be ok with testing it in both as its a simple/quick test and it would mean that it would be clear we have full Object coverage. |
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: #923 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: NickNaso <[email protected]>
Landed in 298ff8d. @JoseExposito thanks for the PR! |
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: nodejs/node-addon-api#923 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: NickNaso <[email protected]>
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: nodejs/node-addon-api#923 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: NickNaso <[email protected]>
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: nodejs/node-addon-api#923 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: NickNaso <[email protected]>
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: nodejs/node-addon-api#923 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: NickNaso <[email protected]>
Hi!
I submitted part of this changes to my mentee application. I don't think it was accepted (:sob:) but it doesn't matter, I think we can still merge them :wink:
This PR adds tests that were missing for the
Napi::Object
class. I splitted the changes in commits to make life easier for the code reviewer:Test that Object::GetPropertyNames does not return properties whose key is a Symbol
As detailed in the documentation when a
Symbol
is used as key, it should not be returned byObject::GetPropertyNames
.Test that Object::Get returns undefined if the key does not exist
The docs point out that
Object::Get
must returnundefined
if the key does not exist. I added an extra test for that case and made a quick refactor to avoid repetition.Add tests for Object::InstanceOf
Add tests for Object::Object empty constructor
Here I tested that the returned object to JavaScript is
undefined
. I wasn't sure if this was the right approach or instead I should test thatValue::IsEmpty()
. I'd appreciate your advice here.Add tests for subscript operator
At this point, the only method in this table that is not being tested is
Object(napi_env env, napi_value value)
. I didn't implement it because it basically callsValue(env, value)
so I think it makes sense to implement it inbasic_types/value
, but let me know what you think.