1333484 - Firefox refresh marionette test inadvertently relies on other tests running before it for some success criteria

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

I'd seen this before and not investigated, and then :ckerschb ran into it again in bug 1307736 comment 39 so I decided to poke at this.

From looking at a one-click loaner, it seems running various tests beforehand is what's breaking things because they load other items in tabs which then show up in the list of tabs that appear after the Firefox refresh.

So I have a patch that removes the extra tabs before using reset, so we can accurately predict what tabs show up after the reset.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1333484 - make Firefox refresh marionette test not rely on previous tests,

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

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8829957 [details]
Bug 1333484 - make Firefox refresh marionette test not rely on previous tests,

https://reviewboard.mozilla.org/r/106912/#review107954

LGTM!

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:133
(Diff revision 1)
>                    }
>                  });
>                }
>              }
>            });
> +          let expectedTabs = new Set([]);

nit: I think you were playing with Array comprehensions before, but I don't think you need to pass in an empty array literal.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:137
(Diff revision 1)
>            });
> +          let expectedTabs = new Set([]);
>            for (let url of expectedURLs) {
> -            gBrowser.addTab(url);
> +            expectedTabs.add(gBrowser.addTab(url));
> +          }
> +          while (gBrowser.tabs.length > 2) {

I think `if (gBrowser.tabs.length > 2) {` would do the trick in a similar fashion.
Also, can you add a comment above it to explain what this is doing? Event though it's a string literal, it's still possible I think.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8829957 [details]
Bug 1333484 - make Firefox refresh marionette test not rely on previous tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106912/diff/1-2/

Comment 4

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:133
> (Diff revision 1)
> >                    }
> >                  });
> >                }
> >              }
> >            });
> > +          let expectedTabs = new Set([]);
> 
> nit: I think you were playing with Array comprehensions before, but I don't
> think you need to pass in an empty array literal.

Oops. Good point.

> ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:137
> (Diff revision 1)
> >            });
> > +          let expectedTabs = new Set([]);
> >            for (let url of expectedURLs) {
> > -            gBrowser.addTab(url);
> > +            expectedTabs.add(gBrowser.addTab(url));
> > +          }
> > +          while (gBrowser.tabs.length > 2) {
> 
> I think `if (gBrowser.tabs.length > 2) {` would do the trick in a similar
> fashion.
> Also, can you add a comment above it to explain what this is doing? Event
> though it's a string literal, it's still possible I think.

Egh, I was messing around to avoid the issues with removing from a live node collection while iterating. This is really not the best way to fix it though. I updated the patch to just clone the list of tabs to an array instead. I also added a comment. Can you check this still makes sense?

Comment 5

[Mike de Boer [:mikedeboer]]

Yup, still makes sense!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0963632525ac
make Firefox refresh marionette test not rely on previous tests, r=mikedeboer

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0963632525ac

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This was uplifted to Beta on the hopes that it'd fix a permafail from the uplift of bug 1315611. It didn't work, but I left it there anyway.
https://hg.mozilla.org/releases/mozilla-beta/rev/fb836ee72ce6