test: use async/await for gc #773
Conversation
* Use promises and async/await to ensure that one test finishes before another one starts. This prevents errors thrown in one test from appearing to originate from another test. * Perform garbage collection consistently, via `testUtil.runGCTests()`, to ensure that it is performed correctly and that its results are awaited.
gabrielschulhof
force-pushed
the
async-await-for-gc
branch
from
e3a324b
to
5d36882
Compare
|
@mhdawson I actually had to make some more substantial changes, because I realized that, in order for exceptions to be reported from the correct test, we need to make sure that the tests are properly sequenced. This means that all tests that do things asynchronously must hold up the loop in test/index.js which advances through the list of tests it is given. A symptom of what we have without this change is that, when you run the tests, it will print out |
This reduces the number of promises created internally
gabrielschulhof
force-pushed
the
async-await-for-gc
branch
from
5a26d3e
to
5ba003c
Compare
| binding.queueWork(worker); | ||
| } | ||
|
|
||
| function cancel(binding) { |
There was a problem hiding this comment.
Had to remove this test, because create → queue → cancel is a racy operation. After queueing completes there is a small chance that by the time the next line – cancel – executes, the work will already have been queued. This was happening on
https://ci.nodejs.org/job/node-test-node-addon-api-new/2520/nodes=debian8-64/console
Error: Unknown failure
at /home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/asyncprogressqueueworker.js:67:13
at new Promise (<anonymous>)
at cancel (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/asyncprogressqueueworker.js:52:10)
at test (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/asyncprogressqueueworker.js:13:9)
at async /home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/index.js:102:5
We have a similar test in core:
I can only conclude that it works in core because the stack is shallower and therefore the distance between queue and cancel is shorter and so it happens to work.
|
@mhdawson I got the tests to pass but I had to remove one that was unfixably racy and failing consistently on our CI, although not on all platforms. It was AsyncProgressQueueWorker cancel. |
|
Does this PR depend on nodejs/node#34386? |
|
@legendecas the fact that nodejs/node#34386 has landed is causing node-addon-api PRs to fail when testing against v15.x. Landing this will fix those failures, and makes it possible to start landing node-addon-api PRs again. |
|
😄👍, just wondering if the changes will break on Node.js versions that not backported 34386. So the question is clarified. LGTM. |
|
@legendecas if you find the PR to be LGTM could you please approve it? |
* Use promises and async/await to ensure that one test finishes before another one starts. This prevents errors thrown in one test from appearing to originate from another test. * Perform garbage collection consistently, via `testUtil.runGCTests()`, to ensure that it is performed correctly and that its results are awaited. PR-URL: #773 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
|
Landed in 461e364. |
* Use promises and async/await to ensure that one test finishes before another one starts. This prevents errors thrown in one test from appearing to originate from another test. * Perform garbage collection consistently, via `testUtil.runGCTests()`, to ensure that it is performed correctly and that its results are awaited. PR-URL: nodejs/node-addon-api#773 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* Use promises and async/await to ensure that one test finishes before another one starts. This prevents errors thrown in one test from appearing to originate from another test. * Perform garbage collection consistently, via `testUtil.runGCTests()`, to ensure that it is performed correctly and that its results are awaited. PR-URL: nodejs/node-addon-api#773 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* Use promises and async/await to ensure that one test finishes before another one starts. This prevents errors thrown in one test from appearing to originate from another test. * Perform garbage collection consistently, via `testUtil.runGCTests()`, to ensure that it is performed correctly and that its results are awaited. PR-URL: nodejs/node-addon-api#773 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* Use promises and async/await to ensure that one test finishes before another one starts. This prevents errors thrown in one test from appearing to originate from another test. * Perform garbage collection consistently, via `testUtil.runGCTests()`, to ensure that it is performed correctly and that its results are awaited. PR-URL: nodejs/node-addon-api#773 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Convert tests that gc into async functions that await 10 ticks after
each gc. This is because gc has become async and because references
will soon be deleted via a native
SetImmediate().Re: nodejs/node#34386