Closed Bug 1315872 Opened 8 years ago Closed 7 years ago

Cleanup tests

Categories

(WebExtensions :: General, defect)

51 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Whiteboard: triaged)

Attachments

(3 files)

Follow-up to bug 1313956
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+
Keywords: leave-open
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 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+
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.
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.
Whiteboard: triaged
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: