Closed
Bug 873035
Opened 11 years ago
Closed 10 years ago
Marionette should have a CLI for stdout log
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
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)
5.26 KB,
patch
|
Details | Diff | Splinter Review | |
8.61 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
5.19 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
Apparently this stuff goes to gecko.log.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
Comment 2•11 years ago
|
||
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
Comment 3•11 years ago
|
||
mdas, if you don't feel like mentoring this, bounce it back to me
Whiteboard: [good first bug][lang=py][mentor=mdas]
Comment 4•11 years ago
|
||
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)?
Comment 5•11 years ago
|
||
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.
Comment 6•11 years ago
|
||
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.
Comment 8•11 years ago
|
||
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:)
Comment 10•11 years ago
|
||
Attachment #762523 -
Flags: review?(mdas)
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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.
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
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-
Comment 15•11 years ago
|
||
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 :)
Comment 16•11 years ago
|
||
First submission. The helping comment on the option will be probably not good :)
Attachment #802459 -
Flags: review?(mdas)
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
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
Comment 19•11 years ago
|
||
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) ?
Comment 20•11 years ago
|
||
(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.
Comment 21•11 years ago
|
||
(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
Comment 22•11 years ago
|
||
hehe, yes thanks, I try to figure out the issue this week !! :)
Comment 23•10 years ago
|
||
Hey, I would like to take up this bug. Please guide me
Comment 24•10 years ago
|
||
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.
Comment 25•10 years ago
|
||
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 26•10 years ago
|
||
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-
Comment 27•10 years ago
|
||
Yep, sorry for "No" it was a typo :) Ok this condition, I'll update the patch.
Updated•10 years ago
|
Whiteboard: [good first bug][lang=py][mentor=mdas] → [good first bug][lang=py][mentor=mdas][runner]
Updated•10 years ago
|
Assignee: redhoodie18 → nobody
Comment 28•10 years ago
|
||
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
Assignee | ||
Comment 29•10 years ago
|
||
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
Assignee | ||
Comment 30•10 years ago
|
||
I answered my last first question myself - I think that kwargs must be updated in any case.
Attachment #8422470 -
Flags: review?(mdas)
Comment 31•10 years ago
|
||
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-
Assignee | ||
Comment 32•10 years ago
|
||
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 33•10 years ago
|
||
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+
Comment 34•10 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=49b0b8a70d59
Comment 35•10 years ago
|
||
try looks good; the one orange is a known intermittent
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8367188 -
Attachment is obsolete: true
Updated•10 years ago
|
Assignee: nobody → j.parkouss
Comment 36•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3225533557
Keywords: checkin-needed
Comment 37•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1a3225533557
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 38•10 years ago
|
||
--gecko-log option for v1.4
Comment 39•10 years ago
|
||
Comment on attachment 8490334 [details] [diff] [review] uplift for b2g30 Had to modify this patch to land on b2g30.
Attachment #8490334 -
Flags: review?(jgriffin)
Updated•10 years ago
|
Attachment #8490334 -
Flags: review?(jgriffin) → review+
Comment 40•10 years ago
|
||
landed on b2g30: https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/07ab32a08e18 and on esr31: https://hg.mozilla.org/releases/mozilla-esr31/rev/07f342aeb78c
Comment 41•10 years ago
|
||
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 hidden (obsolete) |
Comment 43•10 years ago
|
||
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)
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•