1226381 - Marionette should print real path for profile location

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As of now Marionette only logs the following line for the profile location:

 0:00.00 LOG: MainThread INFO Profile destination is TMP

It's not pretty helpful if you want to know the exact location of the profile.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The offending log message is here: https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/testing/marionette/client/marionette/runner/base.py#625

The temporary directory for the profile is created somewhere in here, so that's a better place for the log message: https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/testing/marionette/driver/marionette_driver/geckoinstance.py#97-113

Comment 2

[Tyler Steiman]

I would love to work on this bug if I could get some more information in terms of what needs to be done. Does the log message not need to be in base.py at all, it should just get moved? Any help would be great!

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Great. :)

The log message should get moved and it should display the location of the profile, e.g. "Profile path is /some/absolute/path"

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Actually, let me correct myself: you don't need to move the log message out of base.py. All we care about is that the message displays the path to the profile.

Comment 5

[Tyler Steiman]

Sounds good, and what would be a good way to test for this / replicate the problem?

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Run some tests (any tests) and observe the log messages at the beginning of the test run.

Some examples of running tests:
* Calling runtests.py [1] directly: python runtests.py --binary path/to/firefox/binary ./tests/unit/test_click.py
* Calling runtests.py via mach: ./mach marionette-test browser/components/loop/manifest.ini 
** see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/mach#Running

If you run into any issues or need help with configuration, join #ateam on irc.mozilla.org and ping me (maja_zf) or whimboo.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runtests.py

Comment 7

[Tyler Steiman]

I'm just having trouble finding the best way to get the created profile path from geckoinstance.py and print it out in base.py

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I would figure that out by back-tracking from geckoinstance.py to figure out where it's used in base.py as follows: 
* GeckoInstance.start() [1] is called by the __init__ in Marionette [2]. 
* The __init__ of Marionnette is in turn called in BaseMarionetteTestRunner.run_tests() [3]. At that point, we have a properly initialized Marionette instance and we should be able to look inside it to get the profile path it's using.


[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/geckoinstance.py#81
[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#600
[3] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#809

Comment 9

[Akshat Jain]

I am interested in working on this bug. Can you please assign it to me !!!

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Akshat. We won't assign the bug right away. For new contributors we usually wait for their first patch. Given that we haven't heard from Tyler for about a month I believe that he won't be able to work on that bug. So feel free to propose a solution using the instructions as given above. Thanks.

Comment 11

[Aditya Motwani (:AdityaM)]

Hi I would like to work on this bug.

I am still unclear on how to proceed and get the path of a profile.

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Hi Aditya, thanks for taking a look at the bug. Could you describe to me specifically what you've tried so far or where you are stuck? Have you obtained a copy of the code? Have you been able to run the test-suite with './mach marionette-tests' or by running testing/marionette/client/marionette/runtests.py?

In case this is your first bug or if you're having trouble getting started, I suggest you follow this introduction first: http://areweeveryoneyet.org/onramp/ Then feel free to ask for help on IRC in #introduction or #ateam, or you can comment on this bug. 

I will be back online on Dec 29th. 
Good luck!

Comment 13

[Aditya Motwani (:AdityaM)]

Hey I am stuck on getting the build of firefox-ui-tests. When I run any command the command "firefox-ui-tests --help" I get the following error.

ImportError: 'module' object has no attribute 'cli'

It started occurring when I updated my repo to the latest one and did not happen on an older build.

How do I fix this problem?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Aditya, not sure why you are trying to run the firefox-ui-tests command. This bug is about an issue in Marionette base and it uses its own command line interface script named `marionette`. That is the one you should also use here to get your patch tested.

Comment 15

[Aditya Motwani (:AdityaM)]

Attachment: Bug1226381.patch

Submitted Patch for Bug. Please Review and provide feedback! :)

Comment 16

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8704133 [details] [diff] [review]
Bug1226381.patch

Review of attachment 8704133 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Aditya, thanks, this is a good start and it works well for testing a Firefox desktop instance. However, one can also run Marionette-based tests against a device emulator (for Firefox OS), which doesn't use self.instance. In other words, when someone runs tests using an emulator instead of a Firefox binary, the harness will encounter an AttributeError because self.marionette.instance is None.

The emulator is represented with self.marionette.runner. Since we need to check for `instance` versus `runner` in Marionette, how about you add a property [1] called 'profile_path' to the Marionette class in marionette.py? It should return the profile path via the instance if it's available, or via the runner if that's available. Then your log message could just refer to self.marionette.profile_path.

[1] I'm not familiar with your Python background so here's a reference to `@property` just in case - https://docs.python.org/2/library/functions.html#property

::: testing/marionette/client/marionette/runner/base.py
@@ +811,5 @@
>              # if we're working against a desktop version, we usually don't need
>              # an external ip
>              if self._capabilities['device'] == "desktop":
>                  need_external_ip = False
> +            self.logger.info('Profile Destination is ' + str(self.marionette.instance.profile.profile))

This is good, but please move it outside of the `if not self.marionette` block (i.e. decrease the indentation level) because we want to log the profile location even if the Marionette instance has previously been initialized.

Comment 17

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I forgot to include this earlier: please change the log message to "Initial profile destination..." since a test suite might generate many profiles.

Comment 18

[Aditya Motwani (:AdityaM)]

Right then, I get cracking on this and shall submit a patch soonish. :)

Comment 19

[Aditya Motwani (:AdityaM)]

Attachment: bug1226381_1.patch

Added changes as proposed. Please provide feedback and review.

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8705723 [details] [diff] [review]
bug1226381_1.patch

Review of attachment 8705723 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. :) A couple more changes and it then will be ready for a try run and final review by :AutomatedTester.

::: testing/marionette/driver/marionette_driver/marionette.py
@@ +635,5 @@
>  
>          # for callbacks from a protocol level 2 or lower remote,
>          # we store the callback ID so it can be used by _send_emulator_result
>          self.emulator_callback_id = None
> +    @property

Style: add an empty line above @property

@@ +637,5 @@
>          # we store the callback ID so it can be used by _send_emulator_result
>          self.emulator_callback_id = None
> +    @property
> +    def profile_path(self):
> +        if self.instance is None:

This is the right idea but let's be careful: any of self.runner or self.instance or self.runner.profile/self.instance.profile may be None, so I think you need two cases of the form "if self.x and self.x.profile: return self.x.profile.profile".

Comment 21

[Aditya Motwani (:AdityaM)]

Attachment: bug1226381_2.patch

Added Changes. Please review.

Comment 22

[David Burns :automatedtester]

Comment on attachment 8709587 [details] [diff] [review]
bug1226381_2.patch

Review of attachment 8709587 [details] [diff] [review]:
-----------------------------------------------------------------

one minor change that needs to be done.

For the next review, can you push to MozReview. http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

::: testing/marionette/client/marionette/runner/base.py
@@ +811,5 @@
>              # if we're working against a desktop version, we usually don't need
>              # an external ip
>              if self._capabilities['device'] == "desktop":
>                  need_external_ip = False
> +        self.logger.info('Initial Profile Destination is ' + str(self.marionette.profile_path))

Please use `.format` instead of concat'ing strings

Comment 23

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Hey Aditya, I also pushed your latest patch to the 'try server' on your behalf to test it out. Results are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7cffdf14862

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

And which looks fantasic. See this line of the test output:
https://treeherder.mozilla.org/logviewer.html#?job_id=15638451&repo=try#L982

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alex, do you have an update for us here? It would be nice to see your patch finally landed. Looks like only some minor updates are necessary.

Comment 26

[Aditya Motwani (:AdityaM)]

Attachment: MozReview Request: Bug 1226381 - Marionette should print real path for profile location; r?automatedtester,maja_zf

Review commit: https://reviewboard.mozilla.org/r/37373/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37373/

Comment 27

[Aditya Motwani (:AdityaM)]

Comment on attachment 8725231 [details]
MozReview Request: Bug 1226381 - Marionette should print real path for profile location; r?automatedtester,maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37373/diff/1-2/

Comment 28

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8725231 [details]
MozReview Request: Bug 1226381 - Marionette should print real path for profile location; r?automatedtester,maja_zf

https://reviewboard.mozilla.org/r/37373/#review33961

lgtm

Comment 29

[David Burns :automatedtester]

Comment on attachment 8725231 [details]
MozReview Request: Bug 1226381 - Marionette should print real path for profile location; r?automatedtester,maja_zf

https://reviewboard.mozilla.org/r/37373/#review34373

Comment 30

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Aditya, at this point, when your patch has r+, tests have passed and you think it's ready to land, you can add 'checkin-needed' keyword to the bug so a sheriff can land the patch for you on mozilla-inbound. I'll add the keyword now on your behalf.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f2d8d5d1b5a

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f2d8d5d1b5a

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Moving bugs for Marionette client due to component changes.