browser.test.* stacks are no longer useful

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
WebExtensions: General
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

51 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

They report the stack in the parent process, from the point where the message was received, rather than from where the assertion function was called.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8809685 [details]
Bug 1316780: Part 1 - Proxy extension events between the parent and child process.

https://reviewboard.mozilla.org/r/92220/#review92416
Attachment #8809685 - Flags: review?(aswan) → review+

Comment 8

a year ago
mozreview-review
Comment on attachment 8809686 [details]
Bug 1316780: Part 2 - Remove ext-test.js.

https://reviewboard.mozilla.org/r/92222/#review92418

::: toolkit/components/extensions/ext-c-test.js:87
(Diff revision 2)
> +    extension.emit("test-result", Boolean(value), String(msg));
>    }
>  
>    return {
>      test: {
> -      // These functions accept arbitrary values. Convert the parameters to
> +      sendMessage: function(...args) {

nit: why this syntax instead of just `sendMessage(...args) {`
Attachment #8809686 - Flags: review?(aswan) → review+
(Assignee)

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8809686 [details]
Bug 1316780: Part 2 - Remove ext-test.js.

https://reviewboard.mozilla.org/r/92222/#review92418

> nit: why this syntax instead of just `sendMessage(...args) {`

Just because I moved this code from ext-test.js and didn't change it. I was thinking about it, though, so I guess I will now.

Comment 10

a year ago
mozreview-review
Comment on attachment 8809687 [details]
Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions.

https://reviewboard.mozilla.org/r/92224/#review92420

Looks good to me and the SimpleTest change seems like a good one, but should probably be approved by an owner or peer for that code.

::: testing/mochitest/tests/SimpleTest/ExtensionTestUtils.js:57
(Diff revision 2)
>    }
>  
>    function testHandler(kind, pass, msg, ...args) {
>      if (kind == "test-eq") {
>        var [expected, actual] = args;
> -      SimpleTest.ok(pass, `${msg} - Expected: ${expected}, Actual: ${actual}`);
> +      SimpleTest.ok(pass, `${msg} - Expected: ${expected}, Actual: ${actual}`, undefined, args[2]);

can you just pull this out of args on the line above (and maybe switch var -> let at the same time)

::: toolkit/components/extensions/ext-c-test.js:104
(Diff revision 2)
>        notifyFail: function(msg) {
> -        extension.emit("test-done", false, msg);
> +        extension.emit("test-done", false, msg, getStack());
>        },
>  
>        log: function(msg) {
> -        extension.emit("test-log", true, msg);
> +        extension.emit("test-log", true, msg, getStack());

No need for a stack here...
Attachment #8809687 - Flags: review?(aswan) → review+
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8809687 [details]
Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions.

https://reviewboard.mozilla.org/r/92224/#review92420

> No need for a stack here...

Yeah, I guess, but I figured it might be useful at some point so I may as well include it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9867579cf623cd1396fee24d18179a149399e92
Bug 1316780: Part 1 - Proxy extension events between the parent and child process. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c93fbd9ced3c64a4e48bb45ee90187be46f65212
Bug 1316780: Part 2 - Remove ext-test.js. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3edae1e4e0ac07b3032a30b2685d2cd8c85658
Bug 1316780: Part 3 - Capture the current unprivileged stack in browser.test.* assertion functions. r=aswan

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b9867579cf62
https://hg.mozilla.org/mozilla-central/rev/c93fbd9ced3c
https://hg.mozilla.org/mozilla-central/rev/ee3edae1e4e0
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.