720436 - Generalize the firefox fetching pattern from the mozharness peptest.py script

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P4)

Description

[Jeff Hammel]

the peptest mozharness script features a pattern for fetching firefox for testing:

http://hg.mozilla.org/build/mozharness/file/ac9bf9986aaa/scripts/peptest.py#l57
http://hg.mozilla.org/build/mozharness/file/ac9bf9986aaa/scripts/peptest.py#l256
http://hg.mozilla.org/build/mozharness/file/ac9bf9986aaa/scripts/peptest.py#l307

JetPerf (https://wiki.mozilla.org/Auto-tools/Projects/JetPerf) will want to use a similar pattern and probably want this.  We should generalize this pattern to some sort of base class for this type of harness.

FWIW, I don't really like 'appname' as the name here.  'app' would be fine.  Any thoughts?

Comment 1

[Aki Sasaki (not active)]

As mentioned in IRC:

The script uses appname as both install location and installer location, which I don't like.  We should have an app_path and an installer_url and explicitly install/don't-install depending on config, rather than guessing what the person wants to have happen.

Comment 2

[Jeff Hammel]

see also https://bugzilla.mozilla.org/show_bug.cgi?id=738478#c8

Comment 3

[Jeff Hammel]

So, opposite of comment 1 , I don't like having a separate option for --path-to-browser-executable and --path-to-browser-url . You will only ever want to use one of these per run and using both will cause a conflict that would have to be checked for.  IMHO, it is not "guessing" or otherwise implicit behaviour since in one case you will have a url (http://, https://, or ftp://) and in the other case you will have a file path (or, if we want to be crazy, we could support file:// as well).  

That said, as usual, if someone else wants to do the work I'm happy with the option in comment 1.  I am happy to do the work but I don't really want to be in the game of guessing what is wanted and iterating as I've found this to vastly increase turnaround time.

There is also the issue of get-latest-tinderbox which peptest currently uses.  Do we want to support that?  Or do we want to file a bug to deprecate that too?

Comment 4

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #3)
> There is also the issue of get-latest-tinderbox which peptest currently
> uses.  Do we want to support that?  Or do we want to file a bug to deprecate
> that too?

I haven't been able to get it to work at all, so it won't go in the shared module.
I don't really have a strong opinion whether it should be removed or just live in peptest.py.

Comment 5

[Aki Sasaki (not active)]

Attachment: wip

This patch:

* depends on attachment 613013 [details] [diff] [review] in bug 697462.  The config files and script are edited to match.
* refactors peptest's action list and order to generalize the actions.

The previous order was

    clobber
    create-virtualenv
    read-buildbot-config
    get-latest-tinderbox
    create-deps
    run-peptest

create-virtualenv created a bare-bones virtualenv.  get-latest-tinderbox installed its own module if called, and create-deps installed the various mozbase packages downloaded and extracted earlier in create-deps.

The install happened in run-peptest, the hg repo pull happened inside create-deps.  read-buildbot-configs and get-latest-tinderbox were two optional steps that helped figure out where to download the installer and test zip from.

The current order is

    clobber
    pull
    read-buildbot-configs
    download-and-extract
    create-virtualenv
    install
    run-peptest

Having pull be its own action feels right.  I need to download-and-extract the test zip before I can create the virtualenv, since mozbase is in the test zip.  However, without the virtualenv, I can't install get-latest-tinderbox, which means there's a circular dependency.  Rather than bend over backwards to support something I've never gotten to work, I removed that action.

I think this patch generalizes the actions to where they're useful for all test types.  I successfully ran peptest via this script on fedora 32; next steps are moving code to a module in mozharness.mozilla.* (mozharness.mozilla.buildbot? mozharness.mozilla.testing?) and test across a few more platforms.

Comment 6

[Aki Sasaki (not active)]

Attachment: corresponding peptest patch for attachment 613395

Recorded for posterity before I start moving large chunks of peptest.py under mozharness/mozilla/.

Comment 7

[Aki Sasaki (not active)]

Comment on attachment 613400 [details] [diff] [review]
corresponding peptest patch for attachment 613395 [details] [diff] [review]

This patch:

* removes virtualenv_modules from configs/peptest/*
* updates default_actions to match the new peptest actions
* removes get-latest-tinderbox
* hardcodes test_install_dir to tests/ and application_install_dir to application/.  This removes some configurability, but enables us to know where the python package directories are going to live in advance without a lot of %(variable)s substitution ugliness.

* changes --appname to --installer-url, --installer-path, and --binary-path.

Previously, if we reran the run-peptest action over and over, we would have reinstalled the app over and over (if it was a url).  Now we're a lot more developer-on-plane friendly.  I'm not particularly happy about having to define all 3 to allow for running any subset of actions easily, but that can be done as a one-time hit in a config file.

* updates virtualenv_modules to use the new functionality from bug 697462.
* rearranges the workflow to match the new create_virtualenv() (comment 5)

I'd prefer to land this and the patch in bug 697462 before refactoring to mozharness.mozilla.*, so passing to you for r?.  Thanks, and let me know if you have any questions.

Comment 8

[Jeff Hammel]

Comment on attachment 613400 [details] [diff] [review]
corresponding peptest patch for attachment 613395 [details] [diff] [review]

works for me

Comment 9

[Aki Sasaki (not active)]

Comment on attachment 613400 [details] [diff] [review]
corresponding peptest patch for attachment 613395 [details] [diff] [review]

http://hg.mozilla.org/build/mozharness/rev/124cbc07bc6a

Comment 10

[Aki Sasaki (not active)]

Comment on attachment 613400 [details] [diff] [review]
corresponding peptest patch for attachment 613395 [details] [diff] [review]

I think we need to support setup.py to not burn on windows.

Comment 11

[Aki Sasaki (not active)]

Attachment: os.path.join for virtualenv_modules paths

Interdiff:

(bb08)deathduck:~/src/firefox-fetch/mozharness [13:55:31] (default)
755$ diff ~/Desktop/peptest{4,5}.diff
2c2
< # Parent e0d3eaf8d02693477ec8a7894bdadf143d7ed814
---
> # Parent ade1dfda1936ca803f17957416b57cf751dbcebc
327,335c327,335
< +        {'mozlog': 'tests/mozbase/mozlog'},
< +        {'mozinfo': 'tests/mozbase/mozinfo'},
< +        {'mozhttpd': 'tests/mozbase/mozhttpd'},
< +        {'mozinstall': 'tests/mozbase/mozinstall'},
< +        {'manifestdestiny': 'tests/mozbase/manifestdestiny'},
< +        {'mozprofile': 'tests/mozbase/mozprofile'},
< +        {'mozprocess': 'tests/mozbase/mozprocess'},
< +        {'mozrunner': 'tests/mozbase/mozrunner'},
< +        {'peptest': 'tests/peptest'},
---
> +        {'mozlog': os.path.join('tests', 'mozbase', 'mozlog')},
> +        {'mozinfo': os.path.join('tests', 'mozbase', 'mozinfo')},
> +        {'mozhttpd': os.path.join('tests', 'mozbase', 'mozhttpd')},
> +        {'mozinstall': os.path.join('tests', 'mozbase', 'mozinstall')},
> +        {'manifestdestiny': os.path.join('tests', 'mozbase', 'manifestdestiny')},
> +        {'mozprofile': os.path.join('tests', 'mozbase', 'mozprofile')},
> +        {'mozprocess': os.path.join('tests', 'mozbase', 'mozprocess')},
> +        {'mozrunner': os.path.join('tests', 'mozbase', 'mozrunner')},
> +        {'peptest': os.path.join('tests', 'peptest')},

Comment 12

[Jeff Hammel]

Comment on attachment 613753 [details] [diff] [review]
os.path.join for virtualenv_modules paths

wfm

Comment 13

[Aki Sasaki (not active)]

Comment on attachment 613753 [details] [diff] [review]
os.path.join for virtualenv_modules paths

http://hg.mozilla.org/build/mozharness/rev/14f030069dce

Comment 14

[Jeff Hammel]

So is the next step to move this to a mixin?

Comment 15

[Aki Sasaki (not active)]

Yes, hope to have that in a patch by sometime tomorrow.

Comment 16

[Aki Sasaki (not active)]

Attachment: add mpl 2.0 to mozharness.mozilla.talos

Comment 17

[Aki Sasaki (not active)]

Attachment: create TestingMixin from peptest.py

Comment 18

[Jeff Hammel]

Comment on attachment 613842 [details] [diff] [review]
add mpl 2.0 to mozharness.mozilla.talos

looks good to me

Comment 19

[Jeff Hammel]

Comment on attachment 613843 [details] [diff] [review]
create TestingMixin from peptest.py

+testing_virtualenv_modules = [
+    'simplejson',
+    {'mozlog': os.path.join('tests', 'mozbase', 'mozlog')},
+    {'mozinfo': os.path.join('tests', 'mozbase', 'mozinfo')},
+    {'mozhttpd': os.path.join('tests', 'mozbase', 'mozhttpd')},
+    {'mozinstall': os.path.join('tests', 'mozbase', 'mozinstall')},
+    {'manifestdestiny': os.path.join('tests', 'mozbase', 'manifestdestiny')},
+    {'mozprofile': os.path.join('tests', 'mozbase', 'mozprofile')},
+    {'mozprocess': os.path.join('tests', 'mozbase', 'mozprocess')},
+    {'mozrunner': os.path.join('tests', 'mozbase', 'mozrunner')},
+]
 
Do we really want these in the mixin class?  It seems like the wrong place for them

Comment 20

[Aki Sasaki (not active)]

Attachment: move the virtualenv_modules definition back to peptest.py

Comment 21

[Aki Sasaki (not active)]

Comment on attachment 613842 [details] [diff] [review]
add mpl 2.0 to mozharness.mozilla.talos

http://hg.mozilla.org/build/mozharness/rev/e1072ec48fae

Comment 22

[Jeff Hammel]

Comment on attachment 614061 [details] [diff] [review]
move the virtualenv_modules definition back to peptest.py

looks good

Comment 23

[Aki Sasaki (not active)]

Comment on attachment 614061 [details] [diff] [review]
move the virtualenv_modules definition back to peptest.py

http://hg.mozilla.org/build/mozharness/rev/3e138d932977