1451159 - Provide a way to install add-on XPIs for testing in the tree

Status: RESOLVED FIXED

(Testing :: General, enhancement, P2)

Description

[Bianca Danforth [:bdanforth]]

Shield studies and other applications are often developed outside of the tree using XPI build artifacts for QA and add-on-specific testing. To perform regression tests in the tree, the current process is quite complex.

It would improve the process a great deal if there were a directory in the tree where an XPI could land to be installed as part of the profile for regression testing locally and on the try server.

Comment 1

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

my thinking here is that if we could have a patch that put a file in a directory (lets say testing/profile/) that file would be installed in the test profile used for unit tests.  A few things would probably be needed:
addon.xpi (bundled, not signed)
addon-prefs.js (user_pref() definitions to ensure the addon is properly configured- also to set server IP to http://localhost/dummy or something to prevent network access)

I would expect failures/false positives if:
* addon accessed the internet at all (pinging home)
* addon required specific environment/content/user interactions that otherwise makes it null/void
* specific tests that count the number of addons or verify icons

most of the work here would need to be in mozprofile and where we call mozprofile to pass in the additional files.

Doing this work allows us to ensure shield studies are not going to have odd interactions with users- a better peace of mind for not breaking things or slowing things down.  We could also use this logic to extend generally to addons as well.

I don't see this bug as solving:
* managing the load on our systems
* interpreting the results (perf data, intermittents, etc.)
* choosing the appropriate tests
* easily validating against different build types

:ahal, I would be interested to hear your thoughts on this?

Comment 2

[Andrew Halberstadt [:ahal]]

That makes sense to me, I'd pictures folders like:
testing/profiles/ci
testing/profiles/perf
testing/profiles/mochitest (maybe?)

Each directory could have prefs files and/or extension directory. Harnesses would opt-in to the ones they want to apply. E.g, Mochitest might use both the 'ci' and 'mochitest' profiles. Talos might use the 'ci' and 'perf' profiles. Reftest might only use the 'ci' profile.

So dropping an extension in testing/profiles/ci/extensions would get it installed in every harness that uses the 'ci' profile (which we could make sure is all of them).

The only part I don't like about this is that the profiles don't live next to the harnesses that use them. But having them all live together is also nice for finding where prefs are set. I could be convinced to do this another way :).

Comment 3

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

I agree this adds more randomization and chaos to our problem of managing profile prefs, etc.  Possibly this could set the stage for prefs_general.js and other related bits.

The main goal for calling out a central directory is that shield study authors and addon authors won't necessarily need to know where all the harnesses live in tree and the different nuances- if we can ensure a single location (or easy to follow instructions for multiple directories) then that seems reasonable.  Keep in mind most consumers of this are people that don't develop on m-c and might not even have a clone locally.

I am adding this to the April list of work- we have a few other things on our plate for the next couple weeks- if all goes well this could realistically be done in April- or likewise we can help mentor eager hackers who want to make this happen.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1451159 - [mozprofile] Implement ability to merge other profile directories into the current one

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

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1451159 - [testing/profiles] Store profile data in actual profile look alike folders

This moves testing/profiles/prefs_general.js to
testing/profiles/common/user.js. It also adds an 'extensions' folder to the
common profile. Dropping extension files here will get them installed in all
test harnesses (useful for testing on try).

The idea is that all test harnesses will eventually use this 'common' profile.
We'll also create some new per harness profiles, e.g testing/profiles/mochitest
and testing/profiles/reftest. This way there will be a single location
developers can go to set preferences, both for a specific harness, and across
all harnesses.

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

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1451159 - [testing/profiles] Use 'format' for interpolation instead of %s

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

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1451159 - [mochitest] Load profile data from testing/profiles/common

This makes mochitest use mozprofile's new 'merge' feature instead of
explicitly loading the user.js preference file.

This means that any extensions that get dropped into
testing/profiles/common/extensions, will automatically run as part of the
mochitest run. This can be useful for testing how extensions impact tests
locally or on try.

In the near future, all our other test harnesses will also start using this
profile directory.

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

Comment 8

[Andrew Halberstadt [:ahal]]

This series isn't quite ready to land, there are a couple test failures left to investigate. Unfortunately I'm PTO next week so this will have to wait until early May to finish up.

This series introduces a new 'profile directory' concept to testing/profiles. Basically we'll create a bunch of profile-like directories:

testing
  - profiles
    - common
      - user.js  # prefs shared across all harnesses
      - extensions
        - <put shared extensions here>
    - mochitest
      - user.js  # mochitest specific prefs
      - extensions
        - <put mochitest specific extensions here>
    - reftest
      <etc>

This way when we (or developers) want to set a new pref, either globally across all harnesses or limited to specific harnesses, it will all be in the same place. We can do the same thing with extensions. These extension dirs will also let you drop extensions in temporarily for local testing or on try.

This series only deals with mochitest, I was planning to do the others in a follow-up bug.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969774 [details]
Bug 1451159 - [mochitest] Load profile data from testing/profiles/common

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238598/diff/1-2/

Comment 10

[Andrew Halberstadt [:ahal]]

I think I'd like to get what I have so far landed in this bug and file follow-up bugs to tackle the other suites. This way I can start working on talos/raptor next while this is going through review, then finally reftest/xpcshell/wpt (if time permits).

Try run is looking good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6addc24f410a79f59d00f97c891c6231333577a

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969771 [details]
Bug 1451159 - [mozprofile] Implement ability to merge other profile directories into the current one

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238592/diff/1-2/

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969772 [details]
Bug 1451159 - [testing/profiles] Store profile data in actual profile look alike folders

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238594/diff/1-2/

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969773 [details]
Bug 1451159 - [testing/profiles] Use 'format' for interpolation instead of %s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238596/diff/1-2/

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969774 [details]
Bug 1451159 - [mochitest] Load profile data from testing/profiles/common

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238598/diff/2-3/

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969772 [details]
Bug 1451159 - [testing/profiles] Store profile data in actual profile look alike folders

Hey James, was hoping you could take a quick look at the web-platform changes in this commit. I think this is all that's needed but just want to make sure I didn't miss something somewhere else.

Fwiw, the wpt tasks look green on try.

Comment 16

[Geoff Brown [:gbrown]]

Comment on attachment 8969771 [details]
Bug 1451159 - [mozprofile] Implement ability to merge other profile directories into the current one

https://reviewboard.mozilla.org/r/238592/#review247058

This opens up some interesting possibilities for test profile management -- good stuff.

::: testing/mozbase/mozprofile/mozprofile/profile.py:39
(Diff revision 2)
>      def __init__(self, profile=None, addons=None, preferences=None, restore=True):
> -        self._addons = addons
> +        """Create a new Profile.
> +
> +        All arguments are optional.
> +
> +        :param profile: Path to an existing profile. If not specified, a new profile

Thanks for adding this descriptive comment - I wish there were more of these throughout our code base.

nit - On first reading, I find myself wondering "what is an existing profile? what components are required in the existing profile?"

::: testing/mozbase/mozprofile/mozprofile/profile.py:179
(Diff revision 2)
>        # profile.cleanup() has been called here
>      """
> +    preference_file_names = ('user.js', 'prefs.js')
>  
>      def __init__(self, profile=None, addons=None, preferences=None, locations=None,
> -                 proxy=None, restore=True, whitelistpaths=None):
> +                 proxy=None, restore=True, whitelistpaths=None, **kwargs):

Why add kwargs?

Comment 17

[Geoff Brown [:gbrown]]

Comment on attachment 8969772 [details]
Bug 1451159 - [testing/profiles] Store profile data in actual profile look alike folders

https://reviewboard.mozilla.org/r/238594/#review247090

::: testing/testsuite-targets.mk:246
(Diff revision 2)
>  
>  stage-steeplechase: make-stage-dir
>  	$(NSINSTALL) -D $(PKG_STAGE)/steeplechase/
>  	cp -RL $(DEPTH)/_tests/steeplechase $(PKG_STAGE)/steeplechase/tests
>  	cp -RL $(DIST)/xpi-stage/specialpowers $(PKG_STAGE)/steeplechase
> -	cp -RL $(topsrcdir)/testing/profiles/prefs_general.js $(PKG_STAGE)/steeplechase
> +	cp -RL $(topsrcdir)/testing/profiles/common/user.js $(PKG_STAGE)/steeplechase/prefs_general.js

Does steeplechase require a "prefs_general.js"?

Comment 18

[Geoff Brown [:gbrown]]

Comment on attachment 8969773 [details]
Bug 1451159 - [testing/profiles] Use 'format' for interpolation instead of %s

https://reviewboard.mozilla.org/r/238596/#review247134

Since runjunit.py is being touched, I would like to see a try run with geckoview-junit.

Comment 19

[Geoff Brown [:gbrown]]

Comment on attachment 8969774 [details]
Bug 1451159 - [mochitest] Load profile data from testing/profiles/common

https://reviewboard.mozilla.org/r/238598/#review247094

r+ with junit locations issue addressed.

::: testing/mochitest/runjunit.py:102
(Diff revision 3)
> -                 'http': self.options.httpPort,
> -                 'https': self.options.sslPort,
> -                 'ws': self.options.sslPort
> -                 }
>  
> -        self.profile = Profile(locations=self.locations, preferences=prefs,
> +        self.profile = Profile(proxy=self.proxy(self.options))

What happens to locations here?

Comment 20

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969771 [details]
Bug 1451159 - [mozprofile] Implement ability to merge other profile directories into the current one

https://reviewboard.mozilla.org/r/238592/#review247058

> Thanks for adding this descriptive comment - I wish there were more of these throughout our code base.
> 
> nit - On first reading, I find myself wondering "what is an existing profile? what components are required in the existing profile?"

I think you're right, it doesn't need to be existing, I'll modify the comment.

Btw, our doc generation has the sphinx napoleon extension installed, which means we can (and should probably start) writing docstrings that look like this:
http://www.sphinx-doc.org/en/stable/ext/napoleon.html

I find that much more readable than the default autodoc format.

> Why add kwargs?

Good find. At some intermediate point I had added a new BaseProfile only argument and wanted to forward it from the Profile class. I guess I removed that argument later and didn't revert the \*\*kwargs change.

I think I'll leave it in though as it's a good practice to forward unrecognized args to the parent class anyway.

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969774 [details]
Bug 1451159 - [mochitest] Load profile data from testing/profiles/common

https://reviewboard.mozilla.org/r/238598/#review247094

> What happens to locations here?

Oops, thanks for paying attention :)

Comment 22

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969771 [details]
Bug 1451159 - [mozprofile] Implement ability to merge other profile directories into the current one

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238592/diff/2-3/

Comment 23

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969772 [details]
Bug 1451159 - [testing/profiles] Store profile data in actual profile look alike folders

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238594/diff/2-3/

Comment 24

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969773 [details]
Bug 1451159 - [testing/profiles] Use 'format' for interpolation instead of %s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238596/diff/2-3/

Comment 25

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969774 [details]
Bug 1451159 - [mochitest] Load profile data from testing/profiles/common

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238598/diff/3-4/

Comment 26

[Andrew Halberstadt [:ahal]]

I didn't really mean to cancel the feedback request (apparently mozreview clobbers it after a re-push). But seeing as all of the wpt tests were green on try, I'm quite confident the prefs are still getting picked up properly. So it's probably safe enough to go ahead and land this without James' feedback.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79931e4a2bfb
[mozprofile] Implement ability to merge other profile directories into the current one r=gbrown
https://hg.mozilla.org/integration/autoland/rev/3914937893de
[testing/profiles] Store profile data in actual profile look alike folders r=gbrown
https://hg.mozilla.org/integration/autoland/rev/f90a99682382
[testing/profiles] Use 'format' for interpolation instead of %s r=gbrown
https://hg.mozilla.org/integration/autoland/rev/72926ae685af
[mochitest] Load profile data from testing/profiles/common r=gbrown

Comment 28

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 4 changesets (bug 1451159) for linux build bustages.

Backout push:  https://hg.mozilla.org/integration/autoland/rev/baf0ff2fb10ea14a68e12b9a1f8f73cb4ea7fa1f
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=72926ae685af24250b0b65fdfac4e3cace9f2a90
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=176838625&repo=autoland

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10834 for changes under testing/web-platform/tests

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/w3c/web-platform-tests/pull/10834
* continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/374653377?utm_source=github_status&utm_medium=notification)

Comment 31

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 32

[Andrew Halberstadt [:ahal]]

Oops, this broke pgo and valgrind builds for the same reason. The JUnit failure is something else (I guess those tasks don't have 'mochitest' in the name and therefore were skipped by my try run). Both fixes look easy.

Comment 33

[Andrew Halberstadt [:ahal]]

Looks like this fixes everything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3acc0569f55873dc24f91cfc679a7e70e4d68e6

(flake8 error also fixed)

Comment 34

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969771 [details]
Bug 1451159 - [mozprofile] Implement ability to merge other profile directories into the current one

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238592/diff/3-4/

Comment 35

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969772 [details]
Bug 1451159 - [testing/profiles] Store profile data in actual profile look alike folders

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238594/diff/3-4/

Comment 36

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969773 [details]
Bug 1451159 - [testing/profiles] Use 'format' for interpolation instead of %s

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238596/diff/3-4/

Comment 37

[Andrew Halberstadt [:ahal]]

Comment on attachment 8969774 [details]
Bug 1451159 - [mochitest] Load profile data from testing/profiles/common

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238598/diff/4-5/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05cf74997197
[mozprofile] Implement ability to merge other profile directories into the current one r=gbrown
https://hg.mozilla.org/integration/autoland/rev/9a2ba1c7b0ec
[testing/profiles] Store profile data in actual profile look alike folders r=gbrown
https://hg.mozilla.org/integration/autoland/rev/5f1efa279b09
[testing/profiles] Use 'format' for interpolation instead of %s r=gbrown
https://hg.mozilla.org/integration/autoland/rev/0ba46c050bb2
[mochitest] Load profile data from testing/profiles/common r=gbrown

Comment 39

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/05cf74997197
https://hg.mozilla.org/mozilla-central/rev/9a2ba1c7b0ec
https://hg.mozilla.org/mozilla-central/rev/5f1efa279b09
https://hg.mozilla.org/mozilla-central/rev/0ba46c050bb2

Comment 40

[Andrew Halberstadt [:ahal]]

Bianca, you should be able to drop extensions in testing/profiles/common/extensions and they will be installed in mochitest (both locally with mach, and on try). Bug 1458571 will get this same system working with talos, and additional follow-up bugs will be filed to get it working with other harnesses as well.

Please let me know if anything's not working as expected, or if you need specific harnesses prioritized over others. I'm planning to post to the newsgroups once this is working more widely with additional harnesses.

Comment 41

[Bianca Danforth [:bdanforth]]

Hi :ahal; I was able to verify this both locally and on the try server by writing a simple test to check for the presence/absence of my add-on. It seems to work great. Any team testing their add-on with this method would probably benefit from a similar test to check that the add-on was installed successfully, is active, etc., though perhaps that's best handled by the add-on developer.

It looks like the Talos bug (1458571) was resolved -- is that one ready to try out as well? Would the XPI install path be the same?

Regarding which test harnesses to prioritize, I spoke with :rhelmer, and likely the most important are these Mochitests, along with Talos and XPCShell.

Thanks very much for your work on this. This will go a long way to help make testing easier for this kind of development. rhelmer and I plan to meet with jmaher this week (or very soon) to discuss next steps more broadly, so if there's anything I'm not thinking of right now, hopefully he or I can let you know.