Closed Bug 873035 Opened 11 years ago Closed 10 years ago

Marionette should have a CLI for stdout log

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: snorp, Assigned: parkouss)

Details

(Whiteboard: [status-b2g-v1.3:fixed][status-b2g-v1.3t:fixed][lang=py][mentor=mdas][runner])

Attachments

(3 files, 6 obsolete files)

Right now Marionette just sends stderr/stdout to /dev/null. It should at least put it in a log somewhere or have a cmdline option to change that behavior.
Apparently this stuff goes to gecko.log.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
It does, but we should probably add a CLI for this.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: Marionette should show stdout/stderr somewhere → Marionette should have a CLI for stdout log
mdas, if you don't feel like mentoring this, bounce it back to me
Whiteboard: [good first bug][lang=py][mentor=mdas]
I can mentor it but it's not clear what should go in this log. Will the new argument just determine where the stdout from running the tests will go? Or should it also include all stdout from any process marionette starts (ie: gecko.log output)?
So, we currently write all of Firefox's stdout to gecko.log (in the cwd) while running a Marionette test.  We should add a command-line argument to runtests.py that allows the user to specify the file this is written to, in case another filename or location is desired.
To add a CLI argument, you'll need to modify runtests.py. You can add an argument in class MarionetteTestOptions. The values gathered there go into the MarionetteTestRunner object, which figures out which tests to run and kicks them off.

The log in question that we're talking about is found in geckoinstance.py. This class is instantiated in the marionette.py file. What you'll need to do is accept a CLI argument, call it '--gecko-log' and have it accept a file path, and the default value will be None. You'll need to find a way to pass this value when instantiating GeckoInstance in marionette.py. Then, update GeckoInstance's start function (http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/geckoinstance.py#35) to use the file path passed in, OR if the path is None, then it will use the default path it currently uses.
I would like to take this bug as my first bug :)
ah, sorry there was someone in channel yesterday talking about taking this bug. If I see them in channel, I'll confirm with them that they're taking this bug and assign it to them. I recommend working on Bug 802742 instead, so that you two don't duplicate work:)
Assigning this to someone who asked on IRC.
Assignee: nobody → redhoodie18
Attached patch patch for bug 873035 (obsolete) — Splinter Review
Attachment #762523 - Flags: review?(mdas)
Comment on attachment 762523 [details] [diff] [review]
patch for bug 873035

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

Sorry for the late response, I've been ill.

This patch does not apply cleanly to mozilla-central. Can you please follow these steps to update your patch: https://developer.mozilla.org/en-US/docs/Marionette/Developer_setup#Once_you_have_a_patch_ready... ?

Also, please use spaces instead of tabs. I'll update the developer setup documentation with a small style guide.
Attachment #762523 - Flags: review?(mdas)
Comment on attachment 762523 [details] [diff] [review]
patch for bug 873035

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

Other than the tab and this comment, it looks good. I'll wait for the fixed, clean patch to test it out.

::: testing/marionette/client/marionette/geckoinstance.py
@@ +14,5 @@
>                        "marionette.defaultPrefs.port": 2828,
>                        "startup.homepage_welcome_url": "about:blank",
>                        "browser.warnOnQuit": False}
>  
> +    def __init__(self, host, port, bin, profile, gecko_log):

we don't want to make gecko_log a mandatory parameter, so you'll have to let it be set to None.
Attached patch fix for patch for bug 873035 (1) (obsolete) — Splinter Review
Sorry for the delay. I've been studying for my exams, so I didn't have a chance to fix it up.
Attachment #762523 - Attachment is obsolete: true
Attachment #767187 - Flags: review?(mdas)
Comment on attachment 767187 [details] [diff] [review]
fix for patch for bug 873035 (1)

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

This patch doesn't do as requested. No matter what input you give to --gecko_log, the gecko.log file still gets saved to the current directory (which is the default behaviour).

Please test your code before submission and if you have any questions, please don't hesitate to comment on the bug or ask in channel in #ateam.

::: testing/marionette/client/marionette/geckoinstance.py
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/
>  
>  import os
> +import sys

this can be removed since we won't use it

@@ +32,5 @@
>              profile_args["restore"] = False
>          else:
>              runner_class = CloneRunner
>              profile_args["path_from"] = profile_path
> +        gecko_log_path = sys.argv[1:]

This isn't used anywhere.

@@ +34,5 @@
>              runner_class = CloneRunner
>              profile_args["path_from"] = profile_path
> +        gecko_log_path = sys.argv[1:]
> +        if self.gecko_log:
> +           self.gecko_log = os.path.abspath('gecko.log')

Please look up what os.path.abspath('gecko.log') will do. This isn't what we want to do if self.gecko_log is defined.

By only allowing absolute paths, then if self.gecko_log is set, it will be the path we already want to use. You should just verify that the path given is to a directory that already exists.

If it isn't set, then we want to preserve the previous default behaviour, which is to create the gecko.log file in the current directory.

@@ +35,5 @@
>              profile_args["path_from"] = profile_path
> +        gecko_log_path = sys.argv[1:]
> +        if self.gecko_log:
> +           self.gecko_log = os.path.abspath('gecko.log')
> +        elif os.path.exists('gecko_log_path'):

Again, this is not what you want.

@@ +38,5 @@
> +           self.gecko_log = os.path.abspath('gecko.log')
> +        elif os.path.exists('gecko_log_path'):
> +           self.gecko_log = os.path.abspath('gecko_log_path')
> +        else:
> +           self.gecko_log = os.path.abspath('gecko.log')        

These are 3 space tabs, but we use 4.

Also, line 42 has additional whitespace following it, which needs to be removed.

::: testing/marionette/client/marionette/runtests.py
@@ +695,5 @@
>                          help='if a --timeout value is given, it will set the default page load timeout, search timeout and script timeout to the given value. If not passed in, it will use the default values of 30000ms for page load, 0ms for search timeout and 10000ms for script timeout')
> +        self.add_option('--gecko_log',
> +                        dest='gecko_log',
> +                        action='store',
> +                        help='specify where to store the log file, if not, use the default settings')

--gecko_log should be --gecko-log following the style of the other options.


You should add that this should be an absolute path to the log file location. To make this a bit more interesting, we can accept a path to a file, so this file will be our log, or we can accept a directory, and we can put the gecko.log file in that directory.

We should verify the user's input. If the path is to a directory, verify the directory exists. If the path is to a file, verify that the directory the file lives in exists. If it doesn't exist, throw a related error and quit. The check can be done in geckoinstance.py
Attachment #767187 - Flags: review?(mdas) → review-
Hey Malini,

I will take care of this bugs. 
I try to give you a submission this afternoon. I'll start from scratch, it will should be probably easiest :)
First submission.
The helping comment on the option will be probably not good :)
Attachment #802459 - Flags: review?(mdas)
Comment on attachment 802459 [details] [diff] [review]
First submission : add option on marionette CLI

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

Sorry for the delay, I was away on PTO, then a work week, then I had catching up to do:)

This patch looks good, I was wondering if you can add one more feature though. Instead of having the user give the full path to a file, can you add functionality so that if they give you an existing directory, you can create a gecko<randomnumber>.log file there, and then print the name of the log file created at the end of the test run? This will let you store multiple log files without the user describing the exact file they want.

I'll upload an unbit-rotted patch. There has been a lot of changes so far!:)
Attachment #802459 - Flags: review?(mdas)
mbu, you can use this patch instead of applying your current one to the tip of mozilla-central:)
Attachment #767187 - Attachment is obsolete: true
Attachment #802459 - Attachment is obsolete: true
Hey,

No worry :), I'm very busy with the birth of my little boy, so I'm quite a bit off theses days.
When you are talking about random number, we could create a file with a time stamp in the name; or do you prefer a real random number like that (gecko4535345.log) ?
(In reply to mathieu bultel:mbu from comment #19)
> Hey,
> 
> No worry :), I'm very busy with the birth of my little boy, so I'm quite a
> bit off theses days.
> When you are talking about random number, we could create a file with a time
> stamp in the name; or do you prefer a real random number like that
> (gecko4535345.log) ?

Oh no, sorry, a timestamp of some sort would work best, it would enforce uniqueness and also help you easily figure out when your test started failing.
(In reply to mathieu bultel:mbu from comment #19)
> Hey,
> 
> No worry :), I'm very busy with the birth of my little boy, so I'm quite a
> bit off theses days.

And congratulations! :D
hehe, yes thanks,

I try to figure out the issue this week !! :)
Hey,
I would like to take up this bug. Please guide me
hey,
I think it's not a good idea to take up this bug, I have a patch to land for this one. Take another one, ask some help on irc chan : ateam.
Attached patch bug-873035-fix.patch (obsolete) — Splinter Review
I have added a timestamp to gecko log file but I used time.time() function. So it's close to a random number. I can use another one, from datetime.datetime for example. I'm open to your suggestions :)
Attachment #8367188 - Flags: review?(mdas)
Comment on attachment 8367188 [details] [diff] [review]
bug-873035-fix.patch

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

Thanks for coming back to write the patch!

Unfortunately, we're missing some key functionality, ie: handling directories.

I think it would be best to do the following:
 -If I give you a directory, you then populate it with gecko-(timestamp).log logs. 
 -If I give you a file path, use that path
 -If I don't give any information, just keep the current gecko.log file (no timestamp).

::: testing/marionette/client/marionette/runner/base.py
@@ +443,5 @@
>                          help='run tests in a random order')
> +        self.add_option('--gecko-log',
> +                        dest='gecko_log',
> +                        action='store',
> +                        help='path of the gecko.log file location, if No parameter given, Marionette Client will log on the current path')

"No" doesn't need to be capitalized
Attachment #8367188 - Flags: review?(mdas) → review-
Yep, sorry for "No" it was a typo :)
Ok this condition, I'll update the patch.
Whiteboard: [good first bug][lang=py][mentor=mdas] → [good first bug][lang=py][mentor=mdas][runner]
Assignee: redhoodie18 → nobody
Let me just update the bug with steps with how to do this now:

To get started:

You'll need a local copy of mozilla-central to make changes and test them:
https://hg.mozilla.org/mozilla-central/

To start running tests, go into the Marionette client directory and create a virtual environment:

cd <path to mozilla-central>/testing/marionette/client
virtualenv my_venv

(if you do not have virtualenv, install it using `pip install virtualenv`)

Then activate it and install marionette client:

. my_venv/bin/activate
python setup.py develop

To run the test suite, you can do:

cd marionette
python runtests.py --binary=<path to your firefox executable> --type=browser tests/unit/unit-test.ini

The firefox executable path is an executable file within your Firefox Application named 'firefox' on Mac/Linux and 'firefox.exe' in Windows (not sure about Marionette's compatibility with Windows though!)

This should start up a browser and run tests against it.

To address the bug:

Our test runner can start a Firefox instance and captures Firefox log data. Currently, we just log to a file called gecko.log in the current working directory. We want to add a new command line argument called --gecko-log=<FILE PATH> which will let us route the logging to the given FILE PATH.

For the new behaviour:
 -If FILE PATH is a directory, then we should save the gecko log in that directory with the following filename structure: gecko-(timestamp).log
 -If I give you a file path, use that path
 -If I don't give any information, just keep the current behaviour (gecko.log file in the current working directory with no timestamp information)

To do this:
You need to make our test runner accept the new command line argument, and pass the value all teh way to the GeckoInstance, which is what starts Firefox and does the logging. To do this:

you need to add a CLI argument like this https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py?rev=27f06ab08454#513, and you need to pass that value into BaseMarionetteTestRunner which starts the Marionette object here: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py?rev=27f06ab08454#513,. It uses _build_kwargs to start Marionette with the right arguments, so you'll need to modify _build_kwargs to pass in the gecko-log information to the Marionette object. The Marionette object then starts the GeckoInstance here: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py?rev=cbe169081f1c#487, so you'll need to modify GeckoInstance to take in the argument. It is defined here: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/geckoinstance.py?rev=526ae6094c53#22 and lastly (but most importantly!) the gecko log code that must be modified as per the bug is here: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/geckoinstance.py?rev=526ae6094c53#40
I'd like to work on this bug. I have two questions:

 - In BaseMarionetteTestRunner._build_kwargs, i think i have to update kwargs in the section "if self.emulator:" [1]. Is that right ?

 - Do you want some unit tests for this bug at the same time ? I can then send two patches, one for the code and another for the tests.

[1] https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py?rev=27f06ab08454#690
I answered my last first question myself - I think that kwargs must be updated in any case.
Attachment #8422470 - Flags: review?(mdas)
Comment on attachment 8422470 [details] [diff] [review]
add a new --gecko-log option to redirect gecko logs to a specified file

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

Great, this is working as expected, thank you!
. 
I have some minor fixes highlighted, so if you can update your patch, I will r+ it and push it:)

As for your question, we don't have a way of testing the test runner itself yet, we have a bug for that: Bug 973675. We only have tests for the marionette server, so you don't need to write any tests yet, but thank you for asking, it's a valid concern.

::: testing/marionette/client/marionette/geckoinstance.py
@@ +45,5 @@
> +            self.gecko_log = 'gecko.log'
> +        elif os.path.isdir(self.gecko_log):
> +            self.gecko_log = os.path.join(self.gecko_log,
> +                                          "gecko-%d.log" % time.time())
> +        

extra whitespace

::: testing/marionette/client/marionette/runner/base.py
@@ +485,5 @@
> +                        dest='gecko_log',
> +                        action='store',
> +                        help="Define the path to store log file. If the path is"
> +                             " a directory, the real log file will be created"
> +                             " givent the format gecko-(timestamp).log. If it is"

typo here: givent should be given

@@ +686,5 @@
>          kwargs = {
>              'device_serial': self.device_serial,
>              'symbols_path': self.symbols_path,
>              'timeout': self.timeout,
> +            'gecko_log': self.gecko_log,

This should be moved to the kwargs.update within line 693, since we only need to add this if we are passing a --binary argument (ie: if self.bin is a value)
Attachment #8422470 - Flags: review?(mdas) → review-
Patch updated as required :) If all works well, I may look at bug 973675 for the next one, or maybe you have other ideas ?
Attachment #8422470 - Attachment is obsolete: true
Attachment #8424351 - Flags: review?(mdas)
Comment on attachment 8424351 [details] [diff] [review]
add a new --gecko-log option to redirect gecko logs to a specified file

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

mdas is off this week, so I'll steal this review.  This looks good; I'll push it to try, and if it's green there, we can land it.  Thanks for the patch!
Attachment #8424351 - Flags: review?(mdas) → review+
try looks good; the one orange is a known intermittent
Keywords: checkin-needed
Attachment #8367188 - Attachment is obsolete: true
Assignee: nobody → j.parkouss
https://hg.mozilla.org/mozilla-central/rev/1a3225533557
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attached patch uplift for b2g30Splinter Review
--gecko-log option for v1.4
Comment on attachment 8490334 [details] [diff] [review]
uplift for b2g30

Had to modify this patch to land on b2g30.
Attachment #8490334 - Flags: review?(jgriffin)
Attachment #8490334 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/2bea1fbbcba8
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/2bea1fbbcba8

Don't ask.
Whiteboard: [good first bug][lang=py][mentor=mdas][runner] → [status-b2g-v1.3:fixed][status-b2g-v1.3t:fixed][lang=py][mentor=mdas][runner]
Comment on attachment 8526103 [details] [diff] [review]
add --logcat-stdout to emulator runs

wrong bug
Attachment #8526103 - Attachment is obsolete: true
Attachment #8526103 - Flags: review?(jgriffin)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: