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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Actually the extra logging came in via bug 1227918.

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: 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/

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8774404 [details]
Bug 1284502 - Marionette harness should log exceptions from run() only once.

https://reviewboard.mozilla.org/r/66882/#review63802

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c6a56bb4a6c
Marionette harness should log exceptions from run() only once. r=maja_zf

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c6a56bb4a6c