test: add coverage for napi_cancel_async_work#12575
Conversation
nodejs-github-bot
added
node-api
mhdawson
force-pushed
the
cancel_coverage
branch
2 times, most recently
from
4929d21 to
d4ea33d
Compare
adding test coverage for napi_cancel_async_work based on coverage report
mhdawson
force-pushed
the
cancel_coverage
branch
from
d4ea33d to
a73a991
Compare
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 |
|
CI run as I'd like to make sure timing works across platforms: https://ci.nodejs.org/job/node-test-pull-request/7635/ |
|
ok going to land this since I think I have addressed @addaleax's comment about time and there are 2 approvals. |
|
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/ |
|
CI re-run due to infra issues: https://ci.nodejs.org/job/node-test-pull-request/7693/ |
|
arms failures in new run seem to be infra issues and not related. |
|
2nd CI run good, arm failures were infra issues, landing. |
adding test coverage for napi_cancel_async_work based on coverage report PR-URL: #12575 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
adding test coverage for napi_cancel_async_work based on coverage report PR-URL: nodejs#12575 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
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