Peter Major Assignee
	Description • 7 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

The Assert.throws method definition in Assert.jsm at the moment allows string type for the expected parameter, however the actual implementation doesn't really do anything with that parameter in that case. The end result is that statements like these:

```javascript
    Assert.throws(() => cookie.fromJSON(invalidType), "Expected cookie object");
```

Will be executed and result in passed tests without ever actually verifying the exception message.


Actual results:

The tests pass, because the exception messages aren't verified.


Expected results:

The tests where the exception message did not match the second parameter, should have failed.

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 7 years ago

(In reply to Peter Major from comment #0)

> Will be executed and result in passed tests without ever actually
> verifying the exception message.

It can be argued that the last argument should be treated as an
informative message to be displayed when the assertion fails.  This
is the case for all the other assertion functions.

I’m more worried about the case where the following doesn’t
compare the second argument to the thrown error:

> Assert.throws(() => throw new TypeError("foo"), TypeError);

The only way to currently assert that the right error is thrown is
through using a regular expression, because RegExp is special cased
in Assert.jsm:

> Assert.throws(() => throw new TypeError("foo"), /TypeError/);

As far as I can understand a lot of tests in mozilla-central use
Assert.throws under the assumption that the first code example will
check that TypeError is thrown, whereas in reality it is ignored and
only the second does what you expect.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 7 years ago

The original implementation of the whole module was done on bug 873126. So maybe Mike has some valuable feedback for us given that he wrote/imported this code. Mike, I know it's a while ago... Also cc'ing some folks who reviewed the patches.

	Mike de Boer [:mikedeboer]
	Comment 5 • 7 years ago

I agree things are a little bit confusing here and I'd propose the fix to be better argument validation:

 - When `message` is not provided and `expected` is a string, assume that `expected` is the `message` and `expected` is `null`.
   So the change here is that we'll explicitly check if `message` is _not_ provided.
 - `expected` may only be
   1. `null`,
   2. A RegExp,
   3. A constructor function,
   4. A regular (arrow) function that does custom evaluation, returning a `true` for pass.
 - For any other value of `expected`, throw a SyntaxError.

How does that sound?

	Peter Major Assignee
	Comment 6 • 7 years ago

> When `message` is not provided and `expected` is a string, assume that `expected` is the `message` and `expected` is `null`.
>   So the change here is that we'll explicitly check if `message` is _not_ provided.

If `expected` is null, then what exactly is being asserted? Would that mean that any Error thrown from the `block` would qualify as a passing test, and if there was no Error thrown it would fail?

I was trying to go down the path of least astonishment: given that both Mozilla's and nodejs's assert implementation was based on narwhaljs, I thought it would be better to bring Mozilla's implementation of assert in line of the current nodejs assert implementation (at least from the throws method's perspective):
https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message (short version: they throw an error if `expected` value is of type string)

I'm happy to go down any route you deem appropriate though.

	Mike de Boer [:mikedeboer]
	Comment 7 • 7 years ago

(In reply to Peter Major from comment #6)
> If `expected` is null, then what exactly is being asserted? Would that mean
> that any Error thrown from the `block` would qualify as a passing test, and
> if there was no Error thrown it would fail?

Yes, it would indeed fail if no error (_any_ error, in this case) would be thrown. Thus, pass if an error (_any_ error, in this case) is thrown.

> https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message
> (short version: they throw an error if `expected` value is of type string)

From the document you referenced I quote: "If a string is provided as the second argument, then error is assumed to be omitted and the string will be used for message instead."
This describes the exact behavior we have now and implicitly states that the NodeJS assert module does _not_ throw when the second argument - `error` - is a string.

> I'm happy to go down any route you deem appropriate though.

Heh, well, let's just go for the most obvious, breaks the least type of solution. I also think that it would be nice to not confuse devs who worked with NodeJS before with different semantics for `throws` when possible.

	Peter Major Assignee
	Comment 8 • 7 years ago

> This describes the exact behavior we have now and implicitly states that the NodeJS assert module does _not_ throw when the second argument - `error` - is a string.

Sigh, looks like I've misread the NodeJS source code.. Testing this locally now I can see that the NodeJS behavior is exactly what you've been describing: when the `expected` parameter is a string, then it is only used for reporting and it just verifies that an error was thrown.

	Peter Major Assignee
	Comment 9 • 7 years ago

Given the provided expectations, I've been trying to reproduce the originally reported issue, but looks like my own reproduction steps weren't as solid as I might have liked. It appears that I've got confused and made the assumption that the lack of error messages when the string expected parameter was provided meant that the assertion wasn't working correctly.

Now I know this is not true, it was actually verifying that an error was thrown and it fails as expected when an error is not thrown (and uses the provided string parameter in the report as expected).

I'm turning this issue into improving the documentation for the throws method.

Apologies for taking your time..

	Mike de Boer [:mikedeboer]
	Comment 11 • 7 years ago

Please don't worry about it ;-)

Unclear/ bad documentation are bugs too!

	Mike de Boer [:mikedeboer]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8919075 [details]
Bug 1409195 - Improve Assert.throws documentation

https://reviewboard.mozilla.org/r/189992/#review196186

r=me with nits fixed! Thanks :)

Andreas, I hope you don't mind me stealing your review here...

::: testing/modules/Assert.jsm:335
(Diff revision 3)
>   * 11. Expected to throw an error:
>   * assert.throws(block, Error_opt, message_opt);
>   *
> + * Example:
> + * ```js
> + * // The following will verify that an error of type TypeError was thrown

nit: Can you add a colon preceding your example?

'[...] of type TypeError was thrown:'

::: testing/modules/Assert.jsm:337
(Diff revision 3)
>   *
> + * 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"

nit: Please fix the documentation of the four lines below.

	Mike de Boer [:mikedeboer]
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8919075 [details]
Bug 1409195 - Improve Assert.throws documentation

https://reviewboard.mozilla.org/r/189992/#review196186

> nit: Please fix the documentation of the four lines below.

s/documentation/indentation/

	Peter Major Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8919075 [details]
Bug 1409195 - Improve Assert.throws documentation

https://reviewboard.mozilla.org/r/189992/#review196186

> nit: Can you add a colon preceding your example?
> 
> '[...] of type TypeError was thrown:'

Sure thing.

> s/documentation/indentation/

Do you mean that the lines should be shorter than 78 characters?

I've tried running `./mach lint -l eslint testing/modules/Assert.jsm --no-ignore`, but couldn't see any issue reported.

	Mike de Boer [:mikedeboer]
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8919075 [details]
Bug 1409195 - Improve Assert.throws documentation

https://reviewboard.mozilla.org/r/189992/#review196186

> Do you mean that the lines should be shorter than 78 characters?
> 
> I've tried running `./mach lint -l eslint testing/modules/Assert.jsm --no-ignore`, but couldn't see any issue reported.

No, we're not as strict wrt line length... What I meant was that you need to precede the `*` with a single space, just like the lines above and below: ` * Comment goes here.`

	Andreas Tolfsen ❲:ato❳
	Comment 17 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #3)

> I’m more worried about the case where the following doesn’t
> compare the second argument to the thrown error:
>
> > Assert.throws(() => throw new TypeError("foo"), TypeError);

Disregard this part of my earlier comment, it turned out to be
completely false.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8919075 [details]
Bug 1409195 - Improve Assert.throws documentation

https://reviewboard.mozilla.org/r/189992/#review196316

	Pulsebot
	Comment 19 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d28baf93c4a
Improve Assert.throws documentation. r=mikedeboer,ato

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 20 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2d28baf93c4a