Skip to content
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

Adding unit tests for FunctionReference class #1035

Closed
wants to merge 6 commits into from

Conversation

JckXia
Copy link
Member

@JckXia JckXia commented Aug 14, 2021

Adding test coverage for FunctionReference class. #989

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

mhdawson commented Sep 3, 2021

@nodejs/node-api anybody else want to take a look before I land this?

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you perform some formatting on your JavaScript sources? I see a lot of different spacing conventions between the tokens (eg. spaces between parameters)

let hook;
const events = []
return new Promise((res, reject) =>{
console.log('Installing async hook')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this.

test/binding.gyp Outdated
@@ -27,6 +27,7 @@
'error.cc',
'external.cc',
'function.cc',
'funcRefObject.cc',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure the file names and the name of the init function are consistent for #1078 by @deepakrkris!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepakrkris this is interesting, because it creates a situation where a test file has a dependency. Maybe this class definition should go to the top of function_reference.cc instead because that's the only file using it. Then we can also avoid having to deal with file dependencies.

int _value;
};

#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline.

Napi::FunctionReference ref;
ref.Reset(info[0].As<Function>());

Napi::AsyncContext contxt(info.Env(), "func_ref_resources", {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context

args[argIdx] = info[i];
}

Napi::AsyncContext contxt(info.Env(), "func_ref_resources", {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context

exports["call"] = Function::New(env, Call);
exports["construct"] = Function::New(env, Construct);

return exports;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a newline!

binding.AsyncCallWithArgv(testFuncB, 2, 4, 5, 6) === testFuncB(2, 4, 5, 6)
);
}
function test (binding) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be async because it should await canCallAsyncFunctionWithDifferentOverloads.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and it should itself be awaited at the call site.

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mhdawson pushed a commit that referenced this pull request Oct 25, 2021
PR-URL: #1035
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
@mhdawson
Copy link
Member

Landed as 04b26a9

@mhdawson mhdawson closed this Oct 25, 2021
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
PR-URL: nodejs/node-addon-api#1035
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
PR-URL: nodejs/node-addon-api#1035
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
PR-URL: nodejs/node-addon-api#1035
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
@JckXia JckXia deleted the add-tests-to-function-ref branch December 14, 2022 00:46
austinli64 added a commit to austinli64/node-addon-api that referenced this pull request May 9, 2023
PR-URL: nodejs/node-addon-api#1035
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
PR-URL: nodejs/node-addon-api#1035
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants