Add tests for global objects and missing object tests using uint32 as key #939
Conversation
|
@JckXia could you squash into 1 commit? I tried to pull in locally but I think due to the intermediate changes it was reporting a conflict. |
|
@mhdawson Yeah of course! I will get to it. |
7e8a1ca to
1a17f47
Compare
test/binding.gyp
Outdated
| 'globalObject/delete_property.cc', | ||
| 'globalObject/has_own_property.cc', | ||
| 'globalObject/set_property.cc', | ||
| 'globalObject/get_property.cc', | ||
| 'globalObject/object.cc', |
There was a problem hiding this comment.
The build for the test fails on windows. In windows I'm getting the error reported below:
The problem is that on windows the object files will be created on build/Release/obj/binding or build/Release/obj/binding_noexcept instead in unix system the object files will be created on the subfolders you set.
One solution for me was to chenge the name of the following files:
'globalObject/delete_property.cc',
'globalObject/has_own_property.cc',
'globalObject/set_property.cc',
'globalObject/get_property.cc',
'globalObject/object.cc',
to something like this:
'globalObject/global_object_delete_property.cc',
'globalObject/global_object_has_own_property.cc',
'globalObject/global_object_set_property.cc',
'globalObject/global_object_get_property.cc',
'globalObject/global_object.cc',
This avoid the name conflicts for the object files generated on windows system.
There was a problem hiding this comment.
Okay thank you! I am just wondering, i noticed that we run the tests per PR on MacOS and ubuntu , should we setup a similar CI for a window systems to catch issues like this?
There was a problem hiding this comment.
I agree with you and I just opened a new PR #948 that add the windows platform to CI.
| napi_value napi_key; | ||
| napi_create_int32(info.Env(), 2, &napi_key); |
There was a problem hiding this comment.
Could you use Napi::Number here?
Number napi_key = Number::New(info.Env(), 2);| Object globalObject = info.Env().Global(); | ||
| Name key = info[0].As<Name>(); | ||
| return Boolean::New( | ||
| info.Env(), globalObject.HasOwnProperty(static_cast<napi_value>(key))); |
There was a problem hiding this comment.
I think that here it's not necessary to cast the key value. You can do the same like reported below:
return Boolean::New(
info.Env(), globalObject.HasOwnProperty(key));| Object globalObject = info.Env().Global(); | ||
| Name key = info[0].As<Name>(); | ||
| Value value = info[1]; | ||
| globalObject.Set(static_cast<napi_value>(key), value); |
There was a problem hiding this comment.
I think that here it's not necessary to cast the key value. You can do the same like reported below:
globalObject.Set(key, value);PR-URL: #939 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#939 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#939 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#939 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#939 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#939 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs/node-addon-api#939 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
This pull request aims to test the various CRUD actions that we can perform on the Global object. Because C++ is statically typed and the keys could be of type int, string, C string or napi_value, each of these cases should be checked thoroughly.
(i.e Changes made from the c++ side should be reflected on the javascript side as well)
Also adding some missing object tests where they use uint32 as key