Added test coverage for Symbol class #972

JckXia · 2021-04-20T02:19:31Z

Added new tests for the Symbol class. Also added a static method to the Symbol class to perform "Symbol.For()" from the C++ side, since the exisiting method (Symbol::Wellknown) , which has a different functionality caused some confusion.

JckXia · 2021-04-20T02:54:25Z

I can't seem to recreate this test failure. It seems intermittent as the CI running against this commit is successful in my forked repo. https://github.com/JckXia/node-addon-api/actions/runs/765661855.

NickNaso

@JckXia well done. Just some comments.

NickNaso · 2021-04-20T10:04:33Z

napi.h

@@ -538,6 +538,9 @@ namespace Napi {
   /// Get a public Symbol (e.g. Symbol.iterator).
   static Symbol WellKnown(napi_env, const std::string& name);

+   // Create a symbol in the global registry;
+   static Symbol For(napi_env env, const std::string& name);


Do we need some other overloads for this methods?

static Symbol For(napi_env env, const char* description = nullptr); static Symbol For(napi_env env, String description); static Symbol For(napi_env env, napi_value description);

What do you think about?

Hmm yeah I think that will be a good idea. Will add them in soon.

NickNaso · 2021-04-20T10:05:23Z

napi.h

@@ -538,6 +538,9 @@ namespace Napi {
   /// Get a public Symbol (e.g. Symbol.iterator).
   static Symbol WellKnown(napi_env, const std::string& name);

+   // Create a symbol in the global registry;
+   static Symbol For(napi_env env, const std::string& name);


Please remember to add documentation for this new method.

NickNaso · 2021-04-20T10:08:18Z

test/symbol.cc

+using namespace Napi;
+
+Symbol CreateNewSymbolWithNoArgs(const Napi::CallbackInfo& info) {
+  (void)info;


Why does it necessary do this?

(void)info;

I think this was to suppress compiler warnings for unused parameter, since the default Symbol constructor doesn't take in any values. But i can remove this function since it's not being called from js.

To avoid compiler complains maybe you can try the following solution (comment the formal parameter of the function):

Symbol CreateNewSymbolWithNoArgs(const Napi::CallbackInfo& /*info*/) {

Let me know if it worked.

Yep it works. Thanks!

NickNaso · 2021-04-20T10:11:40Z

test/symbol.js

+
+}
+
+


Please remove one empty new line.

NickNaso · 2021-04-20T10:11:44Z

test/symbol.cc

+  exports["getSymbolFromGlobalRegistry"] =
+      Function::New(env, FetchSymbolFromGlobalRegistry);
+  return exports;
+}


PLease add one empty new line.

NickNaso · 2021-04-20T10:16:03Z

I can't seem to recreate this test failure. It seems intermittent as the CI running against this commit is successful in my forked repo. https://github.com/JckXia/node-addon-api/actions/runs/765661855.

Don't worry I restarted the jobs and now it's all green.

NickNaso

Hi @JckXia,
I added som e comments about you last work.

NickNaso · 2021-04-23T13:10:22Z

doc/symbol.md

@@ -34,7 +34,7 @@ If an error occurs, a `Napi::Error` will get thrown. If C++ exceptions are not
 being used, callers should check the result of `Napi::Env::IsExceptionPending` before
 attempting to use the returned value.

-### Utf8Value
+### Wellkown


Please fix the typo it should be WellKnown

NickNaso · 2021-04-23T13:13:32Z

doc/symbol.md

+```cpp
+static Napi::Symbol Napi::Symbol::WellKnown(napi_env env, const std::string& name);
+```
+


Please add the documentation for all the overloads of Napi::Symbol Napi::Symbol::WellKnown() methos.

NickNaso · 2021-04-23T13:24:48Z

doc/symbol.md

+- `[in] env`: The `napi_env` environment in which to construct the `Napi::Symbol` object.
+- `[in] name`: The C++ string representing the `Napi::Symbol` to retrieve.
+
+Register `Napi::Symbol` in the global registry. If symbol already exist retrieve said symbol. Equivalent to `Symbol.for("symb")` called from JS.


Maybe the following description is better:
Searches in the global registry for existing symbol with the given name. If the symbol already exist it will be returned, otherwise a new symbol will be created in the registry. It's equivalent to Symbol.for() called from JavaScript.

NickNaso · 2021-04-23T13:37:09Z

napi-inl.h

+}
+
+inline Symbol Symbol::For(napi_env env, String description) {
+  napi_value descriptionValue = description;


You can directory pass description to Symbol::For() because Napi::String is a Napi::Value that has the cast operator napi_value.

KevinEady · 2021-04-23T14:51:30Z

doc/symbol.md

@@ -45,4 +45,14 @@ static Napi::Symbol Napi::Symbol::WellKnown(napi_env env, const std::string& nam
 Returns a `Napi::Symbol` representing a well-known `Symbol` from the
 `Symbol` registry.

+### For
+```cpp
+static Napi::Symbol Napi::Symbol::WellKnown(napi_env env, const std::string& name);


You are adding the For method... should this be For...?

NickNaso

Hi @JckXia,
just something to fix.

NickNaso · 2021-04-27T08:25:28Z

doc/symbol.md

@@ -34,7 +34,7 @@ If an error occurs, a `Napi::Error` will get thrown. If C++ exceptions are not
 being used, callers should check the result of `Napi::Env::IsExceptionPending` before
 attempting to use the returned value.

-### Utf8Value
+### Wellknown


It shlould be WellKnown insteand of Wellknown. The k should be in uppercase.

NickNaso · 2021-04-27T08:28:34Z

doc/symbol.md

+static Napi::Symbol For(napi_env env, const char* description = nullptr);
+static Napi::Symbol For(napi_env env, String description);
+static Napi::Symbol For(napi_env env, napi_value description);


Please use the complete namespace there:

static Napi::Symbol Napi::Symbol::For(napi_env env, const char* description = nullptr); static Napi::Symbol Napi::Symbol::For(napi_env env, String description); static Napi::Symbol Napi::Symbol::For(napi_env env,napi_value description);

NickNaso

Hi @JckXia,
could you propose the last change on the package.json on a different PR? I'm asking for this because the change it's not correlated with the test and in case we will need to revert the change it will be more simpler to do. For the rest it's all ok.

JckXia · 2021-04-28T15:27:37Z

Hey @NickNaso
Thanks for the review! I think I accidentally made some changes to package.json and checked it into git for some reason and was trying to revert it. There was no changes needed to be made to package.json and I will try to revert the changes made to it. Sorry about that.

NickNaso

LGTM

gabrielschulhof

Blocking until we figure out what happens when you pass nullptr to napi_create_string_utf8() as its const char* parameter.

gabrielschulhof · 2021-05-28T15:12:33Z

napi.h

+   static Symbol For(napi_env env, const std::string& description);
+
+   // Create a symbol in the global registry, C style string (null terminated)
+   static Symbol For(napi_env env, const char* description = nullptr);
+
+   // Create a symbol in the global registry, String value describing the symbol
+   static Symbol For(napi_env env, String description);
+
+   // Create a symbol in the global registry, napi_value describing the symbol
+   static Symbol For(napi_env env, napi_value description);
+


Suggested change

static Symbol For(napi_env env, const std::string& description);

// Create a symbol in the global registry, C style string (null terminated)

static Symbol For(napi_env env, const char* description = nullptr);

// Create a symbol in the global registry, String value describing the symbol

static Symbol For(napi_env env, String description);

// Create a symbol in the global registry, napi_value describing the symbol

static Symbol For(napi_env env, napi_value description);

static Symbol For(Env env, const std::string& description);

// Create a symbol in the global registry, C style string (null terminated)

static Symbol For(Env env, const char* description = nullptr);

// Create a symbol in the global registry, String value describing the symbol

static Symbol For(Env env, String description);

// Create a symbol in the global registry, napi_value describing the symbol

static Symbol For(Env env, napi_value description);

gabrielschulhof · 2021-05-28T15:13:00Z

napi-inl.h

@@ -998,6 +998,29 @@ inline Symbol Symbol::WellKnown(napi_env env, const std::string& name) {
  return Napi::Env(env).Global().Get("Symbol").As<Object>().Get(name).As<Symbol>();
 }

+inline Symbol Symbol::For(napi_env env, const std::string& description) {


Suggested change

inline Symbol Symbol::For(napi_env env, const std::string& description) {

inline Symbol Symbol::For(Napi::Env env, const std::string& description) {

... and a similar change for all APIs below.

gabrielschulhof · 2021-05-28T15:30:01Z

@JckXia thanks for catching this!

gabrielschulhof · 2021-05-29T15:44:23Z

We must never pass nullptr into napi_create_string_*() because that will cause a crash. Thus, we should actually change all the APIs (even existing ones) that end up calling napi_create_string_*() to return napi_invalid_arg when passed in what evaluates to (const char *)nullptr.

I'll make a PR upstream to fix this in core.

JckXia · 2021-06-02T01:42:55Z

Thanks for the review @gabrielschulhof . Just making sure, are we converting the env parameter in these overloads from type napi_env to type Napi::Env ? Or are we keeping overloads for both napi_env and Napi::Env ?

gabrielschulhof · 2021-06-04T14:13:34Z

I checked napi.h by looking for [(]napi_env and it turns out most of our APIs take napi_env as the first argument so it's OK to keep the napi_env and we need neither replace nor provide overloads. So, LGTM except for the passing-null-to-napi_create_string_* part.

gabrielschulhof · 2021-06-04T14:22:33Z

Refs: nodejs/node#38923

mhdawson · 2021-06-14T15:27:21Z

@JckXia just wondering what the next steps are to get this landed?

JckXia · 2021-06-14T16:09:12Z

@mhdawson I believe that it is ready to be merged.

KevinEady · 2021-06-14T17:39:46Z

doc/symbol.md

+### For
+```cpp
+static Napi::Symbol Napi::Symbol::For(napi_env env, const std::string& description);
+static Napi::Symbol Napi::Symbol::For(napi_env env, const char* description = nullptr);


Hi @JckXia ,

Do we have a test for this nullptr value...? I am not sure what happens when using a nullptr napi_value for the description here:

auto forSymb = symbObject.Get("for").As<Function>().Call(symbObject, {description});

and I think a test would be beneficial

JckXia · 2021-06-16T01:58:40Z

@KevinEady Thanks for catching that! Apparently if we simply pass the C++ nullptr into that function it will cause a SegFault. I updated the Symbol::For method impl to use the napi_value we get from calling the napi_get_null function instead and it seems to work.

KevinEady · 2021-06-16T16:21:52Z

@KevinEady Thanks for catching that! Apparently if we simply pass the C++ nullptr into that function it will cause a SegFault. I updated the Symbol::For method impl to use the napi_value we get from calling the napi_get_null function instead and it seems to work.

@JckXia Aahahh... I am inclined then to remove the std::nullptr default and the "check if nullptr, assume napi_get_null" ... Because that is not something we have in other locations. For example,

{
  [null]: "3"
}

is a valid JSON object, but we do not check in Object::Get for using const char* utf8name with std::nullptr:

node-addon-api/napi-inl.h

Lines 1227 to 1232 in ad76ad0

    
           inline Value Object::Get(const char* utf8name) const { 
        
             napi_value result; 
        
             napi_status status = napi_get_named_property(_env, _value, utf8name, &result); 
        
             NAPI_THROW_IF_FAILED(_env, status, Value()); 
        
             return Value(_env, result); 
        
           }

The application would just crash (I assume). I would think, for consistency purposes, we should remove the nullptr default as well as the runtime check (which is additional overhead anyway).

If the user wanted to explicitly use null, they could use Symbol::For(env, env.Null()).

Thoughts?

JckXia · 2021-06-16T16:45:23Z

@KevinEady Oh okay I see. So I think we can simply pass the description argument into String::New(), and pass the newly constructed Napi string the Symbol::For function? Also, I think this Symbol::For(env, env.Null()) is a good approach, as I believe they could do something similar for undefined. Will update the PR and later today. :)

KevinEady · 2021-07-02T15:16:48Z

This depends on landing #1015 first.

gabrielschulhof · 2021-07-02T15:18:38Z

Please feel free to dismiss my change request after #1015 has landed and the nullptr default for the char * argument was removed!

KevinEady · 2021-07-02T15:18:54Z

As discussed in today's meeting, please remove the std::nullptr default value in Symbol::For.

mhdawson · 2021-07-09T15:52:08Z

1015 landed, dismissing @gabrielschulhof review as suggested above.

He said it could be dismissed after 1015 landed which has landed.

mhdawson

LGTM

mhdawson · 2021-07-09T19:30:02Z

@JckXia could you squash down to 1 commit? I'm having trouble landing because it crashes in an git am and I'm thinking it being in 1 commit would resolve that.

Add new static method to symbol and updated tests Updated Symbol.md 's docs, added function overload for Symbol class and new tests Remove extra space and comments Remove un-necssary string initialization Update PR based on comments received Add test checking if it's possible to pass nullptr to Symbol::For method Update Symbol::For implementations and added new tests Remove default parameter Update documentation for Symbol Fixing merge conflicts

JckXia · 2021-07-09T19:52:51Z

@mhdawson Thanks, it's been fixed.

PR-URL: #972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

mhdawson · 2021-07-09T21:05:03Z

Landed in e02e8a4

PR-URL: nodejs#972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

PR-URL: nodejs/node-addon-api#972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

NickNaso reviewed Apr 20, 2021

View reviewed changes

NickNaso reviewed Apr 23, 2021

View reviewed changes

KevinEady reviewed Apr 23, 2021

View reviewed changes

NickNaso reviewed Apr 27, 2021

View reviewed changes

NickNaso reviewed Apr 28, 2021

View reviewed changes

JckXia force-pushed the symbol-class-tests branch from 188e7e0 to 0366264 Compare April 28, 2021 16:12

NickNaso approved these changes Apr 29, 2021

View reviewed changes

gabrielschulhof previously requested changes May 28, 2021

View reviewed changes

KevinEady reviewed Jun 14, 2021

View reviewed changes

mhdawson approved these changes Jul 9, 2021

View reviewed changes

JckXia force-pushed the symbol-class-tests branch from 726532d to 26e2930 Compare July 9, 2021 19:47

JckXia force-pushed the symbol-class-tests branch from 26e2930 to bdefbb0 Compare July 9, 2021 19:52

mhdawson pushed a commit that referenced this pull request Jul 9, 2021

test: add first set of symbol tests

e02e8a4

PR-URL: #972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

mhdawson closed this Jul 9, 2021

NickNaso mentioned this pull request Jul 16, 2021

[Tests] Add test Coverage for Symbol class #957

Closed

deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Sep 23, 2021

test: add first set of symbol tests

e937efb

PR-URL: nodejs#972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Oct 15, 2021

test: add first set of symbol tests

b6c5f81

PR-URL: nodejs#972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022

test: add first set of symbol tests

022af08

PR-URL: nodejs/node-addon-api#972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

JckXia deleted the symbol-class-tests branch December 14, 2022 00:46

johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023

test: add first set of symbol tests

52ebfcb

PR-URL: nodejs/node-addon-api#972 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>

	inline Symbol Symbol::For(napi_env env, const std::string& description) {
	inline Symbol Symbol::For(Napi::Env env, const std::string& description) {

Added test coverage for Symbol class #972

Added test coverage for Symbol class #972

Conversation

JckXia commented Apr 20, 2021

JckXia commented Apr 20, 2021

NickNaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickNaso commented Apr 20, 2021

NickNaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickNaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickNaso left a comment

Choose a reason for hiding this comment

JckXia commented Apr 28, 2021

NickNaso left a comment

Choose a reason for hiding this comment

gabrielschulhof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented May 28, 2021

gabrielschulhof commented May 29, 2021

JckXia commented Jun 2, 2021 • edited Loading

gabrielschulhof commented Jun 4, 2021

gabrielschulhof commented Jun 4, 2021

mhdawson commented Jun 14, 2021

JckXia commented Jun 14, 2021

Choose a reason for hiding this comment

JckXia commented Jun 16, 2021

KevinEady commented Jun 16, 2021

JckXia commented Jun 16, 2021

KevinEady commented Jul 2, 2021

gabrielschulhof commented Jul 2, 2021

KevinEady commented Jul 2, 2021

mhdawson commented Jul 9, 2021

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jul 9, 2021

JckXia commented Jul 9, 2021

mhdawson commented Jul 9, 2021

JckXia commented Jun 2, 2021 •

edited

Loading