1370899 - mock_runner.driverclass should return mock_marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

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

The fixtures need a small fix to be more strict.

Right now, mock_runner.driverclass = mock_marionette, which means that mock_runner.driverclass() returns another Mock, but it should return mock_marionette. The difference is that mock_marionette is a Mock that is spec'd to behave like an instance of Marionette, so calling invalid methods/attributes will fail.

Comment 1

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

Attachment: Bug 1370899 - mock_runner.driverclass should return mock_marionette;

Previously, mock_runner.driverclass() returned a plain Mock object.

Review commit: https://reviewboard.mozilla.org/r/146712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146712/

Comment 2

[Andreas Tolfsen ❲:ato❳]

Not sure I feel comfortable reviewing this code.  Could you find someone else?

Comment 3

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

(In reply to Andreas Tolfsen ‹:ato› from comment #2)
> Not sure I feel comfortable reviewing this code.  Could you find someone
> else?

Sure. I figured you'd be alright with it since you're doing some work with these fixtures now. 

Probably you, Henrik and David are equally comfortable with the mnh code.

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #3)
> (In reply to Andreas Tolfsen ‹:ato› from comment #2)
> > Not sure I feel comfortable reviewing this code.  Could you find someone
> > else?
> 
> Sure. I figured you'd be alright with it since you're doing some work with
> these fixtures now. 

Doesn’t mean I’m comfortable with them! (-;

Comment 5

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

Comment on attachment 8875297 [details]
Bug 1370899 - mock_runner.driverclass should return mock_marionette;

https://reviewboard.mozilla.org/r/146712/#review151724

::: testing/marionette/harness/marionette_harness/tests/harness_unit/conftest.py:95
(Diff revision 1)
>  
>  
>  @pytest.fixture
>  def mock_marionette(request):
>      """ Mock marionette instance """
> -    marionette_class = MagicMock(spec=Marionette)
> +    mn = MagicMock(spec=Marionette)

Can we keep the full name `marionette` for the variable? Or would that collide with something?

::: testing/marionette/harness/marionette_harness/tests/harness_unit/conftest.py:96
(Diff revision 1)
>  
>  @pytest.fixture
>  def mock_marionette(request):
>      """ Mock marionette instance """
> -    marionette_class = MagicMock(spec=Marionette)
> +    mn = MagicMock(spec=Marionette)
> +    mn.instance = Mock()

For what is the `instance` property needed?

::: testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py:28
(Diff revision 1)
>      """
>      MarionetteTestRunner instance with mocked-out
>      self.marionette and other properties,
>      to enable testing runner.run_tests().
>      """
> -    runner.driverclass = mock_marionette
> +    runner.driverclass = Mock(return_value=mock_marionette)

Nice spot. Maybe that helps me for the --run-until-failure case.

Comment 6

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

Comment on attachment 8875297 [details]
Bug 1370899 - mock_runner.driverclass should return mock_marionette;

https://reviewboard.mozilla.org/r/146712/#review151724

> Can we keep the full name `marionette` for the variable? Or would that collide with something?

Done

> For what is the `instance` property needed?

`mn = MagicMock(spec=Marionette)` means that accessing any attributes on `m` that aren't part of `dir(Marionette)` will raise an AttributeError instead of automagically creating a new Mock object for that attribute. `dir(Marionette)` includes only methods and class attributes, which does not include `instance` as well as other attributes added by `__init__`. So I manually add it in with `mn.instance = Mock()` since our tests touch it.

However... in answering your question I realized the following makes more sense instead: `mn = MagicMock(spec=Marionette())`. (Thank goodness the Marionette ctor doesn't do any work by default...)

Comment 7

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

Comment on attachment 8875297 [details]
Bug 1370899 - mock_runner.driverclass should return mock_marionette;

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c8c383edd02
mock_runner.driverclass should return mock_marionette; r=whimboo

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6c8c383edd02