Closed Bug 1438340 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: Talos, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: rwood, Assigned: rwood)

References

Details

(Whiteboard: [PI:February])

Attachments

(1 file)

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.
Blocks: 1384272
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
Flags: needinfo?(jmaher)
Don't really need info, I'll just go ahead with a patch :)
Flags: needinfo?(jmaher)
Summary: Remove talos web-extensions and addons after each test completes → Only install talos pageloader addon when required, not by default on all tests
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?
Attachment #8951477 - Flags: review?(jmaher) → review-
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 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
Attachment #8951477 - Flags: review- → review+
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c45fa02b3a0
Only install talos pageloader addon when required, not by default on all tests; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/1c45fa02b3a0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.