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

RESOLVED FIXED in Firefox 53

Status

Testing
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: rpl, Assigned: Paolo)

Tracking

Version 3
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
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
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED

Comment 3

a year ago
mozreview-review
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());
}
```
Attachment #8828782 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 4

a year ago
(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 hidden (mozreview-request)
(Assignee)

Comment 6

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

Comment 7

a year ago
mozreview-review
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
Attachment #8828782 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 8

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

Comment 9

a year ago
Pushed by paolo.mozmail@amadzone.org:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96d7d786f56b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.