1438340 - Only install talos pageloader addon when required, not by default on all tests

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

[Robert Wood [:rwood]]

Talos addons and extensions are not being removed after tests complete.

This is causing issues when two tests of different types, that use different addons or extensions, are in the same test suite.

For example, in Bug 1384272 I am adding a new pageloader test to the chromez suite which already has the tresize startup test. I added the new pageloader test to the beginning of the suite. When tresize ran, after the pageloader test, tresize had both the pageloader addon and tresize test addon - so the tresize test started running and then the pageloader addon kicked in and started running a pageloader test at the exact same time! And because of Bug 1438339, it used the manifest from the previous test in the suite so it actually ran a full pageloader test at the same time as tresize.

Also this means that currently talos test results / performance numbers may potentially be affected if the browser has an addon/extension installed from a previous test, but is not needed in the current test.

After each test, we should remove (ideally uninstall, at the very list disable) any addons or extensions that were added by the test. This needs to be done at the test level, not the suite level.

Comment 1

[Robert Wood [:rwood]]

Ahhh... we are installing addons and extensions in ffsetup [1], into the Firefox profile, which is new on each test run so that's fine... no need to uninstall them because we start with a new / clean profile clone each time.

However... here in talos/cmdline.py [2]  we are automatically adding 'pageloader' and 'talos-powers' to be installed on every single test run (along with any extra addons/extensions defined in the talos/test.py).

So 'pageloader' and 'talos-powers' is installed every time. I can understand 'talos-powers' (it's required for browser init/getinfo.html) but pageloader is not required for every test i.e. tresize.

We should remove pageloader from the default extensions list, and add it in test.py where required. :jmaher, what do you think?

[1] https://searchfox.org/mozilla-central/rev/4234703a532006c5ef9ce09b4c487d88124526a0/testing/talos/talos/ffsetup.py#125

[2] https://searchfox.org/mozilla-central/rev/4234703a532006c5ef9ce09b4c487d88124526a0/testing/talos/talos/cmdline.py#100

Comment 2

[Robert Wood [:rwood]]

Don't really need info, I'll just go ahead with a patch :)

Comment 3

[Robert Wood [:rwood]]

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

Comment 4

[Robert Wood [:rwood]]

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

Comment 5

[Robert Wood [:rwood]]

Attachment: Bug 1438340 - Only install talos pageloader addon when required, not by default on all tests;

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

Comment 6

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

Comment on attachment 8951477 [details]
Bug 1438340 - Only install talos pageloader addon when required, not by default on all tests;

https://reviewboard.mozilla.org/r/220776/#review226704

::: testing/talos/talos/test.py:178
(Diff revision 1)
>  
>      1. Set up Firefox to restore from a given sessionstore.js file.
>      2. Launch Firefox.
>      3. Measure the delta between firstPaint and sessionRestored.
>      """
> -    extensions = \
> +    extensions = ['${talos}/pageloader', '${talos}/startup_test/sessionrestore/addon']

does sessionrestore need pageloader?

::: testing/talos/talos/test.py:300
(Diff revision 1)
>      """
>      Tests the amount of time it takes to start up a new content process and
>      initialize it to the point where it can start processing incoming URLs
>      to load.
>      """
> -    extensions = '${talos}/tests/cpstartup'
> +    extensions = ['${talos}/tests/cpstartup', '${talos}/pageloader']

since this inherits from PageloaderTeset, why do we need to add pageloader extension here?

Comment 7

[Robert Wood [:rwood]]

Comment on attachment 8951477 [details]
Bug 1438340 - Only install talos pageloader addon when required, not by default on all tests;

https://reviewboard.mozilla.org/r/220776/#review226704

> does sessionrestore need pageloader?

Yep, I tried it without it and the test hung up - turns out the session restore addon uses the pageloader addon:

https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/testing/talos/talos/startup_test/sessionrestore/addon/content/index.html#7

> since this inherits from PageloaderTeset, why do we need to add pageloader extension here?

Yep, because when the same test key/setting is set under a test vs the test parent class, the test setting replaces it. IMO that is the correct behaviour as the test subclass is overidding the attribute.

Comment 8

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

Comment on attachment 8951477 [details]
Bug 1438340 - Only install talos pageloader addon when required, not by default on all tests;

https://reviewboard.mozilla.org/r/220776/#review226834

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c45fa02b3a0
Only install talos pageloader addon when required, not by default on all tests; r=jmaher

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1c45fa02b3a0