1276974 - Test BaseMarionetteTestRunner.add_test

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Anjana Vakil (:vakila)]

To make sure that e.g. `.ini` files and directories are handled appropriately.

Comment 1

[Anjana Vakil (:vakila)]

Depends on Bug 1275269 because we might change the functionality of `add_test` to check the validity of test file name (which was previously done by `run_test`)

Comment 2

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

I wonder how this bug is related to bug 1237958?

Comment 3

[Anjana Vakil (:vakila)]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> I wonder how this bug is related to bug 1237958?

Thanks for pointing that out, Henrik! Regardless of whether we choose to make the behavior change per that bug, I'll make sure to test that it's doing what we expect (ignoring/using a contained manifest) if a directory is passed.

Comment 4

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

In reply to: "Indeed. For now I'm just going to check extensions in `_add_tests` (plural)
after they've been added to `self.tests`, for the same reason as before
(reporting multiple bad filenames as a single error), but this is giving me
some ideas for refactoring `add_test` (singular) for Bug 1276974. I think
ultimately it would be good to have the filename-checking and
`self.tests`-populating functionality be separate and self-contained (and
ideally in that order), instead of e.g. checking names in the "directory" case
in `add_test` but other cases after the fact in `_add_tests`; what do you
think?"

Yes, go for it. You could have a checking function that gets used in the populating function, say.

Comment 5

[Anjana Vakil (:vakila)]

Attachment: Bug 1276974 - Add tests for BaseMarionetteTestRunner.add_test;

Test that paths to test modules, directories, and manifest files are
processed correctly by BaseMarionetteTestRunner.add_test.

Test modules should only be added if they have valid
Python or JavaScript test names.
If a directory path is provided, all test files discovered
by walking the dir should be added.
Manifest files should be parsed and tests added/skipped accordingly.

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

Comment 6

[Anjana Vakil (:vakila)]

Once Bug 1237958 lands, I'd also like to test the behavior if a manifest file is found in the specified directory (e.g. that the expected warning is logged).

Comment 7

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

Comment on attachment 8761325 [details]
Bug 1276974 - Add tests for BaseMarionetteTestRunner.add_test;

https://reviewboard.mozilla.org/r/58552/#review55786

Thanks, Anjana. These tests look fine overall. Small comments below.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:298
(Diff revision 1)
>          assert runner.record_crash() == has_crashed
>          _check_crash_counts(has_crashed, runner, runner.marionette)
>  
>  
> +def test_add_test_module(runner):
> +    tests = ['test_something.py', 'testSomething.js']

Could you add a bad test name to the list and a comment indicating that add_test does not validate individual module names?

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:310
(Diff revision 1)
> +        assert {'filepath': test,
> +                'expected': 'pass',
> +                'test_container': None} in runner.tests
> +    assert len(runner.tests) == 2
> +
> +def test_add_test_directory(runner):

I think your commit message is backwards maybe?
"Test modules should only be added if they have valid
Python or JavaScript test names." but `add_test` doesn't validate names when modules are added individually.
"If a directory path is provided, all test files discovered
by walking the dir should be added." but walking the dir only adds files that satisfy certain criteria.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:339
(Diff revision 1)
> +    with patch('marionette.runner.base.TestManifest.read') as man_read:
> +        with patch('marionette.runner.base.TestManifest.active_tests',

Have you considered `patch.multiple`? It might be more succinct. I leave it up to you.

Comment 8

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/58552/#review55786

> I think your commit message is backwards maybe?
> "Test modules should only be added if they have valid
> Python or JavaScript test names." but `add_test` doesn't validate names when modules are added individually.
> "If a directory path is provided, all test files discovered
> by walking the dir should be added." but walking the dir only adds files that satisfy certain criteria.

You're right, I think I was thinking that we'd change the name-validation behavior of `add_test` when I wrote this. In the directory case,  by "test files" I meant "validly named test files", but that was unclear. Will amend.

> Have you considered `patch.multiple`? It might be more succinct. I leave it up to you.

I had decided not to use `patch.multiple` because one of them needed a return value and I didn't know how to do that, but now I've figured it out.

Comment 9

[Anjana Vakil (:vakila)]

Comment on attachment 8761325 [details]
Bug 1276974 - Add tests for BaseMarionetteTestRunner.add_test;

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

Comment 10

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

Comment on attachment 8761325 [details]
Bug 1276974 - Add tests for BaseMarionetteTestRunner.add_test;

https://reviewboard.mozilla.org/r/58552/#review56972

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f852be67d625
Add tests for BaseMarionetteTestRunner.add_test; r=maja_zf

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f852be67d625