Closed Bug 1292300 Opened 8 years ago Closed 8 years ago

Refactor Marionette harness unit tests to remove duplication, etc.

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: vakila, Assigned: vakila)

References

Details

(Keywords: pi-marionette-harness-tests)

Attachments

(3 files)

Bug 1292300 - Make mock_runner fixture do more work;
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1292300 - Refactor manifest-related tests with fixtures;
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1292300 - Small readability/typo fixes;
58 bytes, text/x-review-board-request
impossibus
: review+
Details 
We could refactor some tests/fixtures in `test_marionette_runner.py` to remove duplication, make tests shorter/more readable, make better use of Pytest features (e.g. fixtures, monkeypatch, parametrize), etc.

A couple of examples:

- to avoid duplication of the `mozversion.get_version` patch [1], that could probably be moved into the `mock_runner` fixture [2]

- tests like `test_add_test_manifest` [3] could probably make better use of parametrize to avoid duplicate calls to the same method, as well as of fixtures given that we'll be adding more similar tests (for e.g. Bug 1284238 and Bug 1291643) 



[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#420,436

[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#137-146

[3] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#370
Assignee: nobody → anjanavakil
Depends on: 1291643
In a comment on Bug 1291643 [1], Henrik suggested using patch decorators instead of nested `with patch...` statements, as it's more readable. 

I remembered why we didn't do that in the first place: pytest fixtures and mock `patch`es don't play nicely, because they're both using the argument names in the function declaration but in different ways. 

The `pytest-mock` plugin [2] indirectly solves this problem: you still can't use the decorators, but you can set a patch within the body of a test function in a one-liner, instead of a `with` block. That might make things a little more readable, but then we'd have one extra dependency - we had considered this at a couple of points but decided against the extra dependency. 

So for now what I'm doing is trying to minimize the need for `with patch...` blocks as much as possible by e.g. using monkeypatch [3], using more/better fixtures, etc. Other suggestions definitely welcome!


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1291643#c6
[2] https://pypi.python.org/pypi/pytest-mock/1.2
[3] http://doc.pytest.org/en/latest/monkeypatch.html
Blocks: 1284238
Blocks: 1291796
Comment on attachment 8779784 [details]
Bug 1292300 - Make mock_runner fixture do more work;

https://reviewboard.mozilla.org/r/70718/#review68994
Attachment #8779784 - Flags: review?(mjzffr) → review+
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69000

This is a great improvement.

I think there's a typo in the commit message ("feature" vs "fixture"?)

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:393
(Diff revision 1)
> -            assert mocks['read'].call_count == mocks['active_tests'].call_count == 1
> -            args, kwargs = mocks['active_tests'].call_args
> -            assert kwargs['app'] == mock_runner._appName
> -            mock_runner.tests, mock_runner.manifest_skipped_tests = [], []
> -            with patch('marionette.runner.base.os.path.exists', return_value=True):
> -                mock_runner.add_test(manifest)
> +    keys = ('path', 'expected', 'disabled')
> +    return [dict(zip(keys, values)) for values in included]
> +
> +@pytest.fixture
> +def mock_manifest(active_tests):
> +    manifest = Mock()

The mock object you use to patch TestManifest below should behave like the real thing -- I would define it with Mock(spec=TestManifest) to enforce that. So, I think it's best to avoid attaching extra attributes like n_disabled, filepath, etc, to that mock object. 

What about defining a manifest_fixture, with n_disabled/enabled, filepath and the mock manifest as attributes? Then manifest_fixture doesn't need to be a Mock, and you could have something like `manifest_fixture.mock_manifest = Mock(spec=...)` and `manifest_fixture.n_enabled = len...`

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:398
(Diff revision 1)
> -                mock_runner.add_test(manifest)
> -            assert mocks['read'].call_count == mocks['active_tests'].call_count == 2
> -    assert len(mock_runner.tests) == 2
> -    assert len(mock_runner.manifest_skipped_tests) == 1
> +    manifest = Mock()
> +    manifest.filepath = "/path/to/fake/manifest.ini"
> +    manifest.active_tests.return_value = active_tests
> +    manifest.n_disabled = len([t for t in active_tests if 'disabled' in t])
> +    manifest.n_enabled = len(active_tests) - manifest.n_disabled
> +    manifest.return_value = manifest

Why is this line needed? I don't see any calls to `mock_manifest()` anywhere.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:406
(Diff revision 1)
> +@pytest.mark.parametrize("files_exist", [True, False])
> +def test_add_test_manifest(mock_runner, mock_manifest, monkeypatch, files_exist):
> +    monkeypatch.setattr('marionette.runner.base.TestManifest', mock_manifest)
> +    with patch('marionette.runner.base.os.path.exists', return_value=files_exist):
> +        if files_exist or mock_manifest.n_enabled == 0:
> +            mock_runner.add_test(mock_manifest.filepath)

Your if condition seems incorrect: if `n_enabled == 0` but `files_exist == False`, we expect this line to raise an IOError, right? 

I think you really want something like "if file doesn't exist, assert error is raised, otherwise assert all that other stuff"

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:410
(Diff revision 1)
> -        assert test['filepath'].endswith(('test_expected_pass.py', 'test_expected_fail.py'))
> -        if test['filepath'].endswith('test_expected_fail.py'):
> +                assert test['filepath'].endswith(('pass.py', 'fail.py'))
> +                assert test['filepath'].endswith(test['expected'] + '.py')

Looks like this line can be removed; redundant with line below it.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:415
(Diff revision 1)
> +    args, kwargs = mock_manifest.active_tests.call_args
> +    assert kwargs['app'] == mock_runner._appName

Could you take a look at moving the call_args testing to a separate test method, please? (Perhaps as a separate patch/bug)
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69008
Attachment #8779785 - Flags: review?(mjzffr) → review-
Comment on attachment 8779786 [details]
Bug 1292300 - Small readability/typo fixes;

https://reviewboard.mozilla.org/r/70722/#review69010

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:360
(Diff revision 1)
>      ]
>      tests = list(dir_contents[0][2] + dir_contents[1][2])
>      assert len(runner.tests) == 0
> -    with patch('os.path.isdir') as isdir:
> -        # Need to use side effect to make isdir return True for test_dir and False for tests
> +    # Need to use side effect to make isdir return True for test_dir and False for tests
> -        isdir.side_effect = [True] + [False for i in tests]
> +    with patch('os.path.isdir', side_effect = [True]+[False for t in tests]) as isdir:

pep8 nits: remove whitespace around =, add whitespace around +
Attachment #8779786 - Flags: review?(mjzffr) → review+
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69000

Yup, thanks for the catch :D

> Why is this line needed? I don't see any calls to `mock_manifest()` anywhere.

I'm patching the `TestManifest` class as this `mock_manifest` feature, and in `base.py` `add_test` it calls `TestManifest()` to create the manifest object [1], hence this little hack of just making the fixture return itself. Agreed that this is a bit confusing and there's probably a better way to go about it - perhaps with the `manifest_fixture` you suggested in another comment. 

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#977

> Your if condition seems incorrect: if `n_enabled == 0` but `files_exist == False`, we expect this line to raise an IOError, right? 
> 
> I think you really want something like "if file doesn't exist, assert error is raised, otherwise assert all that other stuff"

Actually if the manifest is empty after filtering, it doesn't raise an error in `add_test` (though it writes an error message to the log) [1], but instead goes happily along through `run_tests` until it tries to `run_test_sets` and realizes there are no tests to run [2]. But since here I'm just testing `add_test` and not `run_tests`, the only case where it will throw an error is if the manifest is non-empty and the test files don't exist.

Whether or not we should be raising an error in `add_test` at [1] if the manifest comes up empty is another question - I considered it briefly with Henrik and we decided to leave it as-is, though I'm having trouble remembering the rationale... what do you think?

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#991-994

[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#1075-1076

> Could you take a look at moving the call_args testing to a separate test method, please? (Perhaps as a separate patch/bug)

Sure, in fact this would probably be a good segue into Bug 1284238
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69000

> The mock object you use to patch TestManifest below should behave like the real thing -- I would define it with Mock(spec=TestManifest) to enforce that. So, I think it's best to avoid attaching extra attributes like n_disabled, filepath, etc, to that mock object. 
> 
> What about defining a manifest_fixture, with n_disabled/enabled, filepath and the mock manifest as attributes? Then manifest_fixture doesn't need to be a Mock, and you could have something like `manifest_fixture.mock_manifest = Mock(spec=...)` and `manifest_fixture.n_enabled = len...`

I took your suggestion and created a `ManifestFixture` class as a wrapper around a mock based on `TestManifest`, so that we can keep the other attributes (`filepath`, `n_disabled` etc) around for convenience.
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69000

> I'm patching the `TestManifest` class as this `mock_manifest` feature, and in `base.py` `add_test` it calls `TestManifest()` to create the manifest object [1], hence this little hack of just making the fixture return itself. Agreed that this is a bit confusing and there's probably a better way to go about it - perhaps with the `manifest_fixture` you suggested in another comment. 
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#977

It's probably fine as is then. I guess normally you'd patch TestManifest with return_value=mock_manifest, which just creates a new Mock with return_value mock_manifest.

> Actually if the manifest is empty after filtering, it doesn't raise an error in `add_test` (though it writes an error message to the log) [1], but instead goes happily along through `run_tests` until it tries to `run_test_sets` and realizes there are no tests to run [2]. But since here I'm just testing `add_test` and not `run_tests`, the only case where it will throw an error is if the manifest is non-empty and the test files don't exist.
> 
> Whether or not we should be raising an error in `add_test` at [1] if the manifest comes up empty is another question - I considered it briefly with Henrik and we decided to leave it as-is, though I'm having trouble remembering the rationale... what do you think?
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#991-994
> 
> [2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#1075-1076

Ahh, last night I was thinking the 'exists' patch was also for `manifest.read(filepath)` but that will just always return a Mock, and you're only looking at the existence of test files. Maybe name the condition 'test_files_exist'.
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69096

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:383
(Diff revisions 1 - 2)
> +        self.n_disabled = len([t for t in tests if 'disabled' in t])
> +        self.n_enabled = len(tests) - self.n_disabled
> +        self.mock_manifest = Mock(spec=TestManifest)
> +        self.mock_manifest.active_tests.return_value = tests
> +        self.__call__ = lambda: self.mock_manifest
> +        self.__repr__ = lambda: "<ManifestFixture {}>".format(name)

I just declared the `__call__` and `__repr__` methods like this to save a couple of lines and to keep all the `ManifestFixture` code in one method/block. But I'm happy to rewrite them normally (with `def`) if it's confusing/unreadable like this.

(The `__repr__` method is just there so we can easily see the connection between a given ManifestFixture and the parametrized value of `active_tests` in the output of a failing test. We could also live without it.)
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69000

> Ahh, last night I was thinking the 'exists' patch was also for `manifest.read(filepath)` but that will just always return a Mock, and you're only looking at the existence of test files. Maybe name the condition 'test_files_exist'.

Sure thing. I'll just wait to see if there's anything else that needs to be changed before pushing a new patch with `test_files_exist`.
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69102

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:375
(Diff revisions 1 - 2)
>      for test in runner.tests:
>          assert test_dir in test['filepath']
>      assert len(runner.tests) == 4
>  
>  
> +class ManifestFixture:

You could define this class in manifest_fixture function.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:382
(Diff revisions 1 - 2)
> +        self.filepath = "/path/to/fake/manifest.ini"
> +        self.n_disabled = len([t for t in tests if 'disabled' in t])
> +        self.n_enabled = len(tests) - self.n_disabled
> +        self.mock_manifest = Mock(spec=TestManifest)
> +        self.mock_manifest.active_tests.return_value = tests
> +        self.__call__ = lambda: self.mock_manifest

This doesn't completely resolve the issue I commented about before: let's not conflate the manifest_fixture idea with the mock_manifest that is used for patching -- it's not necessary and might only confuse someone reading this code in a year. See also other line comment in test_method.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:406
(Diff revisions 1 - 2)
> -    manifest.return_value = manifest
> -    return manifest
>  
>  @pytest.mark.parametrize("files_exist", [True, False])
> -def test_add_test_manifest(mock_runner, mock_manifest, monkeypatch, files_exist):
> -    monkeypatch.setattr('marionette.runner.base.TestManifest', mock_manifest)
> +def test_add_test_manifest(mock_runner, manifest_fixture, monkeypatch, files_exist):
> +    monkeypatch.setattr('marionette.runner.base.TestManifest', manifest_fixture)

For clarity, this should be `manifest_fixture.mock_manifest`. Same in other test function.
Attachment #8779785 - Flags: review?(mjzffr) → review-
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69096

> I just declared the `__call__` and `__repr__` methods like this to save a couple of lines and to keep all the `ManifestFixture` code in one method/block. But I'm happy to rewrite them normally (with `def`) if it's confusing/unreadable like this.
> 
> (The `__repr__` method is just there so we can easily see the connection between a given ManifestFixture and the parametrized value of `active_tests` in the output of a failing test. We could also live without it.)

This is good. I don't object to `__repr__` but my earlier comment argues that `__call__` is unnecessary.
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69120

Rewrote the `ManifestFixture` class, and moved it into the `manifest_fixture` method: now `ManifestFixture` has no `__call__` method, and `.mock_manifest` is a Mock that returns another Mock that represents the manifest instance.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:418
(Diff revisions 2 - 3)
>              for test in mock_runner.tests:
>                  assert test['filepath'].endswith(test['expected'] + '.py')
>          else:
>              pytest.raises(IOError, "mock_runner.add_test(manifest_fixture.filepath)")
> -    assert manifest_fixture.mock_manifest.read.called
> -    assert manifest_fixture.mock_manifest.active_tests.called
> +    assert manifest_fixture.mock_manifest().read.called
> +    assert manifest_fixture.mock_manifest().active_tests.called

The drawback with using `manifest_fixture.mock_manifest` as the callable mock version of `TestManifest` is that we need to call it whenever we want to access the properties of the mock. I had originally written it this way (`mock_manifest` is a Mock that returns a Mock), but changed to the other design because I thought that combination of declaring a Mock-within-a-Mock and then accessing `mock_manifest().property` was harder to read than the other way around. But maybe that's just me, and to most people it's clearer this way. 

Just let me know if you think it's OK like this, or if you have any other suggestions/alternatives.
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69120

> The drawback with using `manifest_fixture.mock_manifest` as the callable mock version of `TestManifest` is that we need to call it whenever we want to access the properties of the mock. I had originally written it this way (`mock_manifest` is a Mock that returns a Mock), but changed to the other design because I thought that combination of declaring a Mock-within-a-Mock and then accessing `mock_manifest().property` was harder to read than the other way around. But maybe that's just me, and to most people it's clearer this way. 
> 
> Just let me know if you think it's OK like this, or if you have any other suggestions/alternatives.

The extra parens do make it look busier, but I think `mock_manifest().property` is clearer in the sense that it matches the way the mock is actually being used via the patch.
Comment on attachment 8779785 [details]
Bug 1292300 - Refactor manifest-related tests with fixtures;

https://reviewboard.mozilla.org/r/70720/#review69436
Attachment #8779785 - Flags: review?(mjzffr) → review+
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/813d8dab6d2b
Make mock_runner fixture do more work; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/751d0d591360
Refactor manifest-related tests with fixtures; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/19455d8adc07
Small readability/typo fixes; r=maja_zf
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: