Closed Bug 1426219 Opened 6 years ago Closed 4 years ago

Shared support for failing tests on TypeError, SyntaxError, ReferenceError

Categories

(Testing :: General, enhancement, P3)

Version 3
enhancement

Tracking

(firefox84 fixed)

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

      No description provided.
Comment on attachment 8937826 [details]
Bug 1426219 - Assert.throws now cleans up recentJSDevError;

https://reviewboard.mozilla.org/r/208526/#review214506

Did you mean to also request review on this part?

::: testing/modules/Assert.jsm:371
(Diff revision 1)
> +  // that we probably need to clean it up.
> +  let cleanupRecentJSDevError = false;
> +  if ("recentJSDevError" in ChromeUtils) {
> +    // Check that we're in a build of Firefox that supports
> +    // the `recentJSDevError` mechanism (i.e. Nightly build).
> +    if (ChromeUtils.recentJSDevError == undefined) {

Was this meant to be an === check?

::: testing/modules/Assert.jsm:407
(Diff revision 1)
>    this.report(false, expected, expected, message);
> +
> +  // Make sure that we don't cause failures for JS Dev Errors that
> +  // were expected, typically for tests that attempt to check
> +  // that we react properly to TypeError, ReferenceError, SyntaxError.
> +  if (cleanupRecentJSDevError && "clearRecentJSDevError" in ChromeUtils) {

I would put braces around the 'in' check here... but I actually I think we can just remove it completely as cleanupRecentJSDevError will never be true in a build that doesn't support this.
Comment on attachment 8937825 [details]
Bug 1426219 - Extend PromiseTestUtils to uncaught JavaScript Developer Errors;

https://reviewboard.mozilla.org/r/208524/#review214508

::: toolkit/modules/tests/modules/PromiseTestUtils.jsm:107
(Diff revision 1)
> +      return;
> +    }
> +    let recentJSDevError = ChromeUtils.recentJSDevError;
> +    if (recentJSDevError) {
> +      ChromeUtils.clearRecentJSDevError();
> +      let message = recentJSDevError.message + "\n" + recentJSDevError.stack + "\n----\n Detected at\n";

nit: this may look nicer with a template string.

I would be curious to see what the test log looks like. Should we add ':' after 'Detected at'? Is the the abuse of promise rejections not causing confusing output?
Attachment #8937825 - Flags: review?(florian) → review+
Assignee: nobody → dteller
Comment on attachment 8937825 [details]
Bug 1426219 - Extend PromiseTestUtils to uncaught JavaScript Developer Errors;

https://reviewboard.mozilla.org/r/208524/#review214508

> nit: this may look nicer with a template string.
> 
> I would be curious to see what the test log looks like. Should we add ':' after 'Detected at'? Is the the abuse of promise rejections not causing confusing output?

I'll admit that it's a bit confusing. Sigh. I probably should create a new module for that purpose.
Priority: -- → P3
Assignee: dteller → nobody

This patch uses (and somewhat abuses) the existing PromiseTestUtils
mechanism to also be able to whitelist uncaught errors that are not
actual Promise rejections.

This uses ChromeUtils.recentJSDevError, which lets us find out
whether there is a recent ReferenceError/SyntaxError/TypeError in
chrome code, even if that error was caught.

MozReview-Commit-ID: 5z1pffURNYm

Assignee: nobody → dteller
Status: NEW → ASSIGNED

We are throwing TypeError in many cases that are not type errors. Since we have no manner of distinguishing which of these cases are actual (developer) errors and which are runtime exceptions, this patch removes TypeError from the list of developer errors.

Depends on D94972

Attachment #9184740 - Attachment description: Bug 1426219 - TypeError is not a developer error anymore → Bug 1426219 - TypeError is not a developer error anymore;r?yulia
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38a6d65eb220
TypeError is not a developer error anymore;r=yulia
https://hg.mozilla.org/integration/autoland/rev/8fba1860ba2b
Extend PromiseTestUtils to uncaught JavaScript Developer Errors;r=florian
https://hg.mozilla.org/integration/autoland/rev/371e4b2a0fef
Assert.throws now cleans up recentJSDevError;r=ahal
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: