Closed
Bug 1284502
Opened 9 years ago
Closed 9 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•9 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•9 years ago
|
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 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•9 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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•