1222388 - ./mach marionette-test fails due to "ImportError: cannot import name startTestRunner"

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Mark Banner (:standard8)]

Bug 1212608 broke ./mach marionette-test - it changed startTestRunner to MarionetteHarness but didn't update mach_commands.py. Now when we try to run the tests we get:

Error running mach:

    ['marionette-test']

The error occurred in the implementation of the invoked mach command.

This should never occur and is likely a bug in the implementation of that
command. Consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ImportError: cannot import name startTestRunner

  File "/Users/mark/loop/gecko-dev/testing/marionette/mach_commands.py", line 120, in run_marionette_test
    return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs)
  File "/Users/mark/loop/gecko-dev/testing/marionette/mach_commands.py", line 39, in run_marionette
    from marionette.runtests import (

I took a look at doing a port, but its unclear to me about how the extra items in mach_commands.py (like b2g things) should be handled.

Marked as blocker as this prevents developers from easily running tests.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Maja, is this a regression from any of your refactor of the test runner?

Comment 2

[David Burns :automatedtester]

This is a regression made from Bug 1212608

Comment 3

[David Burns :automatedtester]

Attachment: MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

Bug 1222388: Correct `./mach marionette-test` r?ato

There have been a number of improvements to the marionette test runner
unfortunately we missed this regression.

Comment 4

[David Burns :automatedtester]

Comment on attachment 8684175 [details]
MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

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

Comment 5

[David Burns :automatedtester]

Maja, could you check that I havent regressed firefox-ui/media tests please?

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8684175 [details]
MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

https://reviewboard.mozilla.org/r/24493/#review22017

::: testing/marionette/mach_commands.py:71
(Diff revision 2)
> -    runner = startTestRunner(MarionetteTestRunner, args)
> +    runner = MarionetteHarness(MarionetteTestRunner, args=args).run()
> -    if runner.failed > 0:
> -        return 1
>  
>      return 0

`run()` does not have a return value.

In fact, calling `MarionetteHarness.run` will cause the program to end and the following `return 0` statement is never reached.

It feels to me that invoking `MarionetteHarness` shouldn't exit the program on your behalf and that the previous behaviour where `startTestRunner` returned the exit code was more appropriate.

Comment 7

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/24493/#review22017

> `run()` does not have a return value.
> 
> In fact, calling `MarionetteHarness.run` will cause the program to end and the following `return 0` statement is never reached.
> 
> It feels to me that invoking `MarionetteHarness` shouldn't exit the program on your behalf and that the previous behaviour where `startTestRunner` returned the exit code was more appropriate.

The `return 0` statement is reached if there are no failures. I will remove the `runner` variable.

`startTestRunner` never returned an exit code. It would exit in a similar fashion.

Comment 8

[David Burns :automatedtester]

Comment on attachment 8684175 [details]
MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

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

Comment 9

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24493/#review22017

> The `return 0` statement is reached if there are no failures. I will remove the `runner` variable.
> 
> `startTestRunner` never returned an exit code. It would exit in a similar fashion.

OK, but it still feels wrong that library code you call exits the program for you.  `startTestRunner` did not have this behaviour: https://hg.mozilla.org/mozilla-central/annotate/5665bf654b8f/testing/marionette/client/marionette/runtests.py#l31

It used to return the instantiation of the `MarionetteTestRunner` class reference that is now passed into `MarionetteHarness`'s constructor.

I think it would make sense to move the `if runner.failed > 0` condition outside of `MarionetteHarness.run`, possibly to here, so that the return statement here preserves its function.

Comment 10

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24493/#review22017

> OK, but it still feels wrong that library code you call exits the program for you.  `startTestRunner` did not have this behaviour: https://hg.mozilla.org/mozilla-central/annotate/5665bf654b8f/testing/marionette/client/marionette/runtests.py#l31
> 
> It used to return the instantiation of the `MarionetteTestRunner` class reference that is now passed into `MarionetteHarness`'s constructor.
> 
> I think it would make sense to move the `if runner.failed > 0` condition outside of `MarionetteHarness.run`, possibly to here, so that the return statement here preserves its function.

Actually it would be better not to expose the runnre instantiation at all, which I guess is what Maja was trying to achieve.  The better solution would be to either return the number of failed tests or a boolean indicating that all tests were successful, from `MarionetteHarness.run`.

Comment 11

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

https://reviewboard.mozilla.org/r/24493/#review22017

> Actually it would be better not to expose the runnre instantiation at all, which I guess is what Maja was trying to achieve.  The better solution would be to either return the number of failed tests or a boolean indicating that all tests were successful, from `MarionetteHarness.run`.

...or return a return code, in case the runner is busted.

Comment 12

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

https://reviewboard.mozilla.org/r/24493/#review22067

::: testing/marionette/client/marionette/runtests.py:73
(Diff revision 3)
> -                sys.exit(10)
> +                sys.exit(1)

I believe the `10` distinguishes test failures from a 'busted' state.

Comment 13

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

(In reply to David Burns :automatedtester from comment #5)
> Maja, could you check that I havent regressed firefox-ui/media tests please?

The current patch doesn't break firefox-ui/media tests. 

I'll check again once you push more updates.

Comment 14

[David Burns :automatedtester]

Comment on attachment 8684175 [details]
MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

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

Comment 15

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/24493/#review22017

> ...or return a return code, in case the runner is busted.

Returning a return code without a standard saying what the magic number means scares me a little. Perhaps we can do that at a later stage (if it is needed)

Comment 16

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/24493/#review22067

> I believe the `10` distinguishes test failures from a 'busted' state.

I have dropped the magic number here for a boolean.

Comment 17

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

https://reviewboard.mozilla.org/r/24493/#review22125

::: testing/marionette/client/marionette/runtests.py:83
(Diff revision 4)
>      MarionetteHarness(runner_class, parser_class).run()

We still need sys.exit() here.

Comment 18

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24493/#review22017

> Returning a return code without a standard saying what the magic number means scares me a little. Perhaps we can do that at a later stage (if it is needed)

I agree with the sentiment towards magic numbers.  Why can't it raise an exception in that case?

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8684175 [details]
MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

https://reviewboard.mozilla.org/r/24493/#review22131

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

I still feel this is wrong, and I'm sorry to seem obsessive about this, but hiding exceptions doesn't seem like a good idea.

Maybe the Right Solution is to return a dict of passes/failures/errors, and instead propagate the exception after it has been logged?

Comment 20

[David Burns :automatedtester]

Comment on attachment 8684175 [details]
MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

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

Comment 21

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/24493/#review22131

> I still feel this is wrong, and I'm sorry to seem obsessive about this, but hiding exceptions doesn't seem like a good idea.
> 
> Maybe the Right Solution is to return a dict of passes/failures/errors, and instead propagate the exception after it has been logged?

It is now returning the runner (which has pass/failed etc).

The exception here, according to Maja needs to handle the exiting. We can visit fixing this in a different bug as it will require downstream users getting updated.

Comment 22

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8684175 [details]
MozReview Request: Bug 1222388: Correct `./mach marionette-test` r?ato

https://reviewboard.mozilla.org/r/24493/#review22201

Comment 23

[David Burns :automatedtester]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58e92931b04f6ef6ce6eb87caf0fe0e79859b91d
Bug 1222388: Correct `./mach marionette-test` r=ato

Comment 24

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

https://reviewboard.mozilla.org/r/24493/#review22209

::: testing/marionette/client/marionette/runtests.py:79
(Diff revision 5)
>      MarionetteHarness(runner_class, parser_class).run()

I assume we have to exit here with 10 in case of failures. If we don't do so all existing tools which use Marionette will be broken because they might be checking for 10 as exit code which indicated that tests were only failing. As of now we would exit with 0?

Comment 25

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

https://reviewboard.mozilla.org/r/24493/#review22131

> It is now returning the runner (which has pass/failed etc).
> 
> The exception here, according to Maja needs to handle the exiting. We can visit fixing this in a different bug as it will require downstream users getting updated.

The exception doesn't need to handle the exiting. I meant to say that if run() doesn't handle the exiting, which is fine, then runtests.cli() needs to handle the exiting. Downstream consumers may rely on that and on the magic-number exit codes.

Comment 26

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24493/#review22209

> I assume we have to exit here with 10 in case of failures. If we don't do so all existing tools which use Marionette will be broken because they might be checking for 10 as exit code which indicated that tests were only failing. As of now we would exit with 0?

Judging by [the try run](https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=58e92931b04f) there doesn't appear to be any in-tree tooling that looks for this exit code, so when you say "all existing tools which use Marionette" that's a truth with modifications.

Are there any examples of consumers that rely explicitly on exit code 10, as opposed to say a non-zero exit code?

Comment 27

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

I don't know but I don't think that using the same exit code for everything is a good idea. With that a tool which uses Marionette CLI will not be able to easily figure out if there are only test failures or the script being busted. We cannot force them all to use the API especially not for build systems which are based on scripts and want to call Marionette via its CLI.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58e92931b04f

Comment 29

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

Keeping bug closed but setting ni? for David so we can clarify the remaining changes before we can release a new version of Marionette.

Comment 30

[David Burns :automatedtester]

The changes here shouldnt affect any release. Maja had a couple patches to land before we do a release.

Comment 31

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

(In reply to Andreas Tolfsen (:ato) from comment #26)
> https://reviewboard.mozilla.org/r/24493/#review22209
> 
> > I assume we have to exit here with 10 in case of failures. If we don't do so all existing tools which use Marionette will be broken because they might be checking for 10 as exit code which indicated that tests were only failing. As of now we would exit with 0?
> 
> Judging by [the try
> run](https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=58e92931b04f) there doesn't appear to be any in-tree
> tooling that looks for this exit code, so when you say "all existing tools
> which use Marionette" that's a truth with modifications.
> 
> Are there any examples of consumers that rely explicitly on exit code 10, as
> opposed to say a non-zero exit code?

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

Comment 32

[Andreas Tolfsen ❲:ato❳]

I understand.  Filed bug 1223429 as a follow-up on this.

Comment 33

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/24493/#review22209

> Judging by [the try run](https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=58e92931b04f) there doesn't appear to be any in-tree tooling that looks for this exit code, so when you say "all existing tools which use Marionette" that's a truth with modifications.
> 
> Are there any examples of consumers that rely explicitly on exit code 10, as opposed to say a non-zero exit code?

Following maja_zf's explanation in https://bugzilla.mozilla.org/show_bug.cgi?id=1222388#c31, I filed bug 1223429 to follow up on this.

I have no idea why I can't resolve issues.