Conversation
NickNaso
left a comment
There was a problem hiding this comment.
@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? |
|
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 <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
|
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 <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: nodejs/node-addon-api#923 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: nodejs/node-addon-api#923 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
Test that Object::GetPropertyNames does not return properties whose key is a Symbol PR-URL: nodejs/node-addon-api#923 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: NickNaso <nicoladelgobbo@gmail.com>
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::Objectclass. 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
Symbolis 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::Getmust returnundefinedif 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.