add new talos tests with an active, but blank web extension to measure ts_paint and tp5o

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
this will need a few things:
1) disable non-e10s talos tests on all platforms to give us machine time for new tests (we were already over the threshold, this would kill us)
 - deploy taskcluster changes first so they are on all branches
 - deploy buildbot changes next
2) create a new test job 'g5' which will run new tests
 - create definition in talos.json so this is in tree and merged around
 - add buildbot changes so this new job can be scheduled
 - add taskcluster configs for g5 which will schedule the job on linux/osx
3) copy ts_paint -> ts_paint_webext, and tp5o -> tp5o_webext
 - simple test definitions which can land and run when 'g5' is enabled
4) create a blank webext and figure out how to get it installed in the default profile we use
 - need to determine how to install this programatically.  Currently all extensions are signed and copied to the profile folder before starting the browser
 - ensure that the webext that we use is invoking the right code paths, but not doing too much
5) deploy tests to all platforms
 - land changes in talos, ensure talos.json has the tests we want
6) compare and look for stability
 - do many retriggers, look at data over 100+ data points and at least 20 different pushes.
7) write documentation on https://wiki.mozilla.org/Buildbot/Talos/Tests
(Assignee)

Updated

a year ago
Depends on: 1364944
(Assignee)

Comment 1

a year ago
Created attachment 8868288 [details] [diff] [review]
initial prototype to get talos to install extensions during the warmup

steps 1 and 2 are mostly done, this patch is step 3.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Created attachment 8868765 [details] [diff] [review]
use marionette to install test specific web extensions for talos

this is much closer to production.  What is missing:
1) the dummy web extension (coming up sooner)
2) making this work and not crash tart and cart.
3) testing on windows/osx
Attachment #8868288 - Attachment is obsolete: true

Updated

a year ago
Blocks: 1363905
(Assignee)

Updated

a year ago
Depends on: 1366310
(Assignee)

Updated

a year ago
Depends on: 1366383
(Assignee)

Comment 3

a year ago
Created attachment 8869586 [details] [diff] [review]
support webextensions in talos including a blank one for new g5 tests

this patch is ready for review.  In doing this there is a big hack due to bug 1366310 with no clear solution/root cause.

To work around that we only use marionette when there are webextensions defined for a given test.  This is great for now, but in the future when someone wants to test ALL the tests with a dummy web extension we will not be able to and will have to suffer the failures.
Attachment #8868765 - Attachment is obsolete: true
Attachment #8869586 - Flags: review?(rwood)
Comment on attachment 8869586 [details] [diff] [review]
support webextensions in talos including a blank one for new g5 tests

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

Awesome! R+ just a few small notes/nits

::: testing/talos/talos/ffsetup.py
@@ +132,3 @@
>  
> +            client = Marionette(host='localhost', port=2828)
> +            client.start_session()

What if starting the session fails (I can't remember if there's a return value)?

@@ +133,5 @@
> +            client = Marionette(host='localhost', port=2828)
> +            client.start_session()
> +            addons = Addons(client)
> +
> +            if self.test_config.get('webextensions', None):

Not necessary, already in a "if self.test_config.get('webextensions', None):" block on line 120 above

@@ +147,5 @@
> +                        continue
> +                    if not os.path.exists(filename):
> +                        continue
> +
> +                    addons.install(filename)

What if this fails?

@@ +148,5 @@
> +                    if not os.path.exists(filename):
> +                        continue
> +
> +                    addons.install(filename)
> +            client.close()

Installing the webextensions doesn't create new window(s) does it? client.close will only end the marionette session if it's the last window open

::: testing/talos/talos/webextensions/dummy/manifest.json
@@ +3,5 @@
> +    "gecko": {
> +      "id": "talos@mozilla.org"
> +    }
> +  },
> +  "manifest_version": 2,

Just curious why version 2? Do traditional add-ons use manifest version 1 maybe?
Attachment #8869586 - Flags: review?(rwood) → review+
(Assignee)

Comment 5

a year ago
thanks for the review, I will address these and land or ask for a quick re-review if there are a lot of changes.  As a note, the try pushes all passed and showed valid data.
(Assignee)

Comment 6

a year ago
> ::: testing/talos/talos/ffsetup.py
> @@ +132,3 @@
> >  
> > +            client = Marionette(host='localhost', port=2828)
> > +            client.start_session()
> 
> What if starting the session fails (I can't remember if there's a return
> value)?

I will investigate and figure out a solution for failure.

> 
> @@ +133,5 @@
> > +            client = Marionette(host='localhost', port=2828)
> > +            client.start_session()
> > +            addons = Addons(client)
> > +
> > +            if self.test_config.get('webextensions', None):
> 
> Not necessary, already in a "if self.test_config.get('webextensions',
> None):" block on line 120 above

thanks, my oversight.

> 
> @@ +147,5 @@
> > +                        continue
> > +                    if not os.path.exists(filename):
> > +                        continue
> > +
> > +                    addons.install(filename)
> 
> What if this fails?

I will investigate and figure out a solution for failure.

> 
> @@ +148,5 @@
> > +                    if not os.path.exists(filename):
> > +                        continue
> > +
> > +                    addons.install(filename)
> > +            client.close()
> 
> Installing the webextensions doesn't create new window(s) does it?
> client.close will only end the marionette session if it's the last window
> open

There is one tab remaining and client.close() closes it and terminates the browser.  Is there another more preferred way to close the browser with Marionette?

> 
> ::: testing/talos/talos/webextensions/dummy/manifest.json
> > +  "manifest_version": 2,
> 
> Just curious why version 2? Do traditional add-ons use manifest version 1
> maybe?

Appears to be required:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/manifest_version
(Assignee)

Comment 7

a year ago
Created attachment 8870378 [details] [diff] [review]
changes to support checking for process state and closing the browser properly

this is meant to be applied on top of the first patch, so please consider this an incremental revision.  If you want, I can combine them into one patch and this can be a single change since it is most of the same code.
Attachment #8870378 - Flags: review?(rwood)
Comment on attachment 8870378 [details] [diff] [review]
changes to support checking for process state and closing the browser properly

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

LGTM with a couple of comments, unsure of the order of client.close & client.navigate

::: testing/talos/talos/ffsetup.py
@@ +136,5 @@
> +                client.start_session()
> +                addons = Addons(client)
> +            except Exception, e:
> +                print e
> +                raise TalosError("Failed to initialize Talos profile with Marionette") 

nit: whitespace at eol

@@ +156,1 @@
>              client.close()

Not sure about this (I'm following along in Bug 1366310). Isn't there the possibility that this will terminate the marionette connection completely on some platforms? If so, how will the client.navigate below work? Maybe the client.close() should be moved to after the client.navigate?

@@ +173,5 @@
> +        try:
> +            exit_code = proc.wait()
> +        except Exception, e:
> +            proc.kill()
> +            raise e

Do you want to raise TalosError here like above?
Attachment #8870378 - Flags: review?(rwood) → review+

Comment 9

a year ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b403f00d125e
add new talos tests with an active, but blank web extension to measure ts_paint and tp5o. r=rwood

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b403f00d125e
https://hg.mozilla.org/mozilla-central/rev/1629939669a7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.