Closed Bug 1460743 Opened 2 years ago Closed 2 years ago

Add 'speedometer' benchmark to raptor for google chrome (OSX)

Categories

(Testing :: Raptor, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: rwood, Assigned: rwood)

References

Details

Attachments

(3 files)

No description provided.
Summary: Add google chrome 'speedometer' benchmark to raptor → Add 'speedometer' benchmark to raptor for google chrome
Assignee: nobody → rwood
Status: NEW → ASSIGNED
Depends on: 1460741
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 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 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.
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+
(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 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+
Summary: Add 'speedometer' benchmark to raptor for google chrome → Add 'speedometer' benchmark to raptor for google chrome (OSX)
Blocks: 1467823
No longer blocks: 1467823
Blocks: 1467824
Blocks: 1467823
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
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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.