mock_runner.driverclass should return mock_marionette

RESOLVED FIXED in Firefox 55

Status

Testing
Marionette
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: maja_zf, Assigned: maja_zf)

Tracking

({ateam-marionette-harness-tests})

Version 3
mozilla55
ateam-marionette-harness-tests
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
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 hidden (mozreview-request)
Attachment #8875297 - Flags: review?(ato)
Not sure I feel comfortable reviewing this code.  Could you find someone else?
(Assignee)

Updated

6 months ago
Attachment #8875297 - Flags: review?(hskupin)
(Assignee)

Comment 3

6 months ago
(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.
(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 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.
Attachment #8875297 - Flags: review?(hskupin) → review+
(Assignee)

Comment 6

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 8

6 months ago
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6c8c383edd02
mock_runner.driverclass should return mock_marionette; r=whimboo
https://hg.mozilla.org/mozilla-central/rev/6c8c383edd02
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.