Closed Bug 1451159 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: General, enhancement, P2)

Product:
Testing
Component:
General
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bdanforth, Assigned: ahal)

References

(Blocks 1 open bug)

Details

(Whiteboard: [PI:May][wptsync upstream error])

Attachments

(4 files)

Bug 1451159 - [mozprofile] Implement ability to merge other profile directories into the current one
59 bytes, text/x-review-board-request
gbrown
: review+
Details
Bug 1451159 - [testing/profiles] Store profile data in actual profile look alike folders
59 bytes, text/x-review-board-request
gbrown
: review+
Details
Bug 1451159 - [testing/profiles] Use 'format' for interpolation instead of %s
59 bytes, text/x-review-board-request
gbrown
: review+
Details
Bug 1451159 - [mochitest] Load profile data from testing/profiles/common
59 bytes, text/x-review-board-request
gbrown
: review+
Details 
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.
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?
Flags: needinfo?(ahalberstadt)
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 :).
Flags: needinfo?(ahalberstadt)
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.
Whiteboard: [PI:April]
Priority: -- → P3
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.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [PI:April] → [PI:May]
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 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.
Attachment #8969772 - Flags: feedback?(james)
Blocks: 1458571
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?
Attachment #8969771 - Flags: review?(gbrown) → review+
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"?
Attachment #8969772 - Flags: review?(gbrown) → review+
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.
Attachment #8969773 - Flags: review?(gbrown) → review+
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?
Attachment #8969774 - Flags: review?(gbrown) → review+
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 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 :)
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.
Pushed by ahalberstadt@mozilla.com:
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
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10834 for changes under testing/web-platform/tests
Whiteboard: [PI:May] → [PI:May][wptsync upstream]
Whiteboard: [PI:May][wptsync upstream] → [PI:May][wptsync upstream error]
Upstream PR was closed without merging
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.
Flags: needinfo?(ahalberstadt)
Pushed by ahalberstadt@mozilla.com:
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
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.
Blocks: 1459598
Blocks: 1460912
Blocks: 1460914
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.
Blocks: 1462007
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: