mock_runner.driverclass should return mock_marionette

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: maja_zf, Assigned: maja_zf)

Tracking

({pi-marionette-harness-tests})

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

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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?
Attachment #8875297 - Flags: review?(hskupin)
(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 5

2 years ago
mozreview-review
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

2 years 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

2 years 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: 2 years 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.