Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: kmag, Assigned: kmag)

Tracking

51 Branch
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

Attachments

(3 attachments)

Assignee

Description

3 years ago
Follow-up to bug 1313956
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

3 years ago
mozreview-review
Comment on attachment 8808484 [details]
Bug 1315872: Add browser.test.assertRejects and assertThrows.

https://reviewboard.mozilla.org/r/91320/#review91316

so long testAPIFactory...
Attachment #8808484 - Flags: review?(aswan) → review+
Assignee

Updated

3 years ago
Keywords: leave-open

Comment 6

3 years ago
mozreview-review
Comment on attachment 8808485 [details]
Bug 1315872: Refactor tests that check for promise rejection to use assertRejects.

https://reviewboard.mozilla.org/r/91322/#review91326

::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon_permissions.js:26
(Diff revision 1)
>              detail.tabId = tabId;
>              promises.push(
> -              browser[api].setIcon(detail).then(
> -                () => {
> -                  browser.test.fail("Expected an error on invalid icon size.");
> -                  browser.test.notifyFail("setIcon with invalid icon size");
> +              browser.test.assertRejects(
> +                browser[api].setIcon(detail),
> +                /must be an integer/,
> +                "setIcon with invalid icon size"));

We lost the notifyFail() here, though I guess you can make a case that it didn't need to be there in the first place?
(the same pattern is repeated below)
Attachment #8808485 - Flags: review?(aswan) → review+

Comment 7

3 years ago
mozreview-review
Comment on attachment 8808486 [details]
Bug 1315872: Refactor browser_ext_tabs_cookieStoreId.

https://reviewboard.mozilla.org/r/91324/#review91472

::: browser/components/extensions/test/browser/browser_ext_tabs_cookieStoreId.js:55
(Diff revision 1)
> +            tab = await browser.tabs.create({
> -            windowId: data.privateTab ? this.privateWindowId : this.defaultWindowId,
> +              windowId: data.privateTab ? this.privateWindowId : this.defaultWindowId,
> -            cookieStoreId: data.cookieStoreId,
> +              cookieStoreId: data.cookieStoreId,
> -          }).then((tab) => {
> -            // Tests for tab creation
> -            testTab(data, tab);
> +            });
> +
> +            browser.test.assertTrue(!data.failure, "we want a success");

I think the call to `testTab()` below covers this?  What a helpful message...
Attachment #8808486 - Flags: review?(aswan) → review+
Assignee

Comment 8

3 years ago
mozreview-review-reply
Comment on attachment 8808485 [details]
Bug 1315872: Refactor tests that check for promise rejection to use assertRejects.

https://reviewboard.mozilla.org/r/91322/#review91326

> We lost the notifyFail() here, though I guess you can make a case that it didn't need to be there in the first place?
> (the same pattern is repeated below)

Yeah, but it wasn't really necessary. We get a failure from the error check alone.
Assignee

Comment 9

3 years ago
mozreview-review-reply
Comment on attachment 8808486 [details]
Bug 1315872: Refactor browser_ext_tabs_cookieStoreId.

https://reviewboard.mozilla.org/r/91324/#review91472

> I think the call to `testTab()` below covers this?  What a helpful message...

Hm. Yeah, I guess... Blah.

Updated

3 years ago
Whiteboard: triaged
Assignee

Updated

3 years ago
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.