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

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`)

I wonder how this bug is related to bug 1237958?

(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.

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.

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/

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 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.

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 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 on attachment 8761325 [details] Bug 1276974 - Add tests for BaseMarionetteTestRunner.add_test; https://reviewboard.mozilla.org/r/58552/#review56972 Thanks!

Pushed by mjzffr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f852be67d625 Add tests for BaseMarionetteTestRunner.add_test; r=maja_zf