1291643 - Add harness test to verify cleanup after adding empty manifest

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Anjana Vakil (:vakila)]

+++ This bug was initially created as a clone of Bug #1287591 +++

Per Bug #1287591, add a harness unit test to make sure that the application doesn't get left open if we try to run a manifest with no active tests (e.g. an empty manifest or when using with a filter that selects no tests). 

Per the patch on that bug, `self.marionette` and `self.httpd` should get reset to `None` after trying to call `run_tests` with such a manifest.

Comment 1

[Anjana Vakil (:vakila)]

Attachment: Bug 1291643 - Test cleanup after running (possibly empty) manifest;

Add a test to make sure that if an empty manifest, or one with no active tests,
is provided, Marionette/Firefox are shut down as expected (previous behavior
was that the Marionette cleanup was never reached in this scenario).

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

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8777265 [details]
Bug 1291643 - Test cleanup after running (possibly empty) manifest;

https://reviewboard.mozilla.org/r/68858/#review65992

I'm not an expert of pytest yet, so take my feedback with a grain of salt. Maybe you can ask someone else for a final review until Maja is back. For now I added some feedback which you might find helpful.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:405
(Diff revision 1)
>              assert test['expected'] == 'pass'
>  
>  
> +def test_add_empty_manifest(mock_runner):
> +    mock_runner._appName = 'fake_app'
> +    mani_name, mani_tests = 'fake_empty_manifest.ini', []

Please do not initialize both values in the same line. Do it like in other tests in two lines.

nit: I wouldn't use shortcut names for manifest here.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:414
(Diff revision 1)
> +        with patch('marionette.runner.base.mozversion.get_version'):
> +            with pytest.raises(Exception) as exc:
> +                mock_runner.run_tests([mani_name])
> +    assert "no tests to run" in exc.value.message
> +    assert mock_runner.marionette is None
> +    assert mock_runner.httpd is None

I think we should also generally test this for normal test execution, and not only for this special case.

Should we also test that the pass/fail/skip lines are printed, except for KeyboardInterupt?

Comment 3

[Anjana Vakil (:vakila)]

Comment on attachment 8777265 [details]
Bug 1291643 - Test cleanup after running (possibly empty) manifest;

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

Comment 4

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/68858/#review65992

I've added two rather repetitive tests here; I'll remove the duplication and try to make all the manifest-related tests a bit cleaner for Bug 1292300.

> I think we should also generally test this for normal test execution, and not only for this special case.
> 
> Should we also test that the pass/fail/skip lines are printed, except for KeyboardInterupt?

I added a test for a non-empty manifest. As for the pass/fail/skip lines, I haven't added a test for that yet as I'm not exactly sure how that would work. If it's necessary, let me know and I'll figure it out. 

Another case we could test is a manifest where 1+ of the specified files don't exist; in this case `add_tests` throws an error and as far as I can tell the `cleanup` method isn't called in that case, i.e. Marionette isn't shut down. Not sure if that behavior is desired or not, but I'm guessing not - perhaps we might want to consider wrapping the `add_tests` call in a try/catch to make sure we call `cleanup`?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If we don't cleanup the Marionette object, and even leave the application open it's definitely a thing to get fixed. So please make a note for it or create a bug, so we don't forget about it. Great spot btw!

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8777265 [details]
Bug 1291643 - Test cleanup after running (possibly empty) manifest;

https://reviewboard.mozilla.org/r/68858/#review67214

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:410
(Diff revision 2)
> +    with patch.multiple('marionette.runner.base.TestManifest',
> +                        read=DEFAULT, active_tests=DEFAULT) as mocks:
> +        mocks['active_tests'].return_value = [{'expected':'pass', 'path':'test_something.py'}]
> +        with patch('marionette.runner.base.mozversion.get_version'):
> +            with patch('marionette.runner.base.os.path.exists', return_value=True):
> +                mock_runner.run_tests(['fake_manifest.ini'])

Using decorators should make the test way more readable given that you don't have that many nested blocks:

`
@patch('marionette.runner.base.mozversion.get_version')
@patch('marionette.runner.base.os.path.exists', ..)
@patch.multiple('marionette.runner.base.TestManifest', ..)
test_cleanup_with_manifest(mock_runner, version, exists, active_tests, read):
`

But its just a nice to have, even maybe for later.

Comment 7

[Anjana Vakil (:vakila)]

Comment on attachment 8777265 [details]
Bug 1291643 - Test cleanup after running (possibly empty) manifest;

https://reviewboard.mozilla.org/r/68858/#review65992

> I added a test for a non-empty manifest. As for the pass/fail/skip lines, I haven't added a test for that yet as I'm not exactly sure how that would work. If it's necessary, let me know and I'll figure it out. 
> 
> Another case we could test is a manifest where 1+ of the specified files don't exist; in this case `add_tests` throws an error and as far as I can tell the `cleanup` method isn't called in that case, i.e. Marionette isn't shut down. Not sure if that behavior is desired or not, but I'm guessing not - perhaps we might want to consider wrapping the `add_tests` call in a try/catch to make sure we call `cleanup`?

So, after doing some testing it seems that the `add_tests` thing isn't actually an issue - the application doesn't seem to get left open after all. So I guess it can just stay as it is.

Comment 8

[Anjana Vakil (:vakila)]

Comment on attachment 8777265 [details]
Bug 1291643 - Test cleanup after running (possibly empty) manifest;

https://reviewboard.mozilla.org/r/68858/#review67214

> Using decorators should make the test way more readable given that you don't have that many nested blocks:
> 
> `
> @patch('marionette.runner.base.mozversion.get_version')
> @patch('marionette.runner.base.os.path.exists', ..)
> @patch.multiple('marionette.runner.base.TestManifest', ..)
> test_cleanup_with_manifest(mock_runner, version, exists, active_tests, read):
> `
> 
> But its just a nice to have, even maybe for later.

Indeed, that would be way better - planning to do that or something similar in Bug 1292300.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5179814c12b1
Test cleanup after running (possibly empty) manifest; r=whimboo

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5179814c12b1