Closed Bug 1284238 Opened 8 years ago Closed 8 years ago

Test that manifest.active_tests is called with appropriate arguments

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, pi-marionette-runner)

Attachments

(2 files)

Bug 1284238 - Split manifest fixture into non/parametrized versions;
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1284238 - Test that manifest tests are filtered with correct settings;
58 bytes, text/x-review-board-request
impossibus
: review+
Details 
In the `test_marionette_runner.py` harness unit tests for `BaseMarionetteTestRunner.add_test`, add a test that makes sure that when a .ini manifest file is being added, the call to `manifest.active_test` passes the correct arguments, to make sure that tests are filtered appropriately given the runner's settings (e.g. whether "e10s" is enabled or not).
Can probably make use of the new `mock_manifest` feature in the works for Bug 1292300 (Attachment 8779785 [details]) [1] for this.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8779785
Depends on: 1292300
(In reply to Maja Frydrychowicz (:maja_zf) from Bug 1292300 comment #6)
> Comment on attachment 8779785 [details]
> Bug 1292300 - Refactor manifest-related tests with fixtures;
> https://reviewboard.mozilla.org/r/70720/#review69000
> :::
> 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)

Per Maja's comment on Bug 1292300, this assertion from the `test_add_test_manifest` method should get spun off into another test that checks the validity of the arguments passed to the `TestManifest.active_tests()` method.
While I'm at it on this bug I should also fix a pytest-warning resulting from Bug 1292300: 
change `from manifestparser import TestManifest` to `... as Manifest` or similar, because pytest is confused by the `Test*` name and warns "cannot collect test class 'TestManifest' because it has a __init__ constructor".
(In reply to Anjana Vakil (:vakila) from comment #3)
> While I'm at it on this bug I should also fix a pytest-warning... 

Fixing this warning in Bug 1291796 instead
Comment on attachment 8788906 [details]
Bug 1284238 - Split manifest fixture into non/parametrized versions;

https://reviewboard.mozilla.org/r/77232/#review75498

Looks good. Small changes.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:78
(Diff revision 1)
>          expected[attr] = getattr(runner, attr)
>      return expected
>  
>  
> +class ManifestFixture:
> +    def __init__(self, name='mock_maifest',

typo: mock_maifest

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:85
(Diff revision 1)
> +        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
> +        mock_manifest = Mock(spec=manifestparser.TestManifest,
> +                             active_tests=Mock(return_value=tests))
> +        self.mock_manifest = Mock(return_value=mock_manifest)

If you call this `self.manifest_class`, it will make it clearer why its return value is `mock_manifest`.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:89
(Diff revision 1)
> +                             active_tests=Mock(return_value=tests))
> +        self.mock_manifest = Mock(return_value=mock_manifest)
> +        self.__repr__ = lambda: "<ManifestFixture {}>".format(name)
> +
> +@pytest.fixture
> +def mock_manifest():

For consistency, please just call this `manifest`.

Aside: I wish there was an easy way to just pick/use one "version" of a parametrized fixture for a test instead of having to create a separate "simple" fixture like this. I looked it up briefly and didn't find any promising solutions.
Attachment #8788906 - Flags: review?(mjzffr) → review-
Comment on attachment 8788907 [details]
Bug 1284238 - Test that manifest tests are filtered with correct settings;

https://reviewboard.mozilla.org/r/77234/#review75512

The overall test is correct, but I'd like to organize a little differently to simplify.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:317
(Diff revision 1)
> +@pytest.mark.parametrize('app', (None, 'appname'))
> +@pytest.mark.parametrize('test_tags', (None, ['tag', 'tag2']))
> +def test_args_passed_to_manifest(e10s, app, test_tags,
> +                                 mock_runner, mock_manifest, monkeypatch):
> +    monkeypatch.setattr('marionette.runner.base.TestManifest', mock_manifest.mock_manifest)
> +    mock_runner._appName = app

We don't need to parametrize on 'app'; the test can assume that `BaseMarionetteTestRunner.appname` always returns some string. So you can have `mock_runner._appName = 'firefox'` and just check that that string is found in `call_args`.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:323
(Diff revision 1)
> +    with patch('marionette.runner.base.os.path.exists', return_value=True):
> +        mock_runner.add_test(mock_manifest.filepath)
> +    args, kwargs = mock_manifest.mock_manifest().active_tests.call_args

Rather than one large test that runs with all possible parameter combinations, some of which we don't need, I would like to see a few smaller tests with simpler setup and fewer asserts in each: one for basics (fixed args, appinfo, mozinfo), one for e10s, one for test_tags.

To minimize repetition, I think you can put lines 316,323-325 in a helper function that returns call_args. Then each of the 3 proposed tests can do specific setup, call the helper, then do asserts on call_args.
Attachment #8788907 - Flags: review?(mjzffr) → review-
Comment on attachment 8788906 [details]
Bug 1284238 - Split manifest fixture into non/parametrized versions;

https://reviewboard.mozilla.org/r/77232/#review75922

r+wc -- remember to update the commit message. It still refers to mock_manifest.
Attachment #8788906 - Flags: review?(mjzffr) → review+
Comment on attachment 8788907 [details]
Bug 1284238 - Test that manifest tests are filtered with correct settings;

https://reviewboard.mozilla.org/r/77234/#review75930

Looks good, except one question.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py
(Diff revisions 1 - 2)
>      to enable testing runner.run_tests().
>      """
>      runner.driverclass = mock_marionette
>      for attr in ['_set_baseurl', 'run_test_set', '_capabilities']:
>          setattr(runner, attr, Mock())
> -    runner._appName = 'fake_app'

Why not keep `runner._appName = ...` in the mock_runner fixture, instead of moving it? It's safe to say that runner should always have that attribute set, unless there's something I'm not seeing.
Attachment #8788907 - Flags: review?(mjzffr) → review-
Comment on attachment 8788907 [details]
Bug 1284238 - Test that manifest tests are filtered with correct settings;

https://reviewboard.mozilla.org/r/77234/#review75940
Attachment #8788907 - Flags: review?(mjzffr) → review+
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f58548c8be54
Split manifest fixture into non/parametrized versions; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/c0804fb00f70
Test that manifest tests are filtered with correct settings; r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/f58548c8be54
https://hg.mozilla.org/mozilla-central/rev/c0804fb00f70
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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: