feat: Deno.test() sanitizes ops and resources (denoland#4399)

This PR brings assertOps and assertResources sanitizers to Deno.test() API. assertOps checks that test doesn't leak async ops, ie. there are no unresolved promises originating from Deno APIs. Enabled by default, can be disabled using Deno.TestDefinition.disableOpSanitizer. assertResources checks that test doesn't leak resources, ie. all resources used in test are closed. For example; if a file is opened during a test case it must be explicitly closed before test case finishes. It's most useful for asynchronous generators. Enabled by default, can be disabled using Deno.TestDefinition.disableResourceSanitizer. We've used those sanitizers in internal runtime tests and it proved very useful in surfacing incorrect tests which resulted in interference between the tests. All tests have been sanitized. Closes denoland#4208
afinch7 · Mar 18, 2020 · 6e2df8c · 6e2df8c
1 parent 070464e
commit 6e2df8c
Show file tree

Hide file tree

Showing 28 changed files with 532 additions and 430 deletions.
diff --git a/cli/js/lib.deno.ns.d.ts b/cli/js/lib.deno.ns.d.ts
@@ -18,6 +18,8 @@ declare namespace Deno {
     fn: TestFunction;
     name: string;
     skip?: boolean;
+    disableOpSanitizer?: boolean;
+    disableResourceSanitizer?: boolean;
   }
 
   /** Register a test which will be run when `deno test` is used on the command

diff --git a/cli/js/testing.ts b/cli/js/testing.ts
@@ -4,6 +4,9 @@ import { exit } from "./ops/os.ts";
 import { Console, stringifyArgs } from "./web/console.ts";
 import { stdout } from "./files.ts";
 import { TextEncoder } from "./web/text_encoding.ts";
+import { metrics } from "./ops/runtime.ts";
+import { resources } from "./ops/resources.ts";
+import { assert } from "./util.ts";
 
 const RED_FAILED = red("FAILED");
 const GREEN_OK = green("ok");
@@ -15,12 +18,59 @@ function formatDuration(time = 0): string {
   return gray(italic(timeStr));
 }
 
+// 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: TestFunction): TestFunction {
+  return async function asyncOpSanitizer(): Promise<void> {
+    const pre = metrics();
+    await fn();
+    const post = metrics();
+    // We're checking diff because one might spawn HTTP server in the background
+    // that will be a pending async op before test starts.
+    const dispatchedDiff = post.opsDispatchedAsync - pre.opsDispatchedAsync;
+    const completedDiff = post.opsCompletedAsync - pre.opsCompletedAsync;
+    assert(
+      dispatchedDiff === completedDiff,
+      `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: TestFunction): TestFunction {
+  return async function resourceSanitizer(): Promise<void> {
+    const pre = resources();
+    await fn();
+    const post = resources();
+
+    const preStr = JSON.stringify(pre, null, 2);
+    const postStr = JSON.stringify(post, null, 2);
+    const msg = `Test case is leaking resources.
+Before: ${preStr}
+After: ${postStr}`;
+    assert(preStr === postStr, msg);
+  };
+}
+
 export type TestFunction = () => void | Promise<void>;
 
 export interface TestDefinition {
   fn: TestFunction;
   name: string;
   skip?: boolean;
+  disableOpSanitizer?: boolean;
+  disableResourceSanitizer?: boolean;
 }
 
 const TEST_REGISTRY: TestDefinition[] = [];
@@ -56,7 +106,16 @@ export function test(
     if (!t.name) {
       throw new TypeError("The test name can't be empty");
     }
-    testDef = { fn: t.fn, name: t.name, skip: Boolean(t.skip) };
+
+    testDef = { ...t, skip: Boolean(t.skip) };
+  }
+
+  if (testDef.disableOpSanitizer !== true) {
+    testDef.fn = assertOps(testDef.fn);
+  }
+
+  if (testDef.disableResourceSanitizer !== true) {
+    testDef.fn = assertResources(testDef.fn);
   }
 
   TEST_REGISTRY.push(testDef);

diff --git a/cli/js/tests/test_util.ts b/cli/js/tests/test_util.ts
@@ -112,47 +112,6 @@ function normalizeTestPermissions(perms: UnitTestPermissions): 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 resourceSanitizer(): Promise<void> {
-    const pre = Deno.resources();
-    await fn();
-    const post = Deno.resources();
-    const msg = `Test case is leaking resources.
-    Before: ${JSON.stringify(pre, null, 2)}
-    After: ${JSON.stringify(post, null, 2)}`;
-    assertEquals(pre, post, msg);
-  };
-}
-
 interface UnitTestPermissions {
   read?: boolean;
   write?: boolean;
@@ -209,7 +168,7 @@ export function unitTest(
 
   const unitTestDefinition: UnitTestDefinition = {
     name,
-    fn: assertResources(assertOps(fn)),
+    fn,
     skip: !!options.skip,
     perms: normalizedPerms
   };

diff --git a/cli/tests/workers_test.ts b/cli/tests/workers_test.ts
@@ -27,87 +27,107 @@ export function createResolvable<T>(): Resolvable<T> {
   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",
-    name: "jsWorker"
-  });
-  const tsWorker = new Worker("../tests/subdir/test_worker.ts", {
-    type: "module",
-    name: "tsWorker"
-  });
-
-  tsWorker.onmessage = (e): void => {
-    assertEquals(e.data, "Hello World");
-    promise.resolve();
-  };
+Deno.test({
+  name: "workersBasic",
+  // FIXME(bartlomieju):
+  disableOpSanitizer: true,
+  fn: async function(): Promise<void> {
+    const promise = createResolvable();
+    const jsWorker = new Worker("../tests/subdir/test_worker.js", {
+      type: "module",
+      name: "jsWorker"
+    });
+    const tsWorker = new Worker("../tests/subdir/test_worker.ts", {
+      type: "module",
+      name: "tsWorker"
+    });
+
+    tsWorker.onmessage = (e): void => {
+      assertEquals(e.data, "Hello World");
+      promise.resolve();
+    };
+
+    jsWorker.onmessage = (e): void => {
+      assertEquals(e.data, "Hello World");
+      tsWorker.postMessage("Hello World");
+    };
+
+    jsWorker.onerror = (e: Event): void => {
+      e.preventDefault();
+      jsWorker.postMessage("Hello World");
+    };
 
-  jsWorker.onmessage = (e): void => {
-    assertEquals(e.data, "Hello World");
-    tsWorker.postMessage("Hello World");
-  };
-
-  jsWorker.onerror = (e: Event): void => {
-    e.preventDefault();
     jsWorker.postMessage("Hello World");
-  };
-
-  jsWorker.postMessage("Hello World");
-  await promise;
+    await promise;
+  }
 });
 
-Deno.test(async function nestedWorker(): Promise<void> {
-  const promise = createResolvable();
-
-  const nestedWorker = new Worker("../tests/subdir/nested_worker.js", {
-    type: "module",
-    name: "nested"
-  });
-
-  nestedWorker.onmessage = (e): void => {
-    assert(e.data.type !== "error");
-    promise.resolve();
-  };
-
-  nestedWorker.postMessage("Hello World");
-  await promise;
+Deno.test({
+  name: "nestedWorker",
+  // FIXME(bartlomieju):
+  disableOpSanitizer: true,
+  fn: async function(): Promise<void> {
+    const promise = createResolvable();
+
+    const nestedWorker = new Worker("../tests/subdir/nested_worker.js", {
+      type: "module",
+      name: "nested"
+    });
+
+    nestedWorker.onmessage = (e): void => {
+      assert(e.data.type !== "error");
+      promise.resolve();
+    };
+
+    nestedWorker.postMessage("Hello World");
+    await promise;
+  }
 });
 
-Deno.test(async function workerThrowsWhenExecuting(): Promise<void> {
-  const promise = createResolvable();
-  const throwingWorker = new Worker("../tests/subdir/throwing_worker.js", {
-    type: "module"
-  });
-
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  throwingWorker.onerror = (e: any): void => {
-    e.preventDefault();
-    assert(/Uncaught Error: Thrown error/.test(e.message));
-    promise.resolve();
-  };
-
-  await promise;
+Deno.test({
+  name: "workerThrowsWhenExecuting",
+  // FIXME(bartlomieju):
+  disableOpSanitizer: true,
+  fn: async function(): Promise<void> {
+    const promise = createResolvable();
+    const throwingWorker = new Worker("../tests/subdir/throwing_worker.js", {
+      type: "module"
+    });
+
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    throwingWorker.onerror = (e: any): void => {
+      e.preventDefault();
+      assert(/Uncaught Error: Thrown error/.test(e.message));
+      promise.resolve();
+    };
+
+    await promise;
+  }
 });
 
-Deno.test(async function workerCanUseFetch(): Promise<void> {
-  const promise = createResolvable();
-
-  const fetchingWorker = new Worker("../tests/subdir/fetching_worker.js", {
-    type: "module"
-  });
-
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  fetchingWorker.onerror = (e: any): void => {
-    e.preventDefault();
-    promise.reject(e.message);
-  };
-
-  // Defer promise.resolve() to allow worker to shut down
-  fetchingWorker.onmessage = (e): void => {
-    assert(e.data === "Done!");
-    promise.resolve();
-  };
-
-  await promise;
+Deno.test({
+  name: "workerCanUseFetch",
+  // FIXME(bartlomieju):
+  disableOpSanitizer: true,
+  fn: async function(): Promise<void> {
+    const promise = createResolvable();
+
+    const fetchingWorker = new Worker("../tests/subdir/fetching_worker.js", {
+      type: "module"
+    });
+
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    fetchingWorker.onerror = (e: any): void => {
+      e.preventDefault();
+      promise.reject(e.message);
+    };
+
+    // Defer promise.resolve() to allow worker to shut down
+    fetchingWorker.onmessage = (e): void => {
+      assert(e.data === "Done!");
+      promise.resolve();
+    };
+
+    await promise;
+  }
 });
diff --git a/std/encoding/binary_test.ts b/std/encoding/binary_test.ts
@@ -160,5 +160,3 @@ Deno.test(async function testWriteVarnumLittleEndian(): Promise<void> {
   await buff.read(data);
   assertEquals(data, new Uint8Array([0x01, 0x02, 0x03, 0x04]));
 });
-
-Deno.runTests();