Marionette harness logs exceptions twice in cli() and run()

RESOLVED FIXED in Firefox 50

Status

Testing
Marionette
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

49 Branch
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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

2 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

2 years ago
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8774404 [details]
Bug 1284502 - Marionette harness should log exceptions from run() only once.

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

2 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 6

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5c6a56bb4a6c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.