refactor: preliminary cleanup of Deno.runTests() (#4237)

* refactor: preliminary cleanup of Deno.runTests()

* Change time measurement to use new Date() instead of
  performance.now(). Because there is no guarantee that tests are
  run with "--allow-hr" using new Date() guarantees higher
  precision of 1ms instead of 2ms.

* Support String type filter in "skip" and "only".

* Split "exitOnFail" into "exitOnFail" and "failFast".
  Former tells if "runTests()" should exit with code 1 on test
  failure, while latter tells if "runTests()" should stop
  running tests on first failure.

* Use "defer" to wait for unhandled promise rejection - this bit
  is funky and doesn't seem right, but for now it's just a rewrite
  from using "setTimeout". Intended to be fixed in later commits.

* Remove global "__DENO_TEST_REGISTRY", don't expose list of
  registered tests (to be addressed in follow up commits)

* Remove arbitrary slow test threshold; use uniform coloring
  instead

1 files changed, 71 insertions, 57 deletions

diff --git a/cli/js/testing.ts b/cli/js/testing.ts
index b4c86e8b8..4283f73d7 100644
--- a/cli/js/testing.ts
+++ b/cli/js/testing.ts
@@ -1,20 +1,17 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
-import { red, green, bgRed, bold, white, gray, italic } from "./colors.ts";
+import { red, green, bgRed, gray, italic } from "./colors.ts";
 import { exit } from "./os.ts";
 import { Console } from "./console.ts";
 
-function formatTestTime(time = 0): string {
-  return `${time.toFixed(2)}ms`;
+function formatDuration(time = 0): string {
+  const timeStr = `(${time}ms)`;
+  return gray(italic(timeStr));
 }
 
-function promptTestTime(time = 0, displayWarning = false): string {
-  // if time > 5s we display a warning
-  // only for test time, not the full runtime
-  if (displayWarning && time >= 5000) {
-    return bgRed(white(bold(`(${formatTestTime(time)})`)));
-  } else {
-    return gray(italic(`(${formatTestTime(time)})`));
-  }
+function defer(n: number): Promise<void> {
+  return new Promise((resolve: () => void, _) => {
+    setTimeout(resolve, n);
+  });
 }
 
 export type TestFunction = () => void | Promise<void>;
@@ -24,22 +21,7 @@ export interface TestDefinition {
   name: string;
 }
 
-declare global {
-  // Only `var` variables show up in the `globalThis` type when doing a global
-  // scope augmentation.
-  // eslint-disable-next-line no-var
-  var __DENO_TEST_REGISTRY: TestDefinition[];
-}
-
-let TEST_REGISTRY: TestDefinition[] = [];
-if (globalThis["__DENO_TEST_REGISTRY"]) {
-  TEST_REGISTRY = globalThis.__DENO_TEST_REGISTRY as TestDefinition[];
-} else {
-  Object.defineProperty(globalThis, "__DENO_TEST_REGISTRY", {
-    enumerable: false,
-    value: TEST_REGISTRY
-  });
-}
+const TEST_REGISTRY: TestDefinition[] = [];
 
 export function test(t: TestDefinition): void;
 export function test(fn: TestFunction): void;
@@ -97,20 +79,48 @@ interface TestCase {
 
 export interface RunTestsOptions {
   exitOnFail?: boolean;
-  only?: RegExp;
-  skip?: RegExp;
+  failFast?: boolean;
+  only?: string | RegExp;
+  skip?: string | RegExp;
   disableLog?: boolean;
 }
 
+function filterTests(
+  tests: TestDefinition[],
+  only: undefined | string | RegExp,
+  skip: undefined | string | RegExp
+): TestDefinition[] {
+  return tests.filter((def: TestDefinition): boolean => {
+    let passes = true;
+
+    if (only) {
+      if (only instanceof RegExp) {
+        passes = passes && only.test(def.name);
+      } else {
+        passes = passes && def.name.includes(only);
+      }
+    }
+
+    if (skip) {
+      if (skip instanceof RegExp) {
+        passes = passes && !skip.test(def.name);
+      } else {
+        passes = passes && !def.name.includes(skip);
+      }
+    }
+
+    return passes;
+  });
+}
+
 export async function runTests({
-  exitOnFail = false,
-  only = /[^\s]/,
-  skip = /^\s*$/,
+  exitOnFail = true,
+  failFast = false,
+  only = undefined,
+  skip = undefined,
   disableLog = false
 }: RunTestsOptions = {}): Promise<void> {
-  const testsToRun = TEST_REGISTRY.filter(
-    ({ name }): boolean => only.test(name) && !skip.test(name)
-  );
+  const testsToRun = filterTests(TEST_REGISTRY, only, skip);
 
   const stats: TestStats = {
     measured: 0,
@@ -149,16 +159,17 @@ export async function runTests({
   const RED_BG_FAIL = bgRed(" FAIL ");
 
   originalConsole.log(`running ${testsToRun.length} tests`);
-  const suiteStart = performance.now();
+  const suiteStart = +new Date();
 
   for (const testCase of testCases) {
     try {
-      const start = performance.now();
+      const start = +new Date();
       await testCase.fn();
-      const end = performance.now();
-      testCase.timeElapsed = end - start;
+      testCase.timeElapsed = +new Date() - start;
       originalConsole.log(
-        `${GREEN_OK}     ${testCase.name} ${promptTestTime(end - start, true)}`
+        `${GREEN_OK}     ${testCase.name} ${formatDuration(
+          testCase.timeElapsed
+        )}`
       );
       stats.passed++;
     } catch (err) {
@@ -166,13 +177,13 @@ export async function runTests({
       originalConsole.log(`${RED_FAILED} ${testCase.name}`);
       originalConsole.log(err.stack);
       stats.failed++;
-      if (exitOnFail) {
+      if (failFast) {
         break;
       }
     }
   }
 
-  const suiteEnd = performance.now();
+  const suiteDuration = +new Date() - suiteStart;
 
   if (disableLog) {
     // @ts-ignore
@@ -185,23 +196,26 @@ export async function runTests({
       `${stats.passed} passed; ${stats.failed} failed; ` +
       `${stats.ignored} ignored; ${stats.measured} measured; ` +
       `${stats.filtered} filtered out ` +
-      `${promptTestTime(suiteEnd - suiteStart)}\n`
+      `${formatDuration(suiteDuration)}\n`
   );
 
-  // TODO(bartlomieju): what's it for? Do we really need, maybe add handler for unhandled
-  // promise to avoid such shenanigans
-  if (stats.failed) {
-    // Use setTimeout to avoid the error being ignored due to unhandled
-    // promise rejections being swallowed.
-    setTimeout((): void => {
-      originalConsole.error(`There were ${stats.failed} test failures.`);
-      testCases
-        .filter(testCase => !!testCase.error)
-        .forEach(testCase => {
-          originalConsole.error(`${RED_BG_FAIL} ${red(testCase.name)}`);
-          originalConsole.error(testCase.error);
-        });
+  // TODO(bartlomieju): is `defer` really needed? Shouldn't unhandled
+  // promise rejection be handled per test case?
+  // Use defer to avoid the error being ignored due to unhandled
+  // promise rejections being swallowed.
+  await defer(0);
+
+  if (stats.failed > 0) {
+    originalConsole.error(`There were ${stats.failed} test failures.`);
+    testCases
+      .filter(testCase => !!testCase.error)
+      .forEach(testCase => {
+        originalConsole.error(`${RED_BG_FAIL} ${red(testCase.name)}`);
+        originalConsole.error(testCase.error);
+      });
+
+    if (exitOnFail) {
       exit(1);
-    }, 0);
+    }
   }
 }