Closed Bug 1276974 Opened 5 years ago Closed 5 years ago

Test BaseMarionetteTestRunner.add_test

Categories

(Testing :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: vakila, Assigned: vakila)

References

Details

Attachments

(1 file)

To make sure that e.g. `.ini` files and directories are handled appropriately.
Depends on: 1275269
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.
No longer depends on: 1275269
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/
Attachment #8761325 - Flags: review?(mjzffr)
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).
Depends on: 1237958
Attachment #8761325 - Flags: review?(mjzffr) → review-
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/
Attachment #8761325 - Flags: review- → review?(mjzffr)
Attachment #8761325 - Flags: review?(mjzffr) → review+
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f852be67d625
Add tests for BaseMarionetteTestRunner.add_test; r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/f852be67d625
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.