Closed Bug 1284502 Opened 3 years ago Closed 3 years ago

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

Categories

(Testing :: Marionette, defect)

49 Branch
defect
Not set

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)
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: nobody → hskupin
Status: NEW → ASSIGNED
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/5c6a56bb4a6c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.