1190268 - Refactor FFSetup logic and implementation in Talos

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Julien Pagès (:parkouss)]

FFSetup is intended to setup the browser before running a test , from what I understand.

Right now it only create the profile and initialize it, in a non easy way to read (code is splitted into ffsetup.py and ttest.py, and the API is hard to manipulate).

I think we should think about ffsetup as a more general test initializer - not only for profile. Also we should think again the implementation to encapsulate more and make it easier to use from the outside.

Comment 1

[Julien Pagès (:parkouss)]

Attachment: refactor_ffsetup.patch

This patch needs the patch in bug 1190195 to not conflict and should be applied on top of that. Though it should be easy to resolve conflicts if required, the two patches are making changes on different logical parts.

I removed the sleep of 5 seconds after the profile initialization - so this should be tested well (maybe 10 or 20 runs to be sure). In case we see intermittents because of that, I suggest we look at resolving bug 1190265 then try again here instead of throwing away that patch or put back again the sleep.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8642263 [details] [diff] [review]
refactor_ffsetup.patch

Review of attachment 8642263 [details] [diff] [review]:
-----------------------------------------------------------------

while I have one comment below this looks good overall.  We should ensure we have done a good job of testing this- a lot of fundamental changes and we have odd issues in production sometimes.  It can be pgo, or sometimes just something random.

::: talos/ffsetup.py
@@ +80,5 @@
>                  webserver_var = webserver
>                  if '://' not in webserver:
>                      webserver_var = 'http://' + webserver_var
>                  value = utils.interpolate(value, webserver=webserver_var)
> +                preferences[name] = value

I don't understand this loop as much, are we iterating through every single preference and then setting the value to webserver?  It seems like we could set webserver_var once and then substitute as needed.

Comment 3

[Julien Pagès (:parkouss)]

Eh, that's mainly a copy paste of the old code. But while I am at it, I can rewrite it as suggested - thanks for notifying!

Comment 4

[Julien Pagès (:parkouss)]

Pushed to try, 4 times each job:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bac71314d33&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=running&filter-resultStatus=pending

(this filter the user-cancelled, I did a mistake with previous scheduled jobs)

Comment 5

[Julien Pagès (:parkouss)]

try push with PGO builds:

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

Comment 6

[Julien Pagès (:parkouss)]

So, all went good - except for xperf jobs... Too bad. Retrying those with the sleep 5 seconds back soon.

Comment 7

[Julien Pagès (:parkouss)]

Not a sleep issue. Trying with passing environment to every subprocess call available.

Comment 8

[Julien Pagès (:parkouss)]

Still broken. pushed a new try with early definition of the env var "MOZ_MAIN_THREAD_IO_LOG" and using env vars when profile is initialized.

Comment 9

[Julien Pagès (:parkouss)]

Erf, still broken. I'm running out of ideas. Joel, if you have an idea...

Else I may leave this bug open, and work on other stuff like log. I would like to avoid it to not have merge conflicts later...

Comment 10

[Julien Pagès (:parkouss)]

After our discussion on irc, I applied this fix on top of the patch:

https://bitbucket.org/parkouss/talos/commits/3681cf6cf9d6d4815c3886ed941536202a8bc358?at=default

And retried some xperf jobs on the same try. There was 13 failures (and 1 success), we should not have more failures than that if that works.

Comment 11

[Julien Pagès (:parkouss)]

All green!

Landed in https://hg.mozilla.org/build/talos/rev/5bba8c6b295f.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add env variables to ffsetup processhandler (1.0)

Comment 13

[Julien Pagès (:parkouss)]

Comment on attachment 8645733 [details] [diff] [review]
add env variables to ffsetup processhandler (1.0)

Nice catch! If this fix the e10s issue, ship it!

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/build/talos/rev/9fc5954d0582