Closed Bug 1334691 Opened 4 years ago Closed 4 years ago

Consolidate tab moves tests

Categories

(WebExtensions :: Frontend, defect, P5)

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: andy+bugzilla, Assigned: andy+bugzilla)

Details

(Whiteboard: triaged)

Attachments

(1 file)

59 bytes, text/x-review-board-request
bsilverberg
: review+
Details
If bug 1323311 sticks, then quite a few other move tests can probably be consolidated down into the test cases in browser_ext_tabs_move_window_array.js, which makes testing different move scenarios much easier.
Comment on attachment 8874160 [details]
bug 1334691 consolidate a couple of tests down

https://reviewboard.mozilla.org/r/145582/#review156720

::: browser/components/extensions/test/browser/browser_ext_tabs_move_array.js:70
(Diff revision 1)
> +      // Assuming that tab.id of 12345 does not exist.
> +      await browser.test.assertRejects(
> +        browser.tabs.move([firstId, 12345], {index: -1}),
> +        /Invalid tab/,
> +        "Should receive invalid tab error");
> +      // The first argument got moved, the second on failed.

Wow, is that the way it's supposed to work? The promise gets rejected but some of the tabs are moved anyway? What happens if you pass `[12345, firstId]` instead (i.e., the first tab specified is invalid)? Does it still move `firstId` but not `12345`, or does it move neither?
(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> Wow, is that the way it's supposed to work? The promise gets rejected but
> some of the tabs are moved anyway? 

It's what Chrome does. Whether that's the way it *should* do it is another question :)

> What happens if you pass `[12345,
> firstId]` instead (i.e., the first tab specified is invalid)? Does it still
> move `firstId` but not `12345`, or does it move neither?

It fails on the first invalid tab, so firstId will not be moved.
(In reply to Andy McKay [:andym] from comment #5)
> (In reply to Bob Silverberg [:bsilverberg] from comment #4)
> > Wow, is that the way it's supposed to work? The promise gets rejected but
> > some of the tabs are moved anyway? 
> 
> It's what Chrome does. Whether that's the way it *should* do it is another
> question :)

Yeah, I think that's a valid question to ask, although I'm not sure where we should ask it.
This patch is fine as it matches Chrome's behaviour, so I'll give it my r+.
Comment on attachment 8874160 [details]
bug 1334691 consolidate a couple of tests down

https://reviewboard.mozilla.org/r/145582/#review157234

Nice work. Thanks Andy.
Attachment #8874160 - Flags: review?(bob.silverberg)
Comment on attachment 8874160 [details]
bug 1334691 consolidate a couple of tests down

https://reviewboard.mozilla.org/r/145582/#review157236
Attachment #8874160 - Flags: review?(bob.silverberg) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce85883942f7
consolidate a couple of tests down r=bsilverberg
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce85883942f7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Toolkit → WebExtensions
Attachment #8874160 - Flags: review?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.