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

n-api: port test for make_callback #12409

Closed

Conversation

@digitalinfinity
Copy link
Contributor

commented Apr 14, 2017

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

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

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

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

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 😞

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

@addaleax
Copy link
Member

left a comment

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.

test/addons-napi/test_make_callback/binding.cc Outdated

napi_valuetype func_type;

NAPI_CALL(env, napi_typeof(env, args[1], &func_type));

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 14, 2017

Member

Why args[1] instead of func?

This comment has been minimized.

Copy link
@digitalinfinity

digitalinfinity Apr 14, 2017

Author Contributor

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

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 14, 2017

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

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

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?

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 14, 2017

@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. 😄

@digitalinfinity digitalinfinity force-pushed the digitalinfinity:napi_makecb_test branch to ed5a502 Apr 14, 2017

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

Thanks @addaleax - rebased and squashed.

@digitalinfinity digitalinfinity force-pushed the digitalinfinity:napi_makecb_test branch from ed5a502 Apr 14, 2017

jasnell added a commit that referenced this pull request Apr 17, 2017
test: port test for make_callback to n-api
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>
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

Landed in 70b51c8

@jasnell jasnell closed this Apr 17, 2017

@jasnell jasnell reopened this Apr 17, 2017

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

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.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

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

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

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

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 30, 2017

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?

@refack

This comment has been minimized.

Copy link
Member

commented Apr 30, 2017

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

@refack

This comment has been minimized.

Copy link
Member

commented Apr 30, 2017

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
@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2017

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

@digitalinfinity digitalinfinity force-pushed the digitalinfinity:napi_makecb_test branch to 3d917f9 May 5, 2017

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

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

@refack

This comment has been minimized.

Copy link
Member

commented May 5, 2017

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

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

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

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

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

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?

@kunalspathak

This comment has been minimized.

@seishun

This comment has been minimized.

Copy link
Member

commented May 5, 2017

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).

@refack

This comment has been minimized.

Copy link
Member

commented May 5, 2017

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

@digitalinfinity

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

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

@refack

This comment has been minimized.

Copy link
Member

commented May 6, 2017

@jasnell / @addaleax can I land this?

@addaleax

This comment has been minimized.

Copy link
Member

commented May 6, 2017

@refack I think so, yes

refack added a commit to refack/node that referenced this pull request May 6, 2017
test: port test for make_callback to n-api
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>
@refack

This comment has been minimized.

Copy link
Member

commented May 6, 2017

Landed in 73d9c0f

@refack refack closed this May 6, 2017

@refack

This comment has been minimized.

Copy link
Member

commented May 6, 2017

@joaocgreis

This comment has been minimized.

Copy link
Member

commented May 9, 2017

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.

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: port test for make_callback to n-api
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>
@jasnell jasnell referenced this pull request May 11, 2017
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 11, 2018
test: port test for make_callback to n-api
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018
test: port test for make_callback to n-api
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018
test: port test for make_callback to n-api
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 31, 2018
test: port test for make_callback to n-api
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
test: port test for make_callback to n-api
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>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
test: port test for make_callback to n-api
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>
MylesBorins added a commit that referenced this pull request Apr 16, 2018
test: port test for make_callback to n-api
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>
MylesBorins added a commit that referenced this pull request Apr 16, 2018
test: port test for make_callback to n-api
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>
@MylesBorins MylesBorins referenced this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.