1291796 - Separate Marionette harness unit tests into class-specific modules

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Anjana Vakil (:vakila)]

At the moment, harness unit tests for various runner classes (from [1],[2]) are all contained in a single module, `tests/harness_unit/test_marionette_runner.py`[3]. 

It would be better to divide this into separate modules that are specific to a certain class (or perhaps a related group of classes), e.g. `test_marionette_runner.py`, `test_marionette_arguments.py`, `test_marionette_test_result.py` etc. 


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

[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runtests.py

[3] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py

Comment 1

[Anjana Vakil (:vakila)]

Attachment: Bug 1291796 - Split Marionette harness tests by class;

Split the Marionette harness unit tests, all of which were
previously located in the single module test_marionette_runner.py,
into modules that each test a specific class or group of
similar classes. This will make it easier to add and locate
new tests in the future, based on the class they are testing.

The new module structure within tests/harness_unit/ is:

* test_marionette_harness.py - tests for MarionetteHarness and
  the command-line interface

* test_marionette_arguments.py - tests for MarionetteArguments
  (future: BaseMarionetteArguments and RemoteMarionetteArguments)

* test_marionette_runner.py - tests for MarionetteTestRunner
  and BaseMarionetteTestRunner (future: MarionetteTextTestRunner)

* test_marionette_test_result.py - tests for MarionetteTestResult
  (future: MarionetteTest)

* conftest.py - pytest fixtures used in multiple modules
  (fixtures specific to a single module are defined in that module)

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

Comment 2

[Anjana Vakil (:vakila)]

Attachment: Bug 1291796 - Minor fixes to harness unit tests;

In test_marionette_runner.py, fix pytest warning raised by importing
TestManifest class in global namespace.

In test_marionette_arguments.py, improve readability by
shortening/changing some names and removing unnecessary
comments (not needed as code is self-explanatory).

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

Comment 3

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

Comment on attachment 8785294 [details]
Bug 1291796 - Split Marionette harness tests by class;

https://reviewboard.mozilla.org/r/74550/#review72522

Comment 4

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

Comment on attachment 8785295 [details]
Bug 1291796 - Minor fixes to harness unit tests;

https://reviewboard.mozilla.org/r/74552/#review72524

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:81
(Diff revision 1)
>  def manifest_fixture(request):
>      '''
>      Fixture for the contents of mock_manifest, where a manifest
>      can include enabled tests, disabled tests, both, or neither (empty)
>      '''
> +    from manifestparser import TestManifest

Since this is parametrized, lets avoid running the import over and over. If you just import manifestparser once at the top and then refer to manifestparser.TestManifest in the test(s), the pytest warning should go away.

Comment 5

[Anjana Vakil (:vakila)]

Comment on attachment 8785294 [details]
Bug 1291796 - Split Marionette harness tests by class;

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

Comment 6

[Anjana Vakil (:vakila)]

Comment on attachment 8785295 [details]
Bug 1291796 - Minor fixes to harness unit tests;

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

Comment 7

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

Comment on attachment 8785295 [details]
Bug 1291796 - Minor fixes to harness unit tests;

https://reviewboard.mozilla.org/r/74552/#review72772

Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a81df9a2129
Split Marionette harness tests by class; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/0d7beb4dc862
Minor fixes to harness unit tests; r=maja_zf

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/6a81df9a2129
https://hg.mozilla.org/mozilla-central/rev/0d7beb4dc862