1334691 - Consolidate tab moves tests

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[Andy McKay]

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 1

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f44fa9dcb004edd8e7c1e81e328740437319a356

Comment 2

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=006eca5fa5f8c5a9b7b9fef3dac351e94a1c5862

Comment 3

[Andy McKay]

Attachment: bug 1334691 consolidate a couple of tests down

Review commit: https://reviewboard.mozilla.org/r/145582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145582/

Comment 4

[Bob Silverberg [:bsilverberg]]

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?

Comment 5

[Andy McKay]

(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.

Comment 6

[Bob Silverberg [:bsilverberg]]

(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 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8874160 [details]
bug 1334691 consolidate a couple of tests down

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

Nice work. Thanks Andy.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8874160 [details]
bug 1334691 consolidate a couple of tests down

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce85883942f7
consolidate a couple of tests down r=bsilverberg

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce85883942f7