Closed Bug 1318644 Opened 4 years ago Closed 4 years ago

Auto-detect application type if binary has been specified

Categories

(Testing :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

If you run Marionette with the --binary argument but don't specify --app, we should automatically detect the application. This can be done via mozversion and the specified binary.
Attachment #8812211 - Flags: review?(dburns)
Please keep in mind that as of right now we run all of our unit tests with the default GeckoInstance for Firefox! With this fix in place we will indeed disable safebrowing, application updates, and others.
Comment on attachment 8812211 [details]
Bug 1318644 - Auto-detect application type if binary has been specified.

https://reviewboard.mozilla.org/r/94038/#review94500

::: testing/marionette/client/marionette_driver/marionette.py
(Diff revision 1)
> -                                         'application.ini'))
> -                app = config.get('App', 'Name')
> -                instance_class = geckoinstance.apps[app.lower()]
> -            except (ConfigParser.NoOptionError,
> -                    ConfigParser.NoSectionError,
> -                    KeyError):

David, if you want to keep this logic here please let me know. Then we will have to add mozversion as a new dependency to marionette_client.
Comment on attachment 8812211 [details]
Bug 1318644 - Auto-detect application type if binary has been specified.

https://reviewboard.mozilla.org/r/94038/#review94500

> David, if you want to keep this logic here please let me know. Then we will have to add mozversion as a new dependency to marionette_client.

we don't need to keep it
Comment on attachment 8812211 [details]
Bug 1318644 - Auto-detect application type if binary has been specified.

https://reviewboard.mozilla.org/r/94038/#review94550

::: testing/marionette/harness/marionette/runner/base.py:437
(Diff revision 1)
>  
>          if not args.address and not args.binary and not args.emulator:
>              self.error('You must specify --binary, or --address, or --emulator')
>  
> +        if not os.path.isfile(args.binary):
> +            self.error('You must specify an existent binary.')

s/existent/existing/
Attachment #8812211 - Flags: review?(dburns) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93e4c5f0dc7b
Auto-detect application type if binary has been specified. r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/93e4c5f0dc7b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please uplift this test-only patch to aurora so it will be present in the next ESR release. Thanks.
Whiteboard: [checkin-needed-aurora]
As noticed on bug 1319731 the code should have been left in geckodriver. So removing uplift request for now and asking for a backout.
Whiteboard: [checkin-needed-aurora]
Per your request.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0568a85aa40bd2a2a43c8c813c059451f3d328
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
Thank you Ryan. 

So why this backout was necessary:

1. There wouldn't have been a way to correctly run Marionette for Firefox with the correct Instance type like in the following snippet:

>>> from marionette_driver.marionette import Marionette
>>> m = Marionette(bin="/home/foobar/firefox-50/firefox")
>>> m.start_session()

That is because the auto-detection code has been moved to the harness, but we indeed have to keep it in the driver. Please see comment 4 where I have asked about the location because I wasn't sure. Now it's clear why it has to be kept in the driver.

I will adjust the patch by moving the code block back to the driver and introduce a new dependency to mozversion.
Comment on attachment 8812211 [details]
Bug 1318644 - Auto-detect application type if binary has been specified.

David, this patch makes sure to not move any gecko instance detection code even further away. Now the code is even part of the GeckoInstance base class where I think is indeed the best place, and accessible via a class method. 

The code move also allows us to actually test this code which was not possible before because for each invocation a new application instance would have been launched.
Attachment #8812211 - Flags: review?(dburns)
Comment on attachment 8812211 [details]
Bug 1318644 - Auto-detect application type if binary has been specified.

https://reviewboard.mozilla.org/r/94038/#review95518

::: testing/marionette/client/marionette_driver/geckoinstance.py:139
(Diff revisions 2 - 3)
>                      profile_args["path_to"] = os.path.join(self.workspace,
>                                                             profile_name)
>                  self.profile = Profile.clone(**profile_args)
>  
> +    @classmethod
> +    def create_instance(cls, app=None, *args, **kwargs):

Better API name would be just `create` since this is going to be "public".
Attachment #8812211 - Flags: review?(dburns) → review+
Comment on attachment 8812211 [details]
Bug 1318644 - Auto-detect application type if binary has been specified.

https://reviewboard.mozilla.org/r/94038/#review95518

> Better API name would be just `create` since this is going to be "public".

Good feedback. I have made this change.
https://hg.mozilla.org/mozilla-central/rev/ef0568a85aa4
Whiteboard: backout also from m-c
I had to rebase a couple of times due to latest changes made by other patches for Marionette. Hopefully it can be landed now.
Whiteboard: backout also from m-c
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c41dab3b9a72
Auto-detect application type if binary has been specified. r=automatedtester
https://hg.mozilla.org/mozilla-central/rev/c41dab3b9a72
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Test-only change which we want to have on aurora too. Thanks
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.