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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Migration
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

53 Branch
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
(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)

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0963632525ac
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Comment 8

a year ago
uplift
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
status-firefox53: --- → fixed
You need to log in before you can comment on or make changes to this bug.