1223429 - Return exit code 10 when Marionette harness has failing tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Andreas Tolfsen ❲:ato❳]

As maja_zf and whimboo points out in bug 1222388, mozharness relies on the Marionette harness to return an exit code 10 when there are test failures.  Returning any other exit code will cause it to believe there was an exceptional harness failure.

Following the patch for bug 1222388, any test failures will be classified as harness problems.  We should fix this as soon as possible.

To quote maja_zf:

> Mn, Mn-e10s and Mnw all use a mozharness script [1] that relies of the '10' 
> exit code. This issue isn't detected in the above try run because the tests 
> happen to pass. If there were test failures, however, they would be 
> incorrectly reported as harness failures. 
>
> [1] https://dxr.mozilla.org/mozilla-central/rev/4a7526d26bd47ce2e01f938702b91c95424026ed/testing/mozharness/scripts/marionette.py#469

I think the path of least resistance is to resume the old behaviour and return an exit code 10.

Comment 1

[Andreas Tolfsen ❲:ato❳]

dburns, do you have an opinion about this?

Comment 2

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

Attachment: MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester

Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester

Comment 3

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

I know I may be jumping the gun, but here's a quick fix. 
Try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcbde8c72268

Comment 4

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24809/#review22327

::: testing/marionette/client/marionette/runtests.py:79
(Diff revision 1)
> -            return runner
> +            if runner.failed > 0:
> +                exit_code = 10
>          except Exception:
>              self.args.logger.error('Failure during test execution.',
>                                     exc_info=True)
> -            sys.exit(1)
> +            exit_code = 1
> +        return exit_code

As I pointed out in the review for bug 1222388, I don't feel `MarionetteHarness` should know anything about exit codes.

I feel dburns' patch to return the runner, or more ideally the number of failed tests, is better API design than a magic number.

The determination of the exit code should, I feel, be the responsibility of the consumer.

Comment 5

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

Comment on attachment 8685516 [details]
MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24809/diff/1-2/

Comment 6

[David Burns :automatedtester]

(without reading the patch) *mutters about use of magic numbers and not documenting them*

Comment 7

[David Burns :automatedtester]

Comment on attachment 8685516 [details]
MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester

https://reviewboard.mozilla.org/r/24809/#review22365

::: testing/marionette/client/marionette/runtests.py:73
(Diff revision 2)
> -        except Exception:
> +        except Exception as e:

you don't need `as e`, the `raise` 2 lines later can literally just say `raise`

::: testing/marionette/client/marionette/runtests.py:83
(Diff revision 2)
> +            sys.exit(10)

can you please put comments on what `0`, `1`, `10` means for the exit codes

Comment 8

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

Comment on attachment 8685516 [details]
MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24809/diff/2-3/

Comment 9

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

Comment on attachment 8685516 [details]
MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24809/diff/3-4/

Comment 10

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

https://reviewboard.mozilla.org/r/24809/#review22491

::: testing/marionette/client/marionette/runtests.py:90
(Diff revisions 3 - 4)
> -        failed = MarionetteHarness(runner_class, parser_class).run()
> +        failed = harness_class(runner_class, parser_class).run()

Extra change: adding the `harness_class` param makes it easier to reuse the `cli` method in other suites like Firefox UI Tests.

Comment 11

[David Burns :automatedtester]

Comment on attachment 8685516 [details]
MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester

https://reviewboard.mozilla.org/r/24809/#review22493

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b151b86247fc

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c308db03b242

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c308db03b242