test: add coverage for napi_cancel_async_work#12575
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added node-api 
mhdawsonforce-pushed the cancel_coverage branch 2 times, most recently from 4929d21 to d4ea33dCompare
adding test coverage for napi_cancel_async_work based on coverage report
| NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &global)); | ||
| napi_value result; | ||
| NAPI_CALL_RETURN_VOID(env, | ||
| napi_call_function(env, global, callback, 0, nullptr, &result)); |
There was a problem hiding this comment.
Good point, I had re-used code from the other test in the file, but in this case it should be napi_make_callback
| #if defined _WIN32 | ||
| Sleep(2000); | ||
| #else | ||
| sleep(2); |
There was a problem hiding this comment.
Is this duration guessed? I would assume we could get away with something shorter…
There was a problem hiding this comment.
Yes I just chose a time that I thought would be long enough. Its a trade off between how long the test runs and potentially flaky failures.
There was a problem hiding this comment.
On my machine 1s is fine as well, but I'd lean towards leaving at 2s as I'd rather avoid risking flaky failures.
There was a problem hiding this comment.
I think @nodejs/testing has a general guideline that tests should not take longer than 1 second to run (which makes sense, we have thousands of tests…), that’s why I’m asking
@addaleax Pushed change to update to napi_make_callback |
mhdawson commented Apr 24, 2017
CI run as I'd like to make sure timing works across platforms: https://ci.nodejs.org/job/node-test-pull-request/7635/ |
mhdawson commented Apr 26, 2017
ok going to land this since I think I have addressed @addaleax's comment about time and there are 2 approvals. |
mhdawson commented Apr 26, 2017
mhdawson commented Apr 26, 2017
OS failure was not related, starting new CI run to confirm. - new OSX job https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1010/ |
mhdawson commented Apr 26, 2017
CI re-run due to infra issues: https://ci.nodejs.org/job/node-test-pull-request/7693/ |
mhdawson commented Apr 26, 2017
arms failures in new run seem to be infra issues and not related. |
mhdawson commented Apr 27, 2017
2nd CI run good, arm failures were infra issues, landing. |
mhdawson commented Apr 27, 2017
Landed as 1d96803 |
adding test coverage for napi_cancel_async_work based on coverage report PR-URL: #12575 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
adding test coverage for napi_cancel_async_work based on coverage report PR-URL: nodejs#12575 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
adding test coverage for napi_cancel_async_work based on coverage report Backport-PR-URL: #19447 PR-URL: #12575 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
6 participants
adding test coverage for napi_cancel_async_work based
on coverage report
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test, n-api