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

Function tests #928

Merged
merged 11 commits into from
Mar 15, 2021
Merged

Function tests #928

merged 11 commits into from
Mar 15, 2021

Conversation

JoseExposito
Copy link
Contributor

@JoseExposito JoseExposito commented Mar 10, 2021

This PR adds tests for every Function method.

I splitted the changes in commits so the diff is easier to follow.

EDIT 🤔

All tests passed!
Tests aborted with SIGSEGV

@mhdawson
Copy link
Member

@JoseExposito thanks for working on this.

I see in your output:

All tests passed!
Tests aborted with SIGSEGV

Do you see that every time? It indicates something is not quite right so wondering if its related to the PR or possibly an existing intermittent issue.

@JoseExposito
Copy link
Contributor Author

JoseExposito commented Mar 11, 2021

Hi @mhdawson

I don't have permissions to re-run the failed jobs upstream, however, in my fork, they finished successfully:
https://github.com/JoseExposito/node-addon-api/actions/runs/638531308

My guess is that it is an intermittent crash on macOS. I'm going to merge main, let's see what happens...

@JoseExposito
Copy link
Contributor Author

It worked 🎉 It looks like an intermittent issue, none of the runs on my fork failed:
https://github.com/JoseExposito/node-addon-api/actions

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

@JoseExposito thanks for adding this test cases. Just some comments.

test/function.cc Show resolved Hide resolved
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM.

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 mhdawson merged commit 3b8bdda into nodejs:main Mar 15, 2021
@mhdawson
Copy link
Member

I merged this but I found that the tests were failing after having done that with this output on 14.x

Running test 'object/finalizer'
(node:1257166) UnhandledPromiseRejectionWarning: AssertionError [ERR_ASSERTION]: function should not have been called
at mustNotCall (/home/midawson/newpull/land/node-addon-api/test/common/index.js:74:12)
(Use node --trace-warnings ... to show where the warning was created)
(node:1257166) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag --unhandled-rejections=strict (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:1257166) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

and this output on 16.x

Running test 'object/finalizer'
node:internal/process/promises:227
          triggerUncaughtException(err, true /* fromPromise */);
          ^

AssertionError [ERR_ASSERTION]: function should not have been called
    at mustNotCall (/home/midawson/newpull/land/node-addon-api/test/common/index.js:74:12) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: undefined,
  operator: 'fail'
}

So I've reverted it. Since it's in a different test it must be some interaction between the tests.

@mhdawson
Copy link
Member

@JoseExposito could you take a look and then submit and updated PR?

@JoseExposito
Copy link
Contributor Author

Good catch @mhdawson , I overlooked that log. I created a new PR (#937) fixing the issue.

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
PR-URL: nodejs/node-addon-api#928
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: NickNaso <[email protected]>
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.

3 participants