1237961 - Assert.throws raise a TypeError exception when the expected param is an arrow function

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

If the "expected" param of Assert.throws is an arrow function, a TypeError exception is raised on:

- https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/testing/modules/Assert.jsm#317

when it tries to check if "actual" is an instaceof "expected".

Follows an example stacktrace from Bug 1237357 Comment 5: 

ERROR Unexpected exception TypeError: 'prototype' property of expected is not an object at resource://testing-common/Assert.jsm:317
expectedException@resource://testing-common/Assert.jsm:317:1
proto.throws@resource://testing-common/Assert.jsm:358:31
testInvalidUUID@/zfs-tardis/mozlab/desktop/fx-team/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/toolkit/components/extensions/test/xpcshell/test_locale_converter.js:116:3
_run_next_test@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:1483:9
do_execute_soon/<.run@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:675:9
_do_main@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:208:5
_execute_test@/zfs-tardis/mozlab/desktop/fx-team/testing/xpcshell/head.js:535:5
@-e:1:1

Comment 1

[:Paolo Amadini]

Attachment: Bug 1237961 - Assert.throws raises a TypeError exception when the "expected" parameter is an arrow function.

Review commit: https://reviewboard.mozilla.org/r/106066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106066/

Comment 2

[:Paolo Amadini]

Just hit the same bug. I'm trying to fix it with the simplest approach I can think of.

Let's see if it breaks anything:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93e3323b0a3c72e6e21657fb0b76ff2f48c6aa1b

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8828782 [details]
Bug 1237961 - Assert.throws raises a TypeError exception when the "expected" parameter is an arrow function.

https://reviewboard.mozilla.org/r/106066/#review107026

Thanks for picking this up, Paolo! I didn't know we had this bug :/

Can you make the following changes? Should be an r+ at that point...

::: testing/modules/Assert.jsm:317
(Diff revision 1)
>      return false;
>    }
>  
>    if (instanceOf(expected, "RegExp")) {
>      return expected.test(actual);
> -  } else if (actual instanceof expected) {
> +  } else if (!(typeof expected === "function" && !expected.prototype) &&

1. Can you add a comment that this guard is meant to pass-through arrow-functions explicitly?
2. http://stackoverflow.com/a/28234333 Kangax recommends using `expected.toString()` (as an exeption to the rule) to prevent false positives with built-ins:

```js
function isArrowFunction(f) {
  return typeof f == "function" && !f.hasOwnProperty("prototype") && /^\([^)]*\)\s*=>/.test(f.toString());
}
```

Comment 4

[:Paolo Amadini]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> 2. http://stackoverflow.com/a/28234333 Kangax recommends using
> `expected.toString()` (as an exeption to the rule) to prevent false
> positives with built-ins:

Although Assert.throws is used sparingly, and generally with short functions, I don't think we need the performance hit and general "hackiness" of the "toString" call here.

We don't really care about what happens if somebody passes a built-in function like "Math.min" to "Assert.throws", if the choice is between throwing the TypeError from comment 0 or failing the test because passing "Math.min" doesn't quite make sense :-)

Proper type checking of arguments to Assert.jsm functions is a different matter, we should definitely have more informative messages in case of misuse, but I tend towards considering this out of scope for this bug.

Comment 5

[:Paolo Amadini]

Comment on attachment 8828782 [details]
Bug 1237961 - Assert.throws raises a TypeError exception when the "expected" parameter is an arrow function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106066/diff/1-2/

Comment 6

[:Paolo Amadini]

(Also, I don't think we should have an explicit test case for the built-in function "misuse".)

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8828782 [details]
Bug 1237961 - Assert.throws raises a TypeError exception when the "expected" parameter is an arrow function.

https://reviewboard.mozilla.org/r/106066/#review107042

Comment 8

[:Paolo Amadini]

Thanks! Try looks good, will wait some more time to check everything is green before landing.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96d7d786f56b
Assert.throws raises a TypeError exception when the "expected" parameter is an arrow function. r=mikedeboer

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/96d7d786f56b