From 52fb259a59ca6348c81fec81ddfaa756a062e9f2 Mon Sep 17 00:00:00 2001
From: Masashi Hirano <cherrydog07@gmail.com>
Date: Sun, 21 Apr 2019 17:04:04 +0900
Subject: [PATCH 1/3] test: fix ineffective error tests

Fix tests whether errors are thrown correctly
because they are successful when error doesn't get thrown.
---
 .../non-node-context/test-make-buffer.js      |  2 +
 test/es-module/test-esm-error-cache.js        |  2 +
 test/parallel/test-assert.js                  | 40 +++++++++++-------
 test/parallel/test-util-inspect.js            |  4 ++
 test/parallel/test-vm-codegen.js              |  7 +++-
 test/sequential/test-module-loading.js        | 42 ++++++++++++-------
 6 files changed, 65 insertions(+), 32 deletions(-)

diff --git a/test/addons/non-node-context/test-make-buffer.js b/test/addons/non-node-context/test-make-buffer.js
index 9b17fa4ee930ae..8b398685f17476 100644
--- a/test/addons/non-node-context/test-make-buffer.js
+++ b/test/addons/non-node-context/test-make-buffer.js
@@ -19,4 +19,6 @@ try {
   assert.strictEqual(exception.code, 'ERR_BUFFER_CONTEXT_NOT_AVAILABLE');
   assert.strictEqual(exception.message,
                      'Buffer is not available for the current Context');
+  return;
 }
+assert.fail('Missing expected exception');
diff --git a/test/es-module/test-esm-error-cache.js b/test/es-module/test-esm-error-cache.js
index 79f76357eca3ea..d43c98608a9454 100644
--- a/test/es-module/test-esm-error-cache.js
+++ b/test/es-module/test-esm-error-cache.js
@@ -22,5 +22,7 @@ let error;
     await import(file);
   } catch (e) {
     assert.strictEqual(error, e);
+    return;
   }
+  assert.fail('Missing expected exception');
 })();
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index 4c2b1f978d081f..0b5af41b9a6552 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -296,23 +296,33 @@ testAssertionMessage({ a: undefined, b: null },
 testAssertionMessage({ a: NaN, b: Infinity, c: -Infinity },
                      '{\n+   a: NaN,\n+   b: Infinity,\n+   c: -Infinity\n+ }');
 
-// https://github.com/nodejs/node-v0.x-archive/issues/5292
-try {
-  assert.strictEqual(1, 2);
-} catch (e) {
-  assert.strictEqual(
-    e.message,
-    `${strictEqualMessageStart}\n1 !== 2\n`
-  );
-  assert.ok(e.generatedMessage, 'Message not marked as generated');
+{
+  let threw = false;
+  // https://github.com/nodejs/node-v0.x-archive/issues/5292
+  try {
+    assert.strictEqual(1, 2);
+  } catch (e) {
+    assert.strictEqual(
+      e.message,
+      `${strictEqualMessageStart}\n1 !== 2\n`
+    );
+    assert.ok(e.generatedMessage, 'Message not marked as generated');
+    threw = true;
+  }
+  assert.ok(threw, 'Missing expected exception');
 }
 
-try {
-  assert.strictEqual(1, 2, 'oh no');
-} catch (e) {
-  assert.strictEqual(e.message, 'oh no');
-  // Message should not be marked as generated.
-  assert.strictEqual(e.generatedMessage, false);
+{
+  let threw = false;
+  try {
+    assert.strictEqual(1, 2, 'oh no');
+  } catch (e) {
+    assert.strictEqual(e.message, 'oh no');
+    // Message should not be marked as generated.
+    assert.strictEqual(e.generatedMessage, false);
+    threw = true;
+  }
+  assert.ok(threw, 'Missing expected exception');
 }
 
 {
diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index c1b6e3f98d4480..f79aa0e543f910 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -538,11 +538,15 @@ assert.strictEqual(util.inspect(-5e-324), '-5e-324');
   ].forEach((err) => {
     assert.strictEqual(util.inspect(err), err.stack);
   });
+  let threw = false;
   try {
     undef(); // eslint-disable-line no-undef
   } catch (e) {
     assert.strictEqual(util.inspect(e), e.stack);
+    threw = true;
   }
+  assert.ok(threw, 'Missing expected exception');
+
   const ex = util.inspect(new Error('FAIL'), true);
   assert(ex.includes('Error: FAIL'));
   assert(ex.includes('[stack]'));
diff --git a/test/parallel/test-vm-codegen.js b/test/parallel/test-vm-codegen.js
index ebafe30c076e01..0e18829b5deb3b 100644
--- a/test/parallel/test-vm-codegen.js
+++ b/test/parallel/test-vm-codegen.js
@@ -14,11 +14,14 @@ function expectsError(fn, type) {
     fn();
     assert.fail('expected fn to error');
   } catch (err) {
-    if (typeof type === 'string')
+    if (typeof type === 'string') {
       assert.strictEqual(err.name, type);
-    else
+    } else {
       assert(err instanceof type);
+    }
+    return;
   }
+  assert.fail('Missing expected exception');
 }
 
 {
diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js
index 5bd84086129cc4..74cd4de8b29c58 100644
--- a/test/sequential/test-module-loading.js
+++ b/test/sequential/test-module-loading.js
@@ -201,26 +201,38 @@ assert.throws(
 
   assert.strictEqual(require(`${loadOrder}file1`).file1, 'file1');
   assert.strictEqual(require(`${loadOrder}file2`).file2, 'file2.js');
-  try {
-    require(`${loadOrder}file3`);
-  } catch (e) {
-    // Not a real .node module, but we know we require'd the right thing.
-    if (common.isOpenBSD) // OpenBSD errors with non-ELF object error
-      assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
-    else
-      assert.ok(/file3\.node/.test(e.message.replace(backslash, '/')));
+  {
+    let threw = false;
+    try {
+      require(`${loadOrder}file3`);
+    } catch (e) {
+      // Not a real .node module, but we know we require'd the right thing.
+      if (common.isOpenBSD) { // OpenBSD errors with non-ELF object error
+        assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
+      } else {
+        assert.ok(/file3\.node/.test(e.message.replace(backslash, '/')));
+      }
+      threw = true;
+    }
+    assert.ok(threw, 'Missing expected exception');
   }
 
   assert.strictEqual(require(`${loadOrder}file4`).file4, 'file4.reg');
   assert.strictEqual(require(`${loadOrder}file5`).file5, 'file5.reg2');
   assert.strictEqual(require(`${loadOrder}file6`).file6, 'file6/index.js');
-  try {
-    require(`${loadOrder}file7`);
-  } catch (e) {
-    if (common.isOpenBSD)
-      assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
-    else
-      assert.ok(/file7\/index\.node/.test(e.message.replace(backslash, '/')));
+  {
+    let threw = false;
+    try {
+      require(`${loadOrder}file7`);
+    } catch (e) {
+      if (common.isOpenBSD) {
+        assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
+      } else {
+        assert.ok(/file7\/index\.node/.test(e.message.replace(backslash, '/')));
+      }
+      threw = true;
+    }
+    assert.ok(threw, 'Missing expected exception');
   }
 
   assert.strictEqual(require(`${loadOrder}file8`).file8, 'file8/index.reg');

From 823bfbe8c3d7c0bb9c43c5ac645e3c417943cf59 Mon Sep 17 00:00:00 2001
From: Masashi Hirano <cherrydog07@gmail.com>
Date: Mon, 22 Apr 2019 00:38:20 +0900
Subject: [PATCH 2/3] test: use assert.throws() or rejects() instead of `try /
 catch`

---
 .../non-node-context/test-make-buffer.js      | 23 ++++++++-------
 test/es-module/test-esm-error-cache.js        | 14 +++++-----
 test/parallel/test-assert.js                  | 28 ++++++++-----------
 test/parallel/test-util-inspect.js            | 15 +++++-----
 test/parallel/test-vm-codegen.js              | 24 ++++++++--------
 test/sequential/test-module-loading.js        | 26 +++++++----------
 6 files changed, 59 insertions(+), 71 deletions(-)

diff --git a/test/addons/non-node-context/test-make-buffer.js b/test/addons/non-node-context/test-make-buffer.js
index 8b398685f17476..d134f63b77283c 100644
--- a/test/addons/non-node-context/test-make-buffer.js
+++ b/test/addons/non-node-context/test-make-buffer.js
@@ -9,16 +9,15 @@ const {
 // Because the `Buffer` function and its protoype property only (currently)
 // exist in a Node.js instance’s main context, trying to create buffers from
 // another context throws an exception.
+assert.throws(
+  () => makeBufferInNewContext(),
+  (exception) => {
+    assert.strictEqual(exception.constructor.name, 'Error');
+    assert(!(exception.constructor instanceof Error));
 
-try {
-  makeBufferInNewContext();
-} catch (exception) {
-  assert.strictEqual(exception.constructor.name, 'Error');
-  assert(!(exception.constructor instanceof Error));
-
-  assert.strictEqual(exception.code, 'ERR_BUFFER_CONTEXT_NOT_AVAILABLE');
-  assert.strictEqual(exception.message,
-                     'Buffer is not available for the current Context');
-  return;
-}
-assert.fail('Missing expected exception');
+    assert.strictEqual(exception.code, 'ERR_BUFFER_CONTEXT_NOT_AVAILABLE');
+    assert.strictEqual(exception.message,
+                       'Buffer is not available for the current Context');
+    return true;
+  }
+);
diff --git a/test/es-module/test-esm-error-cache.js b/test/es-module/test-esm-error-cache.js
index d43c98608a9454..26e0d170ac2e1b 100644
--- a/test/es-module/test-esm-error-cache.js
+++ b/test/es-module/test-esm-error-cache.js
@@ -18,11 +18,11 @@ let error;
 
   assert(error);
 
-  try {
-    await import(file);
-  } catch (e) {
-    assert.strictEqual(error, e);
-    return;
-  }
-  assert.fail('Missing expected exception');
+  await assert.rejects(
+    () => import(file),
+    (e) => {
+      assert.strictEqual(error, e);
+      return true;
+    }
+  );
 })();
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index 0b5af41b9a6552..b202aea720b9ff 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -296,34 +296,28 @@ testAssertionMessage({ a: undefined, b: null },
 testAssertionMessage({ a: NaN, b: Infinity, c: -Infinity },
                      '{\n+   a: NaN,\n+   b: Infinity,\n+   c: -Infinity\n+ }');
 
-{
-  let threw = false;
-  // https://github.com/nodejs/node-v0.x-archive/issues/5292
-  try {
-    assert.strictEqual(1, 2);
-  } catch (e) {
+// https://github.com/nodejs/node-v0.x-archive/issues/5292
+assert.throws(
+  () => assert.strictEqual(1, 2),
+  (e) => {
     assert.strictEqual(
       e.message,
       `${strictEqualMessageStart}\n1 !== 2\n`
     );
     assert.ok(e.generatedMessage, 'Message not marked as generated');
-    threw = true;
+    return true;
   }
-  assert.ok(threw, 'Missing expected exception');
-}
+);
 
-{
-  let threw = false;
-  try {
-    assert.strictEqual(1, 2, 'oh no');
-  } catch (e) {
+assert.throws(
+  () => assert.strictEqual(1, 2, 'oh no'),
+  (e) => {
     assert.strictEqual(e.message, 'oh no');
     // Message should not be marked as generated.
     assert.strictEqual(e.generatedMessage, false);
-    threw = true;
+    return true;
   }
-  assert.ok(threw, 'Missing expected exception');
-}
+);
 
 {
   let threw = false;
diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index f79aa0e543f910..eeda567e0d3e22 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -538,14 +538,13 @@ assert.strictEqual(util.inspect(-5e-324), '-5e-324');
   ].forEach((err) => {
     assert.strictEqual(util.inspect(err), err.stack);
   });
-  let threw = false;
-  try {
-    undef(); // eslint-disable-line no-undef
-  } catch (e) {
-    assert.strictEqual(util.inspect(e), e.stack);
-    threw = true;
-  }
-  assert.ok(threw, 'Missing expected exception');
+  assert.throws(
+    () => undef(), // eslint-disable-line no-undef
+    (e) => {
+      assert.strictEqual(util.inspect(e), e.stack);
+      return true;
+    }
+  );
 
   const ex = util.inspect(new Error('FAIL'), true);
   assert(ex.includes('Error: FAIL'));
diff --git a/test/parallel/test-vm-codegen.js b/test/parallel/test-vm-codegen.js
index 0e18829b5deb3b..5d71b8f2660ace 100644
--- a/test/parallel/test-vm-codegen.js
+++ b/test/parallel/test-vm-codegen.js
@@ -10,18 +10,20 @@ const WASM_BYTES = Buffer.from(
 
 
 function expectsError(fn, type) {
-  try {
-    fn();
-    assert.fail('expected fn to error');
-  } catch (err) {
-    if (typeof type === 'string') {
-      assert.strictEqual(err.name, type);
-    } else {
-      assert(err instanceof type);
+  assert.throws(
+    () => {
+      fn();
+      assert.fail('expected fn to error');
+    },
+    (err) => {
+      if (typeof type === 'string') {
+        assert.strictEqual(err.name, type);
+      } else {
+        assert(err instanceof type);
+      }
+      return true;
     }
-    return;
-  }
-  assert.fail('Missing expected exception');
+  );
 }
 
 {
diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js
index 74cd4de8b29c58..612cde3ab0261f 100644
--- a/test/sequential/test-module-loading.js
+++ b/test/sequential/test-module-loading.js
@@ -201,39 +201,33 @@ assert.throws(
 
   assert.strictEqual(require(`${loadOrder}file1`).file1, 'file1');
   assert.strictEqual(require(`${loadOrder}file2`).file2, 'file2.js');
-  {
-    let threw = false;
-    try {
-      require(`${loadOrder}file3`);
-    } catch (e) {
+  assert.throws(
+    () => require(`${loadOrder}file3`),
+    (e) => {
       // Not a real .node module, but we know we require'd the right thing.
       if (common.isOpenBSD) { // OpenBSD errors with non-ELF object error
         assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
       } else {
         assert.ok(/file3\.node/.test(e.message.replace(backslash, '/')));
       }
-      threw = true;
+      return true;
     }
-    assert.ok(threw, 'Missing expected exception');
-  }
+  );
 
   assert.strictEqual(require(`${loadOrder}file4`).file4, 'file4.reg');
   assert.strictEqual(require(`${loadOrder}file5`).file5, 'file5.reg2');
   assert.strictEqual(require(`${loadOrder}file6`).file6, 'file6/index.js');
-  {
-    let threw = false;
-    try {
-      require(`${loadOrder}file7`);
-    } catch (e) {
+  assert.throws(
+    () => require(`${loadOrder}file7`),
+    (e) => {
       if (common.isOpenBSD) {
         assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
       } else {
         assert.ok(/file7\/index\.node/.test(e.message.replace(backslash, '/')));
       }
-      threw = true;
+      return true;
     }
-    assert.ok(threw, 'Missing expected exception');
-  }
+  );
 
   assert.strictEqual(require(`${loadOrder}file8`).file8, 'file8/index.reg');
   assert.strictEqual(require(`${loadOrder}file9`).file9, 'file9/index.reg2');

From b6848f5f7136ba83a2f82c68bb78a75e380c31bb Mon Sep 17 00:00:00 2001
From: shisama <shisama07@gmail.com>
Date: Tue, 23 Apr 2019 19:50:16 +0900
Subject: [PATCH 3/3] test: simplified tests for error case

---
 test/parallel/test-assert.js     | 18 ++++++-----------
 test/parallel/test-vm-codegen.js | 34 ++++++++++----------------------
 2 files changed, 16 insertions(+), 36 deletions(-)

diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index b202aea720b9ff..62ed50f6a43fa9 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -299,23 +299,17 @@ testAssertionMessage({ a: NaN, b: Infinity, c: -Infinity },
 // https://github.com/nodejs/node-v0.x-archive/issues/5292
 assert.throws(
   () => assert.strictEqual(1, 2),
-  (e) => {
-    assert.strictEqual(
-      e.message,
-      `${strictEqualMessageStart}\n1 !== 2\n`
-    );
-    assert.ok(e.generatedMessage, 'Message not marked as generated');
-    return true;
+  {
+    message: `${strictEqualMessageStart}\n1 !== 2\n`,
+    generatedMessage: true
   }
 );
 
 assert.throws(
   () => assert.strictEqual(1, 2, 'oh no'),
-  (e) => {
-    assert.strictEqual(e.message, 'oh no');
-    // Message should not be marked as generated.
-    assert.strictEqual(e.generatedMessage, false);
-    return true;
+  {
+    message: 'oh no',
+    generatedMessage: false
   }
 );
 
diff --git a/test/parallel/test-vm-codegen.js b/test/parallel/test-vm-codegen.js
index 5d71b8f2660ace..0136e143abd1d2 100644
--- a/test/parallel/test-vm-codegen.js
+++ b/test/parallel/test-vm-codegen.js
@@ -8,24 +8,6 @@ const { createContext, runInContext, runInNewContext } = require('vm');
 const WASM_BYTES = Buffer.from(
   [0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00]);
 
-
-function expectsError(fn, type) {
-  assert.throws(
-    () => {
-      fn();
-      assert.fail('expected fn to error');
-    },
-    (err) => {
-      if (typeof type === 'string') {
-        assert.strictEqual(err.name, type);
-      } else {
-        assert(err instanceof type);
-      }
-      return true;
-    }
-  );
-}
-
 {
   const ctx = createContext({ WASM_BYTES });
   const test = 'eval(""); new WebAssembly.Module(WASM_BYTES);';
@@ -44,7 +26,7 @@ function expectsError(fn, type) {
   });
 
   const EvalError = runInContext('EvalError', ctx);
-  expectsError(() => {
+  assert.throws(() => {
     runInContext('eval("x")', ctx);
   }, EvalError);
 }
@@ -57,26 +39,30 @@ function expectsError(fn, type) {
   });
 
   const CompileError = runInContext('WebAssembly.CompileError', ctx);
-  expectsError(() => {
+  assert.throws(() => {
     runInContext('new WebAssembly.Module(WASM_BYTES)', ctx);
   }, CompileError);
 }
 
-expectsError(() => {
+assert.throws(() => {
   runInNewContext('eval("x")', {}, {
     contextCodeGeneration: {
       strings: false,
     },
   });
-}, 'EvalError');
+}, {
+  name: 'EvalError'
+});
 
-expectsError(() => {
+assert.throws(() => {
   runInNewContext('new WebAssembly.Module(WASM_BYTES)', { WASM_BYTES }, {
     contextCodeGeneration: {
       wasm: false,
     },
   });
-}, 'CompileError');
+}, {
+  name: 'CompileError'
+});
 
 common.expectsError(() => {
   createContext({}, {