Closed Bug 1333484 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Migration, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

Details

Attachments

(1 file)

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 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.
Attachment #8829957 - Flags: review?(mdeboer) → review+
(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?
Flags: needinfo?(mdeboer)
Yup, still makes sense!
Flags: needinfo?(mdeboer)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0963632525ac
make Firefox refresh marionette test not rely on previous tests, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/0963632525ac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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
You need to log in before you can comment on or make changes to this bug.