Closed Bug 1316780 Opened 3 years ago Closed 3 years ago

browser.test.* stacks are no longer useful

Categories

(WebExtensions :: General, defect)

51 Branch
defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kmag, Assigned: kmag)

Details

Attachments

(3 files)

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 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 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+
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 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+
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.