Closed Bug 1226381 Opened 4 years ago Closed 4 years ago

Marionette should print real path for profile location

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: whimboo, Assigned: AdityaM, Mentored)

Details

(Keywords: pi-marionette-client, Whiteboard: [good first bug][lang=Python])

Attachments

(1 file, 3 obsolete files)

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.
Mentor: mjzffr, hskupin
Whiteboard: [good first bug][lang=Python]
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!
Flags: needinfo?(mjzffr)
Great. :)

The log message should get moved and it should display the location of the profile, e.g. "Profile path is /some/absolute/path"
Flags: needinfo?(mjzffr)
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.
Sounds good, and what would be a good way to test for this / replicate the problem?
Flags: needinfo?(mjzffr)
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
Flags: needinfo?(mjzffr)
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
Flags: needinfo?(mjzffr)
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
Flags: needinfo?(mjzffr)
I am interested in working on this bug. Can you please assign it to me !!!
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.
Hi I would like to work on this bug.

I am still unclear on how to proceed and get the path of a profile.
Flags: needinfo?(mjzffr)
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!
Flags: needinfo?(mjzffr)
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?
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
Attached patch Bug1226381.patch (obsolete) — Splinter Review
Submitted Patch for Bug. Please Review and provide feedback! :)
Attachment #8704133 - Flags: review?(mjzffr)
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.
Attachment #8704133 - Flags: review?(mjzffr) → feedback-
Assignee: nobody → adityamotwani
I forgot to include this earlier: please change the log message to "Initial profile destination..." since a test suite might generate many profiles.
Right then, I get cracking on this and shall submit a patch soonish. :)
Attached patch bug1226381_1.patch (obsolete) — Splinter Review
Added changes as proposed. Please provide feedback and review.
Attachment #8705723 - Flags: review?(mjzffr)
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".
Attachment #8705723 - Flags: review?(mjzffr) → feedback-
Attached patch bug1226381_2.patch (obsolete) — Splinter Review
Added Changes. Please review.
Attachment #8704133 - Attachment is obsolete: true
Attachment #8705723 - Attachment is obsolete: true
Attachment #8709587 - Flags: review?(mjzffr)
Attachment #8709587 - Flags: review?(mjzffr) → review?(dburns)
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
Attachment #8709587 - Flags: review?(dburns) → review-
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
And which looks fantasic. See this line of the test output:
https://treeherder.mozilla.org/logviewer.html#?job_id=15638451&repo=try#L982
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.
Flags: needinfo?(adityamotwani)
Flags: needinfo?(adityamotwani)
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 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
Attachment #8725231 - Flags: review?(mjzffr) → review+
Attachment #8725231 - Flags: review?(dburns) → review+
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
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f2d8d5d1b5a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.