n-api: port test for make_callback by digitalinfinity · Pull Request #12409 · nodejs/node · GitHub

Improved test coverage for napi_make_callback by porting the
existing addons/make_callback test to napi

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

n-api, test

cc @nodejs/n-api

FYI, @jasongin - noticed that one of the existing node::makeCallback tests was failing- I've commented that test case out in this test, and we can track that with a separate issue

CI: https://ci.nodejs.org/job/node-test-pull-request/7395/

Thanks @vsemozhetbyt - pushed a quick commit to fix the typo that caused the CI break - I haven't had a chance to fully retest after the latest commit, but I'll try that in the morning. Apologies for the break 😞

CI: https://ci.nodejs.org/job/node-test-pull-request/7396/

This looks good to me, but it’s probably worth mentioning that this doesn’t seem to test the relevant use case for MakeCallback, i.e. entering JS from a async worker’s complete callback without any pre-existing JS stack.

That’s probably best kept for another PR, but something like that should be added.

Why args[1] instead of func?

oversight - I'll change (initially everything was args[blah] but I refactored to use locals but missed updating this reference)

Oh, also: For test-only changes we prefer to use test: instead of another subsystem label, that makes it easier to filter out the actual behavioural changes in our changelogs, it would be good if you could update that here

Thanks @addaleax @benjamingr for reviewing. @addaleax I can rewrite the commit title- is that what you meant, or did you mean I should update the PR title?

@digitalinfinity Yes, updating the commit message would be good (you can squash the commits together at the same time, if you like). Or we just hope the person landing these changes doesn’t overlook these comments. 😄

Thanks @addaleax - rebased and squashed.

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api PR-URL: #12409 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 70b51c8

opened a PR to revert this. It's broken on windows.

Post mortem on having to revert this: I had seen the CI failure on windows but had moved this to the wrong list when I was organizing the set of PRs to land and which needed more time. Since the CI failure only shows on Windows, it didn't show up when I ran make test locally on Ubuntu before landing. The Windows CI failure needs to be looked at then it can be landed again.

@digitalinfinity this is waiting for fixup due to the ci failures.

New CI because the old one is inaccessible: https://ci.nodejs.org/job/node-test-commit/9514/

CI looks really good – I can’t see the old failures anymore, maybe this has been fixed as part of other changes? Can somebody from @nodejs/platform-windows check that this works fine now?

I started a build.
Can anyone tell me what to look for? What were the previous failures?

D:\code\node-cur\test\addons-napi\test_make_callback$ d:\code\node-cur\Debug\node.exe --napi-modules test.js
MyFunc was called with 3 arguments
(node:2820) Warning: N-API is an experimental feature and could change at any time.

D:\code\node-cur\test\addons-napi\test_make_callback$ echo %ERRORLEVEL%
0

and

D:\code\node-cur\test\addons-napi\test_make_callback_recurse$ d:\code\node-cur\Debug\node.exe --napi-modules test.js
(node:10872) Warning: N-API is an experimental feature and could change at any time.

D:\code\node-cur\test\addons-napi\test_make_callback_recurse$ echo %ERRORLEVEL%
0

Whoops, my bad y'all, I missed that this guy hadn't landed yet- I'll fix the lint error and update

Ok, rebased and fixed the lint error- can someone help trigger a CI run please for sanity?

New CI: https://ci.nodejs.org/job/node-test-commit/9652/
Good luck 🤞

gah- it fails on the older version of the compiler for windows- i'll update the PR

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api

Updated the PR to use const instead of constexpr- @nodejs/platform-windows what is the support story for compilers on Windows. The failing case was VS2015, but AFAIK VS2015 has supported constexpr since at least VS2015 Update 1. Do we support older compilers or does Jenkins need to be updated?

New CI - https://ci.nodejs.org/job/node-test-pull-request/7904/
https://ci.nodejs.org/job/node-test-pull-request/7905/

It seems CI is accidentally using the wrong VS version to build the addons, since 12.0 is VS2013 (which is not supported for building Node.js).

Ping @nodejs/build is win2008r2/vs2015 purposefully using Visual Studio 2015 without Update 1?

Independent of the CI configuration, it looks like the CI for the latest commit passed- thanks @kunalspathak for triggering the CI

@jasnell / @addaleax can I land this?

@refack I think so, yes

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api PR-URL: nodejs#12409 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Landed in 73d9c0f

Post land CI: https://ci.nodejs.org/job/node-test-commit/9698/

A note about our CI configuration: All Windows 2008 machines run VS2013, since for building native modules VS2013 is still supported. So, the node-compile-windows job does not run on VS2013 for recent versions, but the node-test-binary-windows job will run the addons tests with VS2013 on the configurations that run in Windows 2008.

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api PR-URL: nodejs#12409 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api PR-URL: nodejs#12409 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api PR-URL: nodejs#12409 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api Backport-PR-URL: #19447 PR-URL: #12409 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api Backport-PR-URL: #19447 PR-URL: #12409 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Apr 14, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:12

addaleax approved these changes Apr 14, 2017

View reviewed changes

addaleax added dont-land-on-v4.x labels Apr 14, 2017

benjamingr approved these changes Apr 14, 2017

View reviewed changes

digitalinfinity force-pushed the napi_makecb_test branch from b77207a to ed5a502 Compare April 14, 2017 21:28

digitalinfinity force-pushed the napi_makecb_test branch from ed5a502 to a880893 Compare April 14, 2017 23:23

jasnell approved these changes Apr 17, 2017

View reviewed changes

jasnell closed this Apr 17, 2017

jasnell reopened this Apr 17, 2017

digitalinfinity force-pushed the napi_makecb_test branch from a880893 to 3d917f9 Compare May 5, 2017 01:41

test: port test for make_callback to n-api

7372a5c

Improved test coverage for napi_make_callback by porting the existing addons/make_callback test to n-api

digitalinfinity force-pushed the napi_makecb_test branch from 3d917f9 to 7372a5c Compare May 5, 2017 18:21

refack closed this May 6, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

n-api: port test for make_callback by digitalinfinity · Pull Request #12409 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: port test for make_callback - #12409

n-api: port test for make_callback#12409digitalinfinity wants to merge 1 commit intonodejs:masternodejs/node:masterfrom digitalinfinity:napi_makecb_testdigitalinfinity/node:napi_makecb_testCopy head branch name to clipboard

Conversation

digitalinfinity commented Apr 14, 2017

Checklist

Affected core subsystem(s)

Uh oh!

digitalinfinity commented Apr 14, 2017

Uh oh!

vsemozhetbyt commented Apr 14, 2017

Uh oh!

digitalinfinity commented Apr 14, 2017

Uh oh!

vsemozhetbyt commented Apr 14, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 14, 2017

Choose a reason for hiding this comment

Uh oh!

digitalinfinity Apr 14, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 14, 2017

Uh oh!

digitalinfinity commented Apr 14, 2017

Uh oh!

addaleax commented Apr 14, 2017

Uh oh!

digitalinfinity commented Apr 14, 2017

Uh oh!

jasnell commented Apr 17, 2017

Uh oh!

jasnell commented Apr 17, 2017

Uh oh!

jasnell commented Apr 17, 2017

Uh oh!

mhdawson commented Apr 27, 2017

Uh oh!

addaleax commented Apr 29, 2017

Uh oh!

addaleax commented Apr 30, 2017

Uh oh!

refack commented Apr 30, 2017

Uh oh!

refack commented Apr 30, 2017

Uh oh!

digitalinfinity commented May 4, 2017

Uh oh!

digitalinfinity commented May 5, 2017

Uh oh!

refack commented May 5, 2017

Uh oh!

digitalinfinity commented May 5, 2017

Uh oh!

digitalinfinity commented May 5, 2017

Uh oh!

kunalspathak commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seishun commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented May 5, 2017

Uh oh!

digitalinfinity commented May 5, 2017

Uh oh!

refack commented May 6, 2017

Uh oh!

addaleax commented May 6, 2017

Uh oh!

refack commented May 6, 2017

Uh oh!

refack commented May 6, 2017

Uh oh!

joaocgreis commented May 9, 2017

n-api: port test for make_callback#12409
digitalinfinity wants to merge 1 commit into
nodejs:masterfrom
digitalinfinity:napi_makecb_test

kunalspathak commented May 5, 2017 •
edited
Loading

seishun commented May 5, 2017 •
edited
Loading