Closed Bug 1223429 Opened 7 years ago Closed 7 years ago

Return exit code 10 when Marionette harness has failing tests

Categories

(Testing :: Marionette, defect, P1)

defect

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: ato, Assigned: impossibus)

References

Details

(Keywords: pi-marionette-runner)

Attachments

(1 file)

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.
dburns, do you have an opinion about this?
Flags: needinfo?(dburns)
Priority: -- → P1
Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester
Attachment #8685516 - Flags: review?(dburns)
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
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 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/
(without reading the patch) *mutters about use of magic numbers and not documenting them*
Flags: needinfo?(dburns)
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
Attachment #8685516 - Flags: review?(dburns) → review+
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/
Attachment #8685516 - Attachment description: MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester → MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r=automatedtester
Attachment #8685516 - Flags: review+ → review?(dburns)
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/
Attachment #8685516 - Attachment description: MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r=automatedtester → MozReview Request: Bug 1223429 - Return exit code 10 when Marionette harness has failing tests; r?automatedtester
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.
Attachment #8685516 - Flags: review?(dburns) → review+
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
https://hg.mozilla.org/mozilla-central/rev/c308db03b242
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.