Bug 1452706 - Make the 'expected' argument to Assert.throws/rejects required rather than optional. r?mikedeboer draft
authorMark Banner <standard8@mozilla.com>
Fri, 01 Jun 2018 14:33:31 +0100
changeset 814485 59e979897e9cc2aa262a9112aac7debf91596fac
parent 814484 535598b33b49bb5fef37fb072d93fcd2f23e7b0a
child 814486 e811bc432f7bf51c6f990593e1ccbd67b1ffb725
push id115229
push userbmo:standard8@mozilla.com
push dateThu, 05 Jul 2018 13:40:07 +0000
reviewersmikedeboer
bugs1452706
milestone63.0a1
Bug 1452706 - Make the 'expected' argument to Assert.throws/rejects required rather than optional. r?mikedeboer MozReview-Commit-ID: 25flGw5sWga
testing/modules/Assert.jsm
testing/modules/tests/xpcshell/test_assert.js
--- a/testing/modules/Assert.jsm
+++ b/testing/modules/Assert.jsm
@@ -322,16 +322,29 @@ proto.strictEqual = function strictEqual
  *        (mixed) Test reference to evaluate against `actual`
  * @param message (optional)
  *        (string) Short explanation of the expected result
  */
 proto.notStrictEqual = function notStrictEqual(actual, expected, message) {
   this.report(actual === expected, actual, expected, message, "!==");
 };
 
+function checkExpectedArgument(instance, funcName, expected) {
+  if (!expected) {
+    instance.ok(false, `Error: The 'expected' argument was not supplied to Assert.${funcName}()`);
+  }
+
+  if (!instanceOf(expected, "RegExp") &&
+      typeof expected !== "function" &&
+      typeof expected !== "object") {
+    instance.ok(false,
+      `Error: The 'expected' argument to Assert.${funcName}() must be a RegExp, function or an object`);
+  }
+}
+
 function expectedException(actual, expected) {
   if (!actual || !expected) {
     return false;
   }
 
   if (instanceOf(expected, "RegExp")) {
     return expected.test(actual);
   // We need to guard against the right hand parameter of "instanceof" lacking
@@ -351,52 +364,46 @@ function expectedException(actual, expec
  * assert.throws(block, Error_opt, message_opt);
  *
  * Example:
  * ```js
  * // The following will verify that an error of type TypeError was thrown:
  * Assert.throws(() => testBody(), TypeError);
  * // The following will verify that an error was thrown with an error message matching "hello":
  * Assert.throws(() => testBody(), /hello/);
- * // The following will verify that any error was thrown and will use "hello" in the test report:
- * Assert.throws(() => testBody(), "hello");
  * ```
  *
  * @param block
  *        (function) Function block to evaluate and catch eventual thrown errors
  * @param expected (optional)
- *        (mixed) This parameter can be either a RegExp, a function, or a string. The
+ *        (mixed) This parameter can be either a RegExp or a function. The
  *        function is either the error type's constructor, or it's a method that returns a boolean
- *        that describes the test outcome. When string value is provided, it will be used as if it
- *        was provided as the message parameter.
+ *        that describes the test outcome.
  * @param message (optional)
  *        (string) Short explanation of the expected result
  */
 proto.throws = function(block, expected, message) {
-  let actual;
+  checkExpectedArgument(this, "throws", expected);
 
-  if (typeof expected === "string") {
-    message = expected;
-    expected = null;
-  }
+  let actual;
 
   try {
     block();
   } catch (e) {
     actual = e;
   }
 
-  message = (expected && expected.name ? " (" + expected.name + ")." : ".") +
+  message = (expected.name ? " (" + expected.name + ")." : ".") +
             (message ? " " + message : ".");
 
   if (!actual) {
     this.report(true, actual, expected, "Missing expected exception" + message);
   }
 
-  if ((actual && expected && !expectedException(actual, expected))) {
+  if ((actual && !expectedException(actual, expected))) {
     throw actual;
   }
 
   this.report(false, expected, expected, message);
 };
 
 /**
  * A promise that is expected to reject:
@@ -405,25 +412,22 @@ proto.throws = function(block, expected,
  * @param promise
  *        (promise) A promise that is expected to reject
  * @param expected (optional)
  *        (mixed) Test reference to evaluate against the rejection result
  * @param message (optional)
  *        (string) Short explanation of the expected result
  */
 proto.rejects = function(promise, expected, message) {
+  checkExpectedArgument(this, "rejects", expected);
   return new Promise((resolve, reject) => {
-    if (typeof expected === "string") {
-      message = expected;
-      expected = null;
-    }
     return promise.then(
       () => this.report(true, null, expected, "Missing expected exception " + message),
       err => {
-        if (expected && !expectedException(err, expected)) {
+        if (!expectedException(err, expected)) {
           reject(err);
           return;
         }
         this.report(false, err, expected, message);
         resolve();
       }
     ).catch(reject);
   });
--- a/testing/modules/tests/xpcshell/test_assert.js
+++ b/testing/modules/tests/xpcshell/test_assert.js
@@ -76,25 +76,25 @@ function run_test() {
                 "deepEqual date");
 
   // 7.3
   assert.deepEqual(/a/, /a/);
   assert.deepEqual(/a/g, /a/g);
   assert.deepEqual(/a/i, /a/i);
   assert.deepEqual(/a/m, /a/m);
   assert.deepEqual(/a/igm, /a/igm);
-  assert.throws(makeBlock(assert.deepEqual, /ab/, /a/));
-  assert.throws(makeBlock(assert.deepEqual, /a/g, /a/));
-  assert.throws(makeBlock(assert.deepEqual, /a/i, /a/));
-  assert.throws(makeBlock(assert.deepEqual, /a/m, /a/));
-  assert.throws(makeBlock(assert.deepEqual, /a/igm, /a/im));
+  assert.throws(makeBlock(assert.deepEqual, /ab/, /a/), ns.Assert.AssertionError);
+  assert.throws(makeBlock(assert.deepEqual, /a/g, /a/), ns.Assert.AssertionError);
+  assert.throws(makeBlock(assert.deepEqual, /a/i, /a/), ns.Assert.AssertionError);
+  assert.throws(makeBlock(assert.deepEqual, /a/m, /a/), ns.Assert.AssertionError);
+  assert.throws(makeBlock(assert.deepEqual, /a/igm, /a/im), ns.Assert.AssertionError);
 
   let re1 = /a/;
   re1.lastIndex = 3;
-  assert.throws(makeBlock(assert.deepEqual, re1, /a/));
+  assert.throws(makeBlock(assert.deepEqual, re1, /a/), ns.Assert.AssertionError);
 
   // 7.4
   assert.deepEqual(4, "4", "deepEqual == check");
   assert.deepEqual(true, 1, "deepEqual == check");
   assert.throws(makeBlock(assert.deepEqual, 4, "5"),
                 ns.Assert.AssertionError,
                 "deepEqual == check");
 
@@ -153,20 +153,20 @@ function run_test() {
   }
   makeBlock(thrower, ns.Assert.AssertionError);
   makeBlock(thrower, ns.Assert.AssertionError);
 
   // the basic calls work
   assert.throws(makeBlock(thrower, ns.Assert.AssertionError),
                 ns.Assert.AssertionError, "message");
   assert.throws(makeBlock(thrower, ns.Assert.AssertionError), ns.Assert.AssertionError);
-  assert.throws(makeBlock(thrower, ns.Assert.AssertionError));
+  assert.throws(makeBlock(thrower, ns.Assert.AssertionError), ns.Assert.AssertionError);
 
   // if not passing an error, catch all.
-  assert.throws(makeBlock(thrower, TypeError));
+  assert.throws(makeBlock(thrower, TypeError), TypeError);
 
   // when passing a type, only catch errors of the appropriate type
   let threw = false;
   try {
     assert.throws(makeBlock(thrower, TypeError), ns.Assert.AssertionError);
   } catch (e) {
     threw = true;
     assert.ok(e instanceof TypeError, "type");
@@ -178,17 +178,17 @@ function run_test() {
 
   function ifError(err) {
     if (err) {
       throw err;
     }
   }
   assert.throws(function() {
     ifError(new Error("test error"));
-  });
+  }, /test error/);
 
   // make sure that validating using constructor really works
   threw = false;
   try {
     assert.throws(
       function() {
         throw ({});
       },
@@ -248,17 +248,17 @@ function run_test() {
 
   // https://github.com/joyent/node/issues/2893
   try {
     assert.throws(function() {
       ifError(null);
     });
   } catch (e) {
     threw = true;
-    assert.equal(e.message, "Missing expected exception..");
+    assert.equal(e.message, "Error: The 'expected' argument was not supplied to Assert.throws() - false == true");
   }
   assert.ok(threw);
 
   // https://github.com/joyent/node/issues/5292
   try {
     assert.equal(1, 2);
   } catch (e) {
     assert.equal(e.toString().split("\n")[0], "AssertionError: 1 == 2");
@@ -350,19 +350,16 @@ add_task(async function test_rejects() {
       deepEqual(ex, err, "Assert.rejects threw the original unexpected error");
     }
   }
 
   // A "throwable" error that's not an actual Error().
   let SomeErrorLikeThing = function() {};
 
   // The actual tests...
-  // No "expected" or "message" values supplied.
-  await assert.rejects(Promise.reject(new Error("oh no")));
-  await assert.rejects(Promise.reject("oh no"));
 
   // An explicit error object:
   // An instance to check against.
   await assert.rejects(Promise.reject(new Error("oh no")), Error, "rejected");
   // A regex to match against the message.
   await assert.rejects(Promise.reject(new Error("oh no")), /oh no/, "rejected");
 
   // Failure cases: