add assertOps sanitizer in cli/js/ unit tests #4209
Conversation
bartlomieju
force-pushed
the
op_sanitizer
branch
from
ef686f8
to
244c2cb
Compare
bartlomieju
force-pushed
the
op_sanitizer
branch
from
244c2cb
to
957e9ac
Compare
cli/js/signal_test.ts
Outdated
| // Defer for a moment to allow interval promise to resolve | ||
| await defer(20); |
There was a problem hiding this comment.
This is bad - it was not signal implementation that was leaking ops but interval. Because our internal timer implementation uses promises; after clearing an interval, one actually needs to wait until timer promise resolves. Otherwise that's considered a leak because timers are based on "ops".
There was a problem hiding this comment.
Can you add a TODO here explaining the situation? We should adjust the interval implementation to avoid this.
There was a problem hiding this comment.
Added more description and FIXME in cli/js/timers.ts
cli/js/signal_test.ts
Outdated
| // Defer for a moment to allow interval promise to resolve | ||
| await defer(20); |
There was a problem hiding this comment.
Can you add a TODO here explaining the situation? We should adjust the interval implementation to avoid this.
| postMessage(e.data); | ||
| close(); | ||
| }; | ||
|
|
||
| onerror = function() { | ||
| console.log("called onerror in worker"); | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
I don’t understand why all these console.log statements are being removed?
There was a problem hiding this comment.
Not needed and was harder to match output in integration test
| fetchingWorker.onmessage = (e): void => { | ||
| assert(e.data === "Done!"); | ||
| promise.resolve(); | ||
| }; | ||
|
|
||
| await promise; | ||
| }); | ||
|
|
||
| await Deno.runTests(); |
There was a problem hiding this comment.
Is moving this workers_test related to the assertOps?
There was a problem hiding this comment.
Yes, workers are leaking ops at the moment because there is no way to terminate worker (not implemented yet), added FIXME at the top of the file
* denoland/master: (22 commits) fix event target tests Support async function and EventListenerObject as listeners (denoland#4240) Allow BadResource errors to take a custom message (denoland#4251) Document TypeScript compiler options (denoland#4241) refactor: preliminary cleanup of Deno.runTests() (denoland#4237) refactor: cleanup compiler runtimes (denoland#4230) Use discord instead of gitter (denoland#4253) Remove unnecessary macro from cli/ops/tty.rs (denoland#4254) Remove Deno.errors.Other (denoland#4249) refactor: rewrite testPerm into unitTest (denoland#4231) Migrate internal bundles to System (denoland#4233) Fix inlining of lib.dom.iterable.d.ts. (denoland#4242) Fix `deno install` file name including extra dot on Windows (denoland#4243) assert build success for test plugin (denoland#4223) Disable flaky and broken tests (denoland#4239) add assertOps sanitizer in cli/js/ unit tests (denoland#4209) misc: reduce unnecesarry output in cli/js tests (denoland#4182) feat(std/node): add directory classes (denoland#4087) Do not convert exceptions to JSON and back (denoland#4214) Don't reset exception handle after calling ErrWithV8Handle::get_handle() (denoland#4214) ...
* add "assertOps" test assertion which makes sure test case is not "leaking" ops - ie. after test finishes there are no pending async ops * apply "assertOps" to all tests in "cli/js/" * fix numerous tests leaking ops * document problem with edge case in "clearInterval" and "clearTimeout" implementation where they may leak async ops * move "cli/js/worker_test.ts" to "cli/tests/worker_test.ts" and run as integration test; workers leak ops because of missing "terminate" implementation
Follow up to #4161; this PR adds
assertOpssanitizer that ensures that test case does not have "leaking" async ops - ie. there are no unresolved promises. Having dangling promises from async ops leads to very strange behaviors in tests that we have experienced multiple times.Obviously there are quite a few tests in
cli/js/that leak ops :))