From ee452ad883c1c711839655a307b39e8eea5bf410 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Tue, 3 Mar 2020 18:22:53 +0100
Subject: [PATCH] add assertOps sanitizer in cli/js/ unit tests (#4209)

* 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
---
 cli/js/net_test.ts                    |  2 +-
 cli/js/performance_test.ts            |  7 ++-
 cli/js/signal_test.ts                 | 47 +++++++++++--------
 cli/js/test_util.ts                   | 30 +++++++++++-
 cli/js/timers.ts                      |  8 ++++
 cli/js/timers_test.ts                 | 66 +++++++++++++--------------
 cli/js/unit_tests.ts                  |  1 -
 cli/tests/026_workers.ts              | 26 -----------
 cli/tests/026_workers.ts.out          |  7 ---
 cli/tests/integration_tests.rs        | 12 ++---
 cli/tests/subdir/nested_worker.js     |  1 -
 cli/tests/subdir/test_worker.js       |  3 --
 cli/tests/subdir/test_worker.ts       |  1 -
 cli/tests/subdir/test_worker_basic.js |  2 -
 cli/tests/workers_basic.out           |  3 --
 cli/tests/workers_basic.ts            | 11 -----
 cli/tests/workers_test.out            |  7 +++
 cli/{js => tests}/workers_test.ts     | 46 ++++++++++++++-----
 18 files changed, 147 insertions(+), 133 deletions(-)
 delete mode 100644 cli/tests/026_workers.ts
 delete mode 100644 cli/tests/026_workers.ts.out
 delete mode 100644 cli/tests/workers_basic.out
 delete mode 100644 cli/tests/workers_basic.ts
 create mode 100644 cli/tests/workers_test.out
 rename cli/{js => tests}/workers_test.ts (58%)

diff --git a/cli/js/net_test.ts b/cli/js/net_test.ts
index 8a53a1c742..41d012c563 100644
--- a/cli/js/net_test.ts
+++ b/cli/js/net_test.ts
@@ -54,7 +54,7 @@ testPerm({ net: true }, async function netTcpConcurrentAccept(): Promise<void> {
   const p1 = listener.accept().catch(checkErr);
   await Promise.race([p, p1]);
   listener.close();
-  await [p, p1];
+  await Promise.all([p, p1]);
   assertEquals(acceptErrCount, 1);
 });
 
diff --git a/cli/js/performance_test.ts b/cli/js/performance_test.ts
index 7e7f63d8ca..2275149549 100644
--- a/cli/js/performance_test.ts
+++ b/cli/js/performance_test.ts
@@ -1,10 +1,13 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
-import { testPerm, assert } from "./test_util.ts";
+import { testPerm, assert, createResolvable } from "./test_util.ts";
 
-testPerm({ hrtime: false }, function now(): void {
+testPerm({ hrtime: false }, async function performanceNow(): Promise<void> {
+  const resolvable = createResolvable();
   const start = performance.now();
   setTimeout((): void => {
     const end = performance.now();
     assert(end - start >= 10);
+    resolvable.resolve();
   }, 10);
+  await resolvable;
 });
diff --git a/cli/js/signal_test.ts b/cli/js/signal_test.ts
index 1c86584777..a0588b987c 100644
--- a/cli/js/signal_test.ts
+++ b/cli/js/signal_test.ts
@@ -4,7 +4,8 @@ import {
   testPerm,
   assert,
   assertEquals,
-  assertThrows
+  assertThrows,
+  createResolvable
 } from "./test_util.ts";
 
 function defer(n: number): Promise<void> {
@@ -101,15 +102,13 @@ if (Deno.build.os === "win") {
     );
   });
 } else {
-  testPerm({ run: true, net: true }, async function signalStreamTest(): Promise<
-    void
-  > {
+  testPerm({ run: true }, async function signalStreamTest(): Promise<void> {
+    const resolvable = createResolvable();
     // This prevents the program from exiting.
     const t = setInterval(() => {}, 1000);
 
     let c = 0;
     const sig = Deno.signal(Deno.Signal.SIGUSR1);
-
     setTimeout(async () => {
       await defer(20);
       for (const _ of Array(3)) {
@@ -118,6 +117,7 @@ if (Deno.build.os === "win") {
         await defer(20);
       }
       sig.dispose();
+      resolvable.resolve();
     });
 
     for await (const _ of sig) {
@@ -126,25 +126,32 @@ if (Deno.build.os === "win") {
 
     assertEquals(c, 3);
 
-    clearTimeout(t);
+    clearInterval(t);
+    // Defer for a moment to allow async op from `setInterval` to resolve;
+    // for more explanation see `FIXME` in `cli/js/timers.ts::setGlobalTimeout`
+    await defer(20);
+    await resolvable;
   });
 
-  testPerm(
-    { run: true, net: true },
-    async function signalPromiseTest(): Promise<void> {
-      // This prevents the program from exiting.
-      const t = setInterval(() => {}, 1000);
+  testPerm({ run: true }, async function signalPromiseTest(): Promise<void> {
+    const resolvable = createResolvable();
+    // This prevents the program from exiting.
+    const t = setInterval(() => {}, 1000);
 
-      const sig = Deno.signal(Deno.Signal.SIGUSR1);
-      setTimeout(() => {
-        Deno.kill(Deno.pid, Deno.Signal.SIGUSR1);
-      }, 20);
-      await sig;
-      sig.dispose();
+    const sig = Deno.signal(Deno.Signal.SIGUSR1);
+    setTimeout(() => {
+      Deno.kill(Deno.pid, Deno.Signal.SIGUSR1);
+      resolvable.resolve();
+    }, 20);
+    await sig;
+    sig.dispose();
 
-      clearTimeout(t);
-    }
-  );
+    clearInterval(t);
+    // Defer for a moment to allow async op from `setInterval` to resolve;
+    // for more explanation see `FIXME` in `cli/js/timers.ts::setGlobalTimeout`
+    await defer(20);
+    await resolvable;
+  });
 
   testPerm({ run: true }, async function signalShorthandsTest(): Promise<void> {
     let s: Deno.SignalStream;
diff --git a/cli/js/test_util.ts b/cli/js/test_util.ts
index 6f52c01df2..027610dd92 100644
--- a/cli/js/test_util.ts
+++ b/cli/js/test_util.ts
@@ -106,11 +106,37 @@ function normalizeTestPermissions(perms: TestPermissions): Permissions {
   };
 }
 
+// Wrap `TestFunction` in additional assertion that makes sure
+// the test case does not leak async "ops" - ie. number of async
+// completed ops after the test is the same as number of dispatched
+// ops. Note that "unref" ops are ignored since in nature that are
+// optional.
+function assertOps(fn: Deno.TestFunction): Deno.TestFunction {
+  return async function asyncOpSanitizer(): Promise<void> {
+    const pre = Deno.metrics();
+    await fn();
+    const post = Deno.metrics();
+    // We're checking diff because one might spawn HTTP server in the background
+    // that will be a pending async op before test starts.
+    assertEquals(
+      post.opsDispatchedAsync - pre.opsDispatchedAsync,
+      post.opsCompletedAsync - pre.opsCompletedAsync,
+      `Test case is leaking async ops.
+    Before:
+      - dispatched: ${pre.opsDispatchedAsync}
+      - completed: ${pre.opsCompletedAsync}
+    After: 
+      - dispatched: ${post.opsDispatchedAsync}
+      - completed: ${post.opsCompletedAsync}`
+    );
+  };
+}
+
 // Wrap `TestFunction` in additional assertion that makes sure
 // the test case does not "leak" resources - ie. resource table after
 // the test has exactly the same contents as before the test.
 function assertResources(fn: Deno.TestFunction): Deno.TestFunction {
-  return async function(): Promise<void> {
+  return async function resourceSanitizer(): Promise<void> {
     const preResources = Deno.resources();
     await fn();
     const postResources = Deno.resources();
@@ -131,7 +157,7 @@ export function testPerm(perms: TestPermissions, fn: Deno.TestFunction): void {
     return;
   }
 
-  Deno.test(fn.name, assertResources(fn));
+  Deno.test(fn.name, assertResources(assertOps(fn)));
 }
 
 export function test(fn: Deno.TestFunction): void {
diff --git a/cli/js/timers.ts b/cli/js/timers.ts
index a92896efd6..2b29f2a550 100644
--- a/cli/js/timers.ts
+++ b/cli/js/timers.ts
@@ -43,6 +43,14 @@ async function setGlobalTimeout(due: number, now: number): Promise<void> {
   // Send message to the backend.
   globalTimeoutDue = due;
   pendingEvents++;
+  // FIXME(bartlomieju): this is problematic, because `clearGlobalTimeout`
+  // is synchronous. That means that timer is cancelled, but this promise is still pending
+  // until next turn of event loop. This leads to "leaking of async ops" in tests;
+  // because `clearTimeout/clearInterval` might be the last statement in test function
+  // `opSanitizer` will immediately complain that there is pending op going on, unless
+  // some timeout/defer is put in place to allow promise resolution.
+  // Ideally `clearGlobalTimeout` doesn't return until this op is resolved, but
+  // I'm not if that's possible.
   await sendAsync("op_global_timer", { timeout });
   pendingEvents--;
   // eslint-disable-next-line @typescript-eslint/no-use-before-define
diff --git a/cli/js/timers_test.ts b/cli/js/timers_test.ts
index 06367fc6d5..8cb505ee4f 100644
--- a/cli/js/timers_test.ts
+++ b/cli/js/timers_test.ts
@@ -1,5 +1,11 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
-import { test, assert, assertEquals, assertNotEquals } from "./test_util.ts";
+import {
+  test,
+  createResolvable,
+  assert,
+  assertEquals,
+  assertNotEquals
+} from "./test_util.ts";
 
 function deferred(): {
   promise: Promise<{}>;
@@ -178,8 +184,6 @@ test(async function timeoutCallbackThis(): Promise<void> {
 });
 
 test(async function timeoutBindThis(): Promise<void> {
-  function noop(): void {}
-
   const thisCheckPassed = [null, undefined, window, globalThis];
 
   const thisCheckFailed = [
@@ -194,41 +198,37 @@ test(async function timeoutBindThis(): Promise<void> {
     Object.prototype
   ];
 
-  thisCheckPassed.forEach(
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    (thisArg: any): void => {
-      let hasThrown = 0;
-      try {
-        setTimeout.call(thisArg, noop, 1);
-        hasThrown = 1;
-      } catch (err) {
-        if (err instanceof TypeError) {
-          hasThrown = 2;
-        } else {
-          hasThrown = 3;
-        }
+  for (const thisArg of thisCheckPassed) {
+    const resolvable = createResolvable();
+    let hasThrown = 0;
+    try {
+      setTimeout.call(thisArg, () => resolvable.resolve(), 1);
+      hasThrown = 1;
+    } catch (err) {
+      if (err instanceof TypeError) {
+        hasThrown = 2;
+      } else {
+        hasThrown = 3;
       }
-      assertEquals(hasThrown, 1);
     }
-  );
+    await resolvable;
+    assertEquals(hasThrown, 1);
+  }
 
-  thisCheckFailed.forEach(
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    (thisArg: any): void => {
-      let hasThrown = 0;
-      try {
-        setTimeout.call(thisArg, noop, 1);
-        hasThrown = 1;
-      } catch (err) {
-        if (err instanceof TypeError) {
-          hasThrown = 2;
-        } else {
-          hasThrown = 3;
-        }
+  for (const thisArg of thisCheckFailed) {
+    let hasThrown = 0;
+    try {
+      setTimeout.call(thisArg, () => {}, 1);
+      hasThrown = 1;
+    } catch (err) {
+      if (err instanceof TypeError) {
+        hasThrown = 2;
+      } else {
+        hasThrown = 3;
       }
-      assertEquals(hasThrown, 2);
     }
-  );
+    assertEquals(hasThrown, 2);
+  }
 });
 
 test(async function clearTimeoutShouldConvertToNumber(): Promise<void> {
diff --git a/cli/js/unit_tests.ts b/cli/js/unit_tests.ts
index f20e2f137c..5093ce0b2d 100644
--- a/cli/js/unit_tests.ts
+++ b/cli/js/unit_tests.ts
@@ -62,7 +62,6 @@ import "./utime_test.ts";
 import "./write_file_test.ts";
 import "./performance_test.ts";
 import "./version_test.ts";
-import "./workers_test.ts";
 
 if (import.meta.main) {
   await Deno.runTests();
diff --git a/cli/tests/026_workers.ts b/cli/tests/026_workers.ts
deleted file mode 100644
index 7ec6996e11..0000000000
--- a/cli/tests/026_workers.ts
+++ /dev/null
@@ -1,26 +0,0 @@
-const jsWorker = new Worker("./subdir/test_worker.js", {
-  type: "module",
-  name: "jsWorker"
-});
-const tsWorker = new Worker("./subdir/test_worker.ts", {
-  type: "module",
-  name: "tsWorker"
-});
-
-tsWorker.onmessage = (e): void => {
-  console.log("Received ts: " + e.data);
-};
-
-jsWorker.onmessage = (e): void => {
-  console.log("Received js: " + e.data);
-
-  tsWorker.postMessage("Hello World");
-};
-
-jsWorker.onerror = (e: Event): void => {
-  e.preventDefault();
-  console.log("called onerror in script");
-  jsWorker.postMessage("Hello World");
-};
-
-jsWorker.postMessage("Hello World");
diff --git a/cli/tests/026_workers.ts.out b/cli/tests/026_workers.ts.out
deleted file mode 100644
index 92f7550ad1..0000000000
--- a/cli/tests/026_workers.ts.out
+++ /dev/null
@@ -1,7 +0,0 @@
-Hello World
-called onerror in worker
-called onerror in script
-Hello World
-Received js: Hello World
-Hello World
-Received ts: Hello World
diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs
index a475e9111a..5cd1f34315 100644
--- a/cli/tests/integration_tests.rs
+++ b/cli/tests/integration_tests.rs
@@ -828,14 +828,10 @@ itest!(_026_redirect_javascript {
   http_server: true,
 });
 
-itest!(_026_workers {
-  args: "run --reload 026_workers.ts",
-  output: "026_workers.ts.out",
-});
-
-itest!(workers_basic {
-  args: "run --reload workers_basic.ts",
-  output: "workers_basic.out",
+itest!(workers {
+  args: "run --reload --allow-net workers_test.ts",
+  http_server: true,
+  output: "workers_test.out",
 });
 
 itest!(_027_redirect_typescript {
diff --git a/cli/tests/subdir/nested_worker.js b/cli/tests/subdir/nested_worker.js
index b0acd70d75..a4eed723ae 100644
--- a/cli/tests/subdir/nested_worker.js
+++ b/cli/tests/subdir/nested_worker.js
@@ -9,7 +9,6 @@ jsWorker.onerror = _e => {
 };
 
 jsWorker.onmessage = e => {
-  console.log("js worker on message");
   postMessage({ type: "msg", text: e });
   close();
 };
diff --git a/cli/tests/subdir/test_worker.js b/cli/tests/subdir/test_worker.js
index 70e1d8b731..9c1e555b51 100644
--- a/cli/tests/subdir/test_worker.js
+++ b/cli/tests/subdir/test_worker.js
@@ -5,8 +5,6 @@ if (self.name !== "jsWorker") {
 }
 
 onmessage = function(e) {
-  console.log(e.data);
-
   if (thrown === false) {
     thrown = true;
     throw new SyntaxError("[test error]");
@@ -17,6 +15,5 @@ onmessage = function(e) {
 };
 
 onerror = function() {
-  console.log("called onerror in worker");
   return false;
 };
diff --git a/cli/tests/subdir/test_worker.ts b/cli/tests/subdir/test_worker.ts
index 2ea8f9214c..1f924c0736 100644
--- a/cli/tests/subdir/test_worker.ts
+++ b/cli/tests/subdir/test_worker.ts
@@ -3,7 +3,6 @@ if (self.name !== "tsWorker") {
 }
 
 onmessage = function(e): void {
-  console.log(e.data);
   postMessage(e.data);
   close();
 };
diff --git a/cli/tests/subdir/test_worker_basic.js b/cli/tests/subdir/test_worker_basic.js
index db00b6d0c7..aef1658c0f 100644
--- a/cli/tests/subdir/test_worker_basic.js
+++ b/cli/tests/subdir/test_worker_basic.js
@@ -6,12 +6,10 @@ if (self.name !== "jsWorker") {
 }
 
 onmessage = function(e) {
-  console.log("jsWorker onmessage", e.data);
   postMessage(e.data);
   close();
 };
 
 onerror = function() {
-  console.log("called onerror in worker");
   return false;
 };
diff --git a/cli/tests/workers_basic.out b/cli/tests/workers_basic.out
deleted file mode 100644
index 15c5735303..0000000000
--- a/cli/tests/workers_basic.out
+++ /dev/null
@@ -1,3 +0,0 @@
-hello from test_worker_basic.js
-jsWorker onmessage msg1
-main recv: msg1
diff --git a/cli/tests/workers_basic.ts b/cli/tests/workers_basic.ts
deleted file mode 100644
index 64bd58fcca..0000000000
--- a/cli/tests/workers_basic.ts
+++ /dev/null
@@ -1,11 +0,0 @@
-// Tests basic postMessage, close, onmessage
-const jsWorker = new Worker("./subdir/test_worker_basic.js", {
-  type: "module",
-  name: "jsWorker"
-});
-
-jsWorker.onmessage = (e): void => {
-  console.log("main recv: " + e.data);
-};
-
-jsWorker.postMessage("msg1");
diff --git a/cli/tests/workers_test.out b/cli/tests/workers_test.out
new file mode 100644
index 0000000000..691301f810
--- /dev/null
+++ b/cli/tests/workers_test.out
@@ -0,0 +1,7 @@
+running 4 tests
+OK     workersBasic [WILDCARD]
+OK     nestedWorker [WILDCARD]
+OK     workerThrowsWhenExecuting [WILDCARD]
+OK     workerCanUseFetch [WILDCARD]
+
+test result: OK 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out [WILDCARD]
\ No newline at end of file
diff --git a/cli/js/workers_test.ts b/cli/tests/workers_test.ts
similarity index 58%
rename from cli/js/workers_test.ts
rename to cli/tests/workers_test.ts
index 5b8b1ef974..44e7a17766 100644
--- a/cli/js/workers_test.ts
+++ b/cli/tests/workers_test.ts
@@ -1,13 +1,33 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
-import {
-  test,
-  testPerm,
-  assert,
-  assertEquals,
-  createResolvable
-} from "./test_util.ts";
 
-test(async function workersBasic(): Promise<void> {
+// Requires to be run with `--allow-net` flag
+
+// FIXME(bartlomieju): this file is an integration test only because
+// workers are leaking ops at the moment - `worker.terminate()` is not
+// yet implemented. Once it gets implemented this file should be
+// again moved to `cli/js/` as an unit test file.
+
+import { assert, assertEquals } from "../../std/testing/asserts.ts";
+
+export interface ResolvableMethods<T> {
+  resolve: (value?: T | PromiseLike<T>) => void;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  reject: (reason?: any) => void;
+}
+
+export type Resolvable<T> = Promise<T> & ResolvableMethods<T>;
+
+export function createResolvable<T>(): Resolvable<T> {
+  let methods: ResolvableMethods<T>;
+  const promise = new Promise<T>((resolve, reject): void => {
+    methods = { resolve, reject };
+  });
+  // TypeScript doesn't know that the Promise callback occurs synchronously
+  // therefore use of not null assertion (`!`)
+  return Object.assign(promise, methods!) as Resolvable<T>;
+}
+
+Deno.test(async function workersBasic(): Promise<void> {
   const promise = createResolvable();
   const jsWorker = new Worker("../tests/subdir/test_worker.js", {
     type: "module",
@@ -37,7 +57,7 @@ test(async function workersBasic(): Promise<void> {
   await promise;
 });
 
-test(async function nestedWorker(): Promise<void> {
+Deno.test(async function nestedWorker(): Promise<void> {
   const promise = createResolvable();
 
   const nestedWorker = new Worker("../tests/subdir/nested_worker.js", {
@@ -54,9 +74,8 @@ test(async function nestedWorker(): Promise<void> {
   await promise;
 });
 
-test(async function workerThrowsWhenExecuting(): Promise<void> {
+Deno.test(async function workerThrowsWhenExecuting(): Promise<void> {
   const promise = createResolvable();
-
   const throwingWorker = new Worker("../tests/subdir/throwing_worker.js", {
     type: "module"
   });
@@ -71,7 +90,7 @@ test(async function workerThrowsWhenExecuting(): Promise<void> {
   await promise;
 });
 
-testPerm({ net: true }, async function workerCanUseFetch(): Promise<void> {
+Deno.test(async function workerCanUseFetch(): Promise<void> {
   const promise = createResolvable();
 
   const fetchingWorker = new Worker("../tests/subdir/fetching_worker.js", {
@@ -84,6 +103,7 @@ testPerm({ net: true }, async function workerCanUseFetch(): Promise<void> {
     promise.reject(e.message);
   };
 
+  // Defer promise.resolve() to allow worker to shut down
   fetchingWorker.onmessage = (e): void => {
     assert(e.data === "Done!");
     promise.resolve();
@@ -91,3 +111,5 @@ testPerm({ net: true }, async function workerCanUseFetch(): Promise<void> {
 
   await promise;
 });
+
+await Deno.runTests();