Closed
Bug 1284502
Opened 8 years ago
Closed 8 years ago
Marionette harness logs exceptions twice in cli() and run()
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox50 fixed)
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(1 file)
As noticed today we do an extra logging for exceptions happening inside of `run()` within the cli. This doesn't seem to be necessary: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runtests.py#72 https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runtests.py#95 As the `cli()` logging message says it should only log exceptions for harness setup but not the execution of tests. Maja, maybe this came in with your refactoring a couple of months ago?
Flags: needinfo?(mjzffr)
Assignee | ||
Comment 1•8 years ago
|
||
Actually the extra logging came in via bug 1227918.
Blocks: 1227918
Keywords: regression
The logging added via bug 1227918 is necessary, as explained in that bug's summary -- in particular, if there is any issue with the `harness_class` constructor. If you want to change how logging is done with respect to test execution versus harness setup, I suppose you could remove the logging message at line 72, since the exception is re-raised anyway, and generalize the message at line 95.
Flags: needinfo?(mjzffr)
Keywords: regression
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
Whenever an exception is raised while tests are executed, the log error message should only be printed once. As best this should happen in `cli()`, so that subclasses can better set their own behavior, and we safe us from re-raising the exception. Review commit: https://reviewboard.mozilla.org/r/66882/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66882/
Attachment #8774404 -
Flags: review?(mjzffr)
Assignee | ||
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/66882/#review63632 ::: testing/marionette/harness/marionette/runtests.py:86 (Diff revision 1) > """ > logger = mozlog.commandline.setup_logging('Marionette test runner', {}) > try: > - failed = harness_class(runner_class, parser_class, testcase_class, args=args).run() > + harness_instance = harness_class(runner_class, parser_class, testcase_class, > + args=args) > + failed = harness_instance.run() I split this line up to make it easier to spot if the exception actually happened during setup or test execution.
Attachment #8774404 -
Flags: review?(mjzffr) → review+
Comment on attachment 8774404 [details] Bug 1284502 - Marionette harness should log exceptions from run() only once. https://reviewboard.mozilla.org/r/66882/#review63802
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5c6a56bb4a6c Marionette harness should log exceptions from run() only once. r=maja_zf
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5c6a56bb4a6c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
•