Closed
Bug 1460743
Opened 6 years ago
Closed 6 years ago
Add 'speedometer' benchmark to raptor for google chrome (OSX)
Categories
(Testing :: Raptor, enhancement)
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: rwood, Assigned: rwood)
References
Details
Attachments
(3 files)
No description provided.
Assignee | ||
Updated•6 years ago
|
Summary: Add google chrome 'speedometer' benchmark to raptor → Add 'speedometer' benchmark to raptor for google chrome
Assignee | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8983167 [details] Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome; https://reviewboard.mozilla.org/r/249024/#review255898 a few concerns here- maybe I need more understanding or context. ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:135 (Diff revision 4) > + def install_chrome(self): > + # temporary hack to install google chrome in production; just so we can continue raptor development > + if self.app != "chrome": > + self.info("Google Chrome is not required") > + return > + else: you do not need the 'else:' here since the if returns. ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:139 (Diff revision 4) > + return > + else: > + if self.config.get("run_local"): > + self.info("expecting Google Chrome to be pre-installed locally") > + return > + chrome_url = "https://dl.google.com/chrome/mac/stable/GGRO/googlechrome.dmg" it would be nice if this were defined in raptor, but lets not do it now; we might be getting updated chrome bits from another job as an artifact and then defining it inside raptor would be pointless ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:140 (Diff revision 4) > + else: > + if self.config.get("run_local"): > + self.info("expecting Google Chrome to be pre-installed locally") > + return > + chrome_url = "https://dl.google.com/chrome/mac/stable/GGRO/googlechrome.dmg" > + # self.chrome_dest = os.path.join(self.query_abs_dirs()['abs_work_dir'], 'chrome') getrid of this comment ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:142 (Diff revision 4) > + self.info("expecting Google Chrome to be pre-installed locally") > + return > + chrome_url = "https://dl.google.com/chrome/mac/stable/GGRO/googlechrome.dmg" > + # self.chrome_dest = os.path.join(self.query_abs_dirs()['abs_work_dir'], 'chrome') > + self.chrome_dest = os.path.join(self.obj_path, 'chrome') > + chrome_dmg = os.path.join(self.chrome_dest, 'googlechrome.dmg') this is specific to OSX- can you prepare this code for windows/linux at least? ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:193 (Diff revision 4) > + binary_arg_provided = False > + for next_rap_arg in self.config['raptor_extra_options']: > + if "--app" in next_rap_arg and \ > + "firefox" not in next_rap_arg and \ > + "firefox" not in self.config['raptor_extra_options']: > + on_firefox = False this logic looks hard to maintain and maybe unnecessary. Reading the code and the comment I am still not sure why we do things this way. Is it not as easy as: self.config.get('app') and self.config.get('binary') ? ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:258 (Diff revision 4) > self.fatal("Test name not provided") > + if '--app' in self.config['raptor_extra_options']: > + # --app specified, get app (firefox or chrome) from cmd line > + app_name_index = self.config['raptor_extra_options'].index('--app') + 1 > + if app_name_index < len(self.config['raptor_extra_options']): > + self.app = self.config['raptor_extra_options'][app_name_index] why would --app be passed inside of raptor_extra_options? I thought it would be a regular config item such as |self.get['app']| ? ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:367 (Diff revision 4) > """run raptor tests""" > > # get raptor options > options = self.raptor_options(args=args, **kw) > + if options == []: > + self.error("abort: invalid raptor options") this error is misleading as there are no options. Why would we get into this case? Is this related to google chrome? ::: testing/raptor/raptor/raptor.py:186 (Diff revision 4) > def main(args=sys.argv[1:]): > args = parse_args() > commandline.setup_logging('raptor', args, {'tbpl': sys.stdout}) > LOG = get_default_logger(component='raptor-main') > > + LOG.info("received args: %s" % str(args)) if we keep this, maybe it have a better description of what the line means.
Attachment #8983167 -
Flags: review?(jmaher) → review-
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8983871 [details] Bug 1460743 - TC configs for raptor speedometer on google chrome; https://reviewboard.mozilla.org/r/249722/#review255910
Attachment #8983871 -
Flags: review?(jmaher) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8983167 [details] Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome; https://reviewboard.mozilla.org/r/249024/#review255898 Thanks! > it would be nice if this were defined in raptor, but lets not do it now; we might be getting updated chrome bits from another job as an artifact and then defining it inside raptor would be pointless Ah, right, yeah this is probably just going to be temporary. > this is specific to OSX- can you prepare this code for windows/linux at least? I'd prefer to land this for OSX first if that's ok, I'm going to need to get windows and linux loaners in order to port to those platforms, which is def my next priority. > this logic looks hard to maintain and maybe unnecessary. Reading the code and the comment I am still not sure why we do things this way. Is it not as easy as: > self.config.get('app') and self.config.get('binary') ? No, it's because when running in production vs locally via mach. And when running locally we want the option of just running: ./mach raptor-test --test raptor-speedometer Which defaults to firefox and grabs the binary from your local build. However locally someone might also do: ./mach raptor-test --test raptor-speedometer --app firefox Also when running on Chrome we can't grab the binary from the local build or mozharness/production build, so that's why the extra bits. It's defintely not ideal but maybe once we support Google Chrome in CI then I'll be able to grab the Chrome binary location etc. right from mozharness too. > why would --app be passed inside of raptor_extra_options? I thought it would be a regular config item such as |self.get['app']| ? Just the method that mach (locally) and mozharness use to build a command line that mozharness uses later to invoke raptor. Once inside raptor it's just self.app etc. It's because of having to grab command line opts both locally and in production, and wanting to be able to default to firefox but also be able to specifiy a different browwer completely etc is why this code is not fun. I'll see if I can improve it a bit thanks. > this error is misleading as there are no options. Why would we get into this case? Is this related to google chrome? Yes for example if someone on the command line locally tries to run: ./mach raptor-test --test raptor-speedometer --app chrome But they don't provide the '--binary' argument. A more specific error does show up earlier saying that binary argument is required - this is just after returning from parsing out the options, double checking they exist, and if not erroring out completely. I can just remove that message completely or change it, or have the raptor_options method return an error message maybe. I'll change it thanks.
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b131719a3ab87c250bf54575ba91cd146f87f9b9
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6914c7a7023ab4c7ad715960084ecc3d6d7a9e41
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8983167 [details] Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome; https://reviewboard.mozilla.org/r/249024/#review256152 just some nits remain ::: commit-message-6d741:1 (Diff revisions 4 - 5) > -Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome; r?jmaher > +Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome (wip) you might want to remove the (wip) from the commit message ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:109 (Diff revisions 4 - 5) > self.mitmdump = None # path to mitmdump tool itself, in py3 venv > self.app = self.config.get("app", "firefox") > > + # temp debugging > + self.info("* RW * mozharness/testing/raptor.py/init") > + self.info("self.config: %s" + str(self.config)) do we want this to land? the comment says temp, but if this lands, please change the RW to something more generic :) ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:357 (Diff revisions 4 - 5) > - return > > + # temp debugging > + self.info("in run tests, options are:") > + self.info(str(options)) > + os.sys.exit() ack, more temp debugging :)
Attachment #8983167 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #18) > Comment on attachment 8983167 [details] > Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome (wip) > > https://reviewboard.mozilla.org/r/249024/#review256152 > > just some nits remain > > ::: commit-message-6d741:1 > (Diff revisions 4 - 5) > > -Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome; r?jmaher > > +Bug 1460743 - Add 'speedometer' benchmark to raptor for google chrome (wip) > > you might want to remove the (wip) from the commit message > > ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:109 > (Diff revisions 4 - 5) > > self.mitmdump = None # path to mitmdump tool itself, in py3 venv > > self.app = self.config.get("app", "firefox") > > > > + # temp debugging > > + self.info("* RW * mozharness/testing/raptor.py/init") > > + self.info("self.config: %s" + str(self.config)) > > do we want this to land? the comment says temp, but if this lands, please > change the RW to something more generic :) > > ::: testing/mozharness/mozharness/mozilla/testing/raptor.py:357 > (Diff revisions 4 - 5) > > - return > > > > + # temp debugging > > + self.info("in run tests, options are:") > > + self.info(str(options)) > > + os.sys.exit() > > ack, more temp debugging :) Hey - sorry it wasn't actually ready to be reviewed again yet - maybe mozreview reset the flag automatically (that's why I changed the r? to 'wip' haha). Anyway thanks for the feedback - I'll put it up for review again if you don't mind, once I have the final version ready.
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8983883 [details] Bug 1460743 - Don't install testing/profiles on Google Chrome; https://reviewboard.mozilla.org/r/249728/#review256198 ::: testing/mozbase/mozprofile/mozprofile/profile.py:499 (Diff revision 4) > addons = [addons] > self.extend(addons) > > @classmethod > def is_addon(self, addon): > # TODO Implement this properly I think you can remove this TODO now.
Attachment #8983883 -
Flags: review?(ahal) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6bc99f040ed62499b4134ba90c79a4b5428a89c
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f2d3dfadf200669bdb23ae5c73ba843e7f69301
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 36•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aec357e1261da68efad2c2f30876296e544cfe28
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a992f7a0d7f113a758a1f13dcf517571690bc668
Assignee | ||
Updated•6 years ago
|
Summary: Add 'speedometer' benchmark to raptor for google chrome → Add 'speedometer' benchmark to raptor for google chrome (OSX)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 47•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbfed779f7d89ee3a0ee18137157f8b10febb9c1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d252ac7bc6122812768a5d4ddf114169530b4cf
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 63•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f2e9b644487d1f0c7db1585f5cea894d1d90eff
Comment 64•6 years ago
|
||
Pushed by rwood@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0fd1e635f181 Add 'speedometer' benchmark to raptor for google chrome; r=jmaher https://hg.mozilla.org/integration/autoland/rev/daaa667524c7 TC configs for raptor speedometer on google chrome; r=jmaher https://hg.mozilla.org/integration/autoland/rev/6e95ded62596 Don't install testing/profiles on Google Chrome; r=ahal
Comment 65•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0fd1e635f181 https://hg.mozilla.org/mozilla-central/rev/daaa667524c7 https://hg.mozilla.org/mozilla-central/rev/6e95ded62596
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•