Closed Bug 1284502 Opened 3 years ago Closed 3 years ago

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


(Testing :: Marionette, defect)

49 Branch
Not set


(firefox50 fixed)

Tracking Status
firefox50 --- fixed


(Reporter: whimboo, Assigned: whimboo)




(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:

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
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:
See other reviews:
Attachment #8774404 - Flags: review?(mjzffr)

::: testing/marionette/harness/marionette/
(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 =

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
Marionette harness should log exceptions from run() only once. r=maja_zf
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.