759396 - jetperf.py should have a way of downloading testing addons from a (hg) repository

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Jeff Hammel]

We now have an hg repository of a set of addons to be tested with
jetperf:

http://hg.mozilla.org/projects/addon-sdk-jetperf-tests/

jetperf.py should have an option to test these addons.

Comment 1

[Jeff Hammel]

So there are a few issues:

1. How should the addons in the repository interact with the existing --addon switch?  They can extend it, which is my inclination, but other solutions are possible as well

2. Are we loading one/all of these addons in the repository or ???  Currently using multiple --addon switches will do a single run with all of the addons installed (vs none with the baseline run).  I'm inclined to load all of the addons for expedience for the time being.  If not, we should spec out exactly what is desired

Comment 2

[Alexandre Poirot [:ochameau]]

If we are only testings probes set in addons source code (time to run main.js, time to run a content script from a page-mod, ...), you can install all of them in a single run.
If we are running other kind of perf test while these addons are installed, it might be a good idea to run them individually. I would allow to identify API that introduce perf regressions. For example, only page-mod-addon may introduce pref regression for pageload tests.

Having said that, we can start by doing a single run and enventually tune this later if we think it would really worth it.

Comment 3

[Jeff Hammel]

Attachment: example implementation

Basic implementation. Not 100% sure if this is what we want to do but this does work.

Comment 4

[Aki Sasaki (not active)]

Comment on attachment 628513 [details] [diff] [review]
example implementation

Hm.

I'd lean more towards this approach:

1. Instead of having 2 different repo config options, go with a list of repos, like this:

http://hg.mozilla.org/build/mozharness/file/636e0f7b6ab6/configs/single_locale/mozilla-aurora_android.py#l26

To clone those, instead of calling MercurialVCS.clone(), we'd call self.vcs_checkout_repos(self.config['repos']) (after inheriting MercurialScript).

We know what we're going to name the directory for the addons repo.
If the addons in there are going to be static, we can then, by default, specify that the default addons to run will be

    ["addons_dir/empty-addon", "addons_dir/pagemod-addon", "addons_dir/widget_addon"]

although we can override that list with command line options.

Alternately, we could use the cloned_addons code in your patch to look inside that directory.  I'd just use it as a generic "parse this directory for subdirectories with addons" instead of necessarily tying it to self.test_addons_clone.

Does that make sense?

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 628513 [details] [diff] [review]
example implementation

I'm missing test infrastructure big picture. 
But shouldn't we move this code to a script specific to talos and let jetperf.py only accept addons direcctories?
So that talos (or whatever call jetperf automatically on xx revisions), will call a first script like "run_jetpack_perf_tests.py", that will download necessary tests addons, put them somewhere and then call jetperf.py. 
Why would I do that? In order to keep jetperf.py a generic tool to test jetpack performances on any given addons.

Having said that, I'm totally fine with this implementation.

Comment 6

[Jeff Hammel]

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> Comment on attachment 628513 [details] [diff] [review]
> example implementation
> 
> I'm missing test infrastructure big picture. 
> But shouldn't we move this code to a script specific to talos and let
> jetperf.py only accept addons direcctories?
> So that talos (or whatever call jetperf automatically on xx revisions), will
> call a first script like "run_jetpack_perf_tests.py", that will download
> necessary tests addons, put them somewhere and then call jetperf.py. 
> Why would I do that? In order to keep jetperf.py a generic tool to test
> jetpack performances on any given addons.
> 
> Having said that, I'm totally fine with this implementation.

I'm not sure if I understand what you're suggesting.  To give a brief overview of the architecture at this point:

- jetperf.py:JetPerf inherits from mozharness.mozilla.testing.talos:Talos, which downloads, installs, and invokes the harness from http://hg.mozilla.org/build/talos

- jetperf.py will be invoked by buildbot on (hg) changes

Does that make sense?  So jetperf.py is the highest level script outside of buildbot.  In light of that, I'm not sure what exactly you're suggesting, could you clarify please?

Comment 7

[Alexandre Poirot [:ochameau]]

Ok so if jetperf.py is mainly designed to be invoked automatically by buildbot, then this patch is totally fine. Otherwise, if we want to allow some humans to use jetperf.py as a tool to test their own addons performances; I'd suggest to keep this patch for yet another script between buildbot and jetperf.py.

Does that make sense?
In any case, it is just a feedback suggestion, it is not that important.

Comment 8

[Aki Sasaki (not active)]

The script should be able to do multiple workflows; that's what mozharness is designed to do.
Production will have production-specific config files.
The default workflow, when run by hand (potentially with a separate standalone config file), should be developer-friendly.

Comment 9

[Jeff Hammel]

> We know what we're going to name the directory for the addons repo.
> If the addons in there are going to be static, we can then, by default, specify that the default addons to run will be
> 
>     ["addons_dir/empty-addon", "addons_dir/pagemod-addon", "addons_dir/widget_addon"]
> 
> although we can override that list with command line options.

I tend to think it doesn't make sense to have a hard-coded list of paths that live in another repository.  If the repository given is configuration (vs hardcoded) as it currently is, this prohibits jetpack and other developers from adding new addons or testing different combinations of addons.  If the addons lived in the mozharness repo, this would make more sense, IMO.
 
> Alternately, we could use the cloned_addons code in your patch to look inside that directory.  I'd just use it as a generic "parse this directory for subdirectories with addons" instead of necessarily tying it to self.test_addons_clone.

I prefer this method, as it is more flexible and IMO more appropriate to where a repository containing jetpacks is specified in configuration. Given this, should we ignore all dotfiles (i.e. .hg) or other strategy?

Comment 10

[Jeff Hammel]

If (jetpack) developers are interested in using jetperf.py (etc) and find the existing workflow inadequate for their purposes, we should note these issues as they're probably easily solvable.

Comment 11

[Aki Sasaki (not active)]

Agreed. Just noting that it won't require a new script.

Comment 12

[Jeff Hammel]

Attachment: move addon gathering to its own method

This patch moves the addon gathering to its own method.  :aki, is this amenable to you?  I haven't addressed the multiple repos method yet

Comment 13

[Jeff Hammel]

Attachment: use config['repos'] and mercurialscript

I'm not really sure if this is what you want but its my best guess

Comment 14

[Jeff Hammel]

> Instead of having 2 different repo config options, go with a list of
> repos, like this:
> http://hg.mozilla.org/build/mozharness/file/636e0f7b6ab6/configs/single_local\
e/mozilla-aurora_android.py#l26
>
> To clone those, instead of calling MercurialVCS.clone(), we'd call
> self.vcs_checkout_repos(self.config['repos']) (after inheriting
> MercurialScript).

I admit that I'm a bit confused by this pattern, both conceptually and
implementation-wise.  Conceptually it seems like the configuration .py
file (in this case) supplies a list of repositories, their revision,
and where to put them (relative to the workdir for relative paths, I
would guess). What I don't understand (conceptually) is how
since the script (jetperf.py in this case) (seems to?) depends on the
location of the checked-out repository for its function
(e.g. http://hg.mozilla.org/build/mozharness/file/636e0f7b6ab6/scripts/jetperf.\
py#l88
), why this lives at the configuration level?

Implementation-wise and in combination, given the usual defaults of
'http://hg.mozilla.org/projects/addon-sdk and
http://hg.mozilla.org/projects/addon-sdk-jetperf-tests/ , is it
acceptable to set the default config['repos'] to what is currently in
jetperf.py if none is present in the configuration step?  Otherwise I
don't see how sane defaults can be kept.  I would like to avoid
breaking up jetperf.py into having to have a configuration file (which
we have no documentation on how to generate) and a script, both of
which must be read, especially for a workflow that I don't see what it
buys.

I've done as best I can with attachment 628928 [details] [diff] [review] . If that's not what you want, please let me know how to correct it

Comment 15

[Aki Sasaki (not active)]

Comment on attachment 628928 [details] [diff] [review]
use config['repos'] and mercurialscript

Nit: I found these when running pyflakes:

scripts/jetperf.py:14: 'BaseScript' imported but unused
scripts/jetperf.py:215: local variable 'args' is assigned to but never used

Could you clean those up?

>+    def addons_from_directory(self, directory):
>+        """scans a directory for jetpack addon sources and returns a list"""
>+
>+        retval = [os.path.join(directory, i) for i in os.listdir(directory)
>+                  if not i.startswith('.')] # ignore dotfiles
>+        retval = [addon for addon in retval
>+                  if os.path.isdir(addon)] # directories only
>+        return retval

Could you self.info("Scanning %s for addon directories" % directory) at the beginning and self.info("Found %s addon directories" % str(retval)) to avoid doing this silently?

Other than the above this patch looks great; r=me with those addressed.


Answering your questions below:

> What I don't understand (conceptually) is how
> since the script (jetperf.py in this case) (seems to?) depends on the
> location of the checked-out repository for its function
> (e.g.
> http://hg.mozilla.org/build/mozharness/file/636e0f7b6ab6/scripts/jetperf.\
> py#l88
> ), why this lives at the configuration level?

There's nothing forcing you to check out that specific repo or branch.
Someone developing new addons could point their config to a user repo or branch, or work entirely locally and skip the clobber/pull actions.

So conceptually, we require that the addon-sdk live somewhere on disk, and that self.addon_sdk points to it.  We don't, however, care if the jetperf.py script pulled that directory from source this specific test run (or ever), as long as it exists and was built before running the script.

To be more config-file friendly, we could do something like

    self.addon_sdk = self.config.get('addon_sdk_path', os.path.join(self.workdir, 'addon-sdk'))

to allow someone to point at any arbitrary addon sdk directory.

> Implementation-wise and in combination, given the usual defaults of
> 'http://hg.mozilla.org/projects/addon-sdk and
> http://hg.mozilla.org/projects/addon-sdk-jetperf-tests/ , is it
> acceptable to set the default config['repos'] to what is currently in
> jetperf.py if none is present in the configuration step?

I think it's a good idea to have the script user-friendly to developers out of the box if possible.

If they want to clone those two repos by default, then I don't mind specifying that in jetperf.py as the default self.config['repos'].  If we want to override that, we can do so with a config file.

>  Otherwise I
> don't see how sane defaults can be kept.  I would like to avoid
> breaking up jetperf.py into having to have a configuration file (which
> we have no documentation on how to generate) and a script, both of
> which must be read, especially for a workflow that I don't see what it
> buys.

Let me know what documentation would help.
The simplest explanation might be: "the config file can be a .py file with a dict named 'config' that gets read into self.config ."

The script already has dependencies outside of a single file.  I'm not certain why having a config file is harder than, say, inheritance of objects or usage of methods that are defined in external files and imported.  I think it's easier to read a simple config file than read an entire script, as well, but it appears you don't agree.

We may need config files to run jetperf.py in production, like we do talos.
However, I'm ok with orienting the script towards developers out of the box, as I mentioned above.  If we aren't planning on running jetperf.py in production, or if it happens to work out of the box across all platforms and branches without one, then we may be config-file free.

Comment 16

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/mozharness/rev/244adee59a23

Comment 17

[Jeff Hammel]

> To be more config-file friendly, we could do something like
> 
>     self.addon_sdk = self.config.get('addon_sdk_path', os.path.join(self.workdir, 'addon-sdk'))
> 
> to allow someone to point at any arbitrary addon sdk directory.

Follow-up ticketed at bug 760667 (but not essential for getting jetperf in production)