Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Description • 7 years ago

In nearly all of our Marionette xpcshell tests which use `Assert.throws` we inappropriately pass in the error class directly, but not via a regular expression. As result strange test failures will pop-up in case of such an assertion fails.

What has to be done is the following:

1) Check all the files [1] for a usage of `Assert.throws`.
2) If the error class as second argument gets passed in directly fix it.

Here an example:

This is the wrong usage:
> Assert.throws(() => fromJSON(typ), InvalidArgumentError);

Needs to be corrected to:
> Assert.throws(() => fromJSON(typ), /InvalidArgumentError/);

[1] https://dxr.mozilla.org/mozilla-central/search?q=path%3Atesting%2Fmarionette%20file%3Atest_*.js&=mozilla-central

	Peter Major
	Comment 1 • 7 years ago

I may be reading the wrong documentation:
https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message

but for me it feels like that the current usage is exactly like the "Validate instanceof using constructor" example.

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

Yeah, node.js is not related here. I digged around on mozilla-central and found the following example:

https://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_rusturl.js#211-230

Not sure where exactly where the `Assert.throws()` method is defined. Maybe Andreas can point us to this location, because he mentioned that to me. Anyway, as it looks like the `throws()` method simply checks the error via the class name and error message.

	Peter Major
	Comment 3 • 7 years ago

Actually, the example you have pointed to has the same incorrect usage of the Assert.throws library as what I've fixed in Marionette: Bug 1405240

Looking at testing/modules/Assert.jsm it looks like there isn't much of an assertion on messages, instead the throws method always appears to expect a non-string expected object and only then does it assert anything. Feels like Assert.jsm behavior could be improved, but that would probably result in shedloads of test failures due to similar misuses of the throws method.

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

Hm, I see. But we should still be fine in checking against the exception class, and won't have to pass in the error message as RegExp?

	Peter Major
	Comment 5 • 7 years ago

I've investigated this a bit further and came to the conclusion that both Assert.jsm and the nodejs assert module are derived works of the now defunct narwhaljs project. Looking at node's implementation, it looks like it has been updated in a few regards, and for example they fail early when the second parameter provided is of string type. That might be something that we'd like to port over (tracked as a separate bug/rfe).

	Peter Major
	Comment 6 • 7 years ago

To answer your original question: You can either pass in a function, or a regular expression to the throws method. In case you pass in something like InvalidCookieDomainError, then that seems to mean that you are passing in a function (the constructor I guess), hence Assert.jsm will try to verify that the prototype of the actual exception matches with the expected type, which seems like the right behavior to me.

Can you provide an example that demonstrates the issue you are having? What sort of strange test failures are you seeing, and do you have a reliable way to trigger it?

	Peter Major
	Comment 7 • 7 years ago

I've been trying to look into Bug 1409195 today and as part of that process I think I bumped into this issue.
An easy way to reproduce this issue for me was to change test_insert.js around line 75:

>   Assert.throws(
>    () => PlacesUtils.history.insert({
>      url: TEST_URL,
>      visits: [
>        {
>          transition: TRANSITION_LINK,
>          date: futureDate,
>        }
>      ]}),
>    TypeError,
>    "passing a visit object with a future date to History.insert should throw a TypeError"

Then run the test using:

> ./mach test toolkit/components/places/tests/history/test_insert.js

Something magical is happening in that code, because the following code returns false inside PlacesUtils.jsm:

> new TypeError("foo") instanceof TypeError

I'm not really sure why or how could this happen, so I'd love to hear any suggestions you may have. Alternatively I may need to return this issue to the unassigned pool.

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

(In reply to Peter Major from comment #6)
> Can you provide an example that demonstrates the issue you are having? What
> sort of strange test failures are you seeing, and do you have a reliable way
> to trigger it?

A simple example would be a syntax error in eg. session.js. Just remove the `let` for a variable declaration and run the test_session.js file. You will see a message similar to what we fixed in bug 1406150.

Otherwise we may want to wait with this bug until any questions are resolved on bug 1409195, and the problem actually fixed. Thanks for filing and starting to work on that other one!

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

Comment on attachment 8933718 [details] [diff] [review]
Use regular expressions in Assert.throws

Review of attachment 8933718 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, that we haven't updated the bug since the documentation changes on bug 1409195 got landed. But we cannot use a regex here for the exception type. When using a regex a message has to be passed in, which will be used to compare with the exception message (see test_cookie.js). For the exception type itself, just leave the type as is (see test_assert.js - specifically the newly landed acyclic tests).

So we would have to remove the regex if only the exception type is interesting. Otherwise we might want to check for a specific exception message via the regex.

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

I _can_ actually use a regexp here for the error type because the
error type is included in the string, e.g. "TypeError: <message>".

But I agree it does not make sense to replace cases where the second
argument is just an error type.  Which leads me to wonder, is this
bug really still actionable?

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

s/I _can_/You _can_/

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

No, I think it's still valid due to the following reason:

1) Some instances of `throws()` do not explicitly check for the type of exception thrown. We should make sure to have an explicit check for that.

2) Other instances like in test_session.js make use of the regex for the exception, which has to be reverted.

3) And others are using the 3rd parameter called `message` for a short explanation. Does anyone know where this text will be printed? When I explicitly make a check fail, the used message is not visible. Peter, do you know?

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 14 • 6 years ago

This gets actually been fixed via bug 1465530 now.