Closed Bug 1279005 Opened 8 years ago Closed 8 years ago

Better test file name validation in BaseMarionetteTestRunner.add_test

Categories

(Remote Protocol :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: vakila, Assigned: njasm, Mentored)

References

Details

(Whiteboard: [good first bug][lang=py])

Attachments

(1 file, 2 obsolete files)

Test files passed to Marionette should have valid filenames (e.g. "test_something.py" or "testSomething.js").

Currently, `BaseMarionetteTestRunner.add_test` validates filenames only in the case where a directory is specified as the test path [1]. If a manifest or test module is provided, file names are not checked in `add_test`, but rather the invalidly named tests are added and then validated later in `run_tests` [2].

It would be better to validate filenames once, immediately before an individual test is appended to `self.tests` [3]. In that case, the validation in [1] and [2] could be removed, as only valid test files will be added.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#892-893
[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#786-793
[3] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#939
One issue I see with this, however, is how to notify the user if a test has an invalid file name. We could raise an exception as in [2] above, but if we remove the filename validation in the directory case ([1]), then we'll attempt to add lots of files (e.g. ".pyc"s) that we wouldn't want to throw an error on. Even if we just e.g. logged a warning instead of raising, that would probably be way too much output.

Perhaps we should keep the filename filtering in [1], so that we still only attempt to add valid files in the directory case, but also raise an exception at [3] to catch bad filenames in the other two cases (test module or manifest)?
Flags: needinfo?(mjzffr)
Based on this and previous discussion:
* The directory filtering should stay as is
* Validating all the files after add_test allows us to raise one exception that lists all the invalid filenames, so let's keep that as is.

You could define a boolean "is_valid" function and call it in both cases -- at least that avoids repetition.
Flags: needinfo?(mjzffr)
Depends on: 1275269
Assignee: anjanavakil → nobody
Whiteboard: [good first bug][lang=py]
Mentor: anjanavakil, mjzffr
Hi, I would like to take this bug with your help.

According to the comments and what i've seen in the code, what we wanna do is:
- Maintain behavior/logic of both function [1] and [2], avoiding duplication by extracting to a new function the code that validates if a test filename is valid.

If my assumption is correct, we could create a function like,

def _is_filename_valid(self, filename):
        pattern = "^test(((_.+?)+?\.((py)|(js)))|(([A-Z].*?)+?\.js))$"
        return re.search(pattern, filename)

and would refactor line [3] from,

elif (filename.startswith('test_') and
                        (filename.endswith('.py') or filename.endswith('.js'))):

to,

elif _is_filename_valid(filename):

and removing this four lines [4], and refactoring the [5] line to,

invalid_tests = [t['filepath'] for t in self.tests if not _is_filename_valid(t['filepath'])]

would this be a good approach?

My only problem with this approach, is that now we're noting compiling the regex, and since i don't know python nor the mozilla tests code, I don't know if I'm introducing here a bottleneck.


[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#947
[2] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#822
[3] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#958-959
[4] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#826-829
[5] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#830
Flags: needinfo?(mjzffr)
Flags: needinfo?(anjanavakil)
(In reply to Nelson João Morais (:njasm) from comment #3)
> would this be a good approach?

Yes, that's basically what I would do! 

> My only problem with this approach, is that now we're noting compiling the
> regex, and since i don't know python nor the mozilla tests code, I don't
> know if I'm introducing here a bottleneck.

As Maja pointed out on another bug [1], it's more efficient to compile the regex outside of the `_is_valid_filename` method. I'd probably make it a class variable, myself. But perhaps that has other drawbacks that I'm not aware of - maybe Maja has thoughts?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1275269#c54
Flags: needinfo?(anjanavakil)
Assignee: nobody → njmorais
You could make the compiled regex a `@property` of the class so that it's compiled the first time you ask for it.

https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/testing/marionette/harness/marionette/runner/base.py#615-623
Flags: needinfo?(mjzffr)
ok, here's first patch for review based on the comments..

you guys see anything we can/should make better?
Attachment #8770740 - Flags: review?(anjanavakil)
Attachment #8770740 - Flags: review?(mjzffr)
Attachment #8770870 - Flags: review?(anjanavakil)
https://reviewboard.mozilla.org/r/64200/#review61234

Thanks for the patch! It looks good to me, just one thought about `_is_filename_valid` (see line comment). Let's see what Maja thinks as well.

::: testing/marionette/harness/marionette/runner/base.py:842
(Diff revision 1)
>                              "'test_something.py', 'test_something.js', or 'testSomething.js'."
>                              " Invalid test names:\n  %s"
>                              % '\n  '.join(invalid_tests))
>  
> +    def _is_filename_valid(self, filename, is_filepath=False):
> +        if is_filepath:

Have I understood correctly that `is_filepath` is meant to distinguish between e.g. "/some/path/to/file.ext" and "file.ext"? As far as I know, `os.path.basename` will return the same thing ("file.ext") for either of those - IMHO it might be a tiny bit more readable if we drop `is_filepath` and just always use the basename.
Attachment #8770870 - Flags: review?(mjzffr)
Comment on attachment 8770740 [details] [diff] [review]
bug_1279005_Better_filename_validation.patch1

Thanks for the patch! I'm hiding the splinter review request since I'm only going to look at the MozReview request.
Attachment #8770740 - Attachment is obsolete: true
Attachment #8770740 - Flags: review?(mjzffr)
Attachment #8770740 - Flags: review?(anjanavakil)
https://reviewboard.mozilla.org/r/64200/#review61234

> Have I understood correctly that `is_filepath` is meant to distinguish between e.g. "/some/path/to/file.ext" and "file.ext"? As far as I know, `os.path.basename` will return the same thing ("file.ext") for either of those - IMHO it might be a tiny bit more readable if we drop `is_filepath` and just always use the basename.

Yes, you did. Your suggestion makes perfect sense. I'll change that later today. thanks for the review!
https://reviewboard.mozilla.org/r/64200/#review61234

> Yes, you did. Your suggestion makes perfect sense. I'll change that later today. thanks for the review!

OK sounds good!
https://reviewboard.mozilla.org/r/64200/#review61234

One other thing, in your commit message(s) could you please provide a [descriptive message body](http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages) that explains what you're changing, in addition to the "Bug X -" first line?  You can look at the messages from recent changes in [this file's history](https://hg.mozilla.org/mozilla-central/log/tip/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py) for some examples. 

Thanks a lot!
Attachment #8770870 - Flags: review?(mjzffr)
Attachment #8770870 - Flags: review?(anjanavakil)
https://reviewboard.mozilla.org/r/64200/#review61234

One other other thing :) - I cleared the review flags for Maja and myself for now, since I found out we should be doing things a little differently - once you've submitted the new patch, please flag me for feedback (f?) on Bugzilla (don't think you can do it from MozReview), and then once I've given f+, flag maja_zf for review (r?). Hope that makes sense, if not just ping us in #automation.
Attachment #8771150 - Flags: feedback?(anjanavakil)
https://reviewboard.mozilla.org/r/64394/#review61540

Looks good to me!

Could you please squash the two commits into one (using `hg histedit`) and update the commit message per my earlier comment? In the squashed message, keep the "MozReview Commit ID" from the first commit (that way MozReview knows that it's the same work even though the changeset hash has changed from the histedit), but drop the MozReview ID from the second commit (I think MozReview expects exactly one ID per changeset). If any of that is unclear just ask on IRC.

Then please flag r?maja_zf and push it back up to review, and per her comments we should be good to go! Thanks :)

::: testing/marionette/harness/marionette/runner/base.py:834
(Diff revision 1)
>  
>      def _add_tests(self, tests):
>          for test in tests:
>              self.add_test(test)
>  
> -        invalid_tests = [t['filepath'] for t in self.tests if not self._is_filename_valid(t['filepath'], True)]
> +        invalid_tests = [t['filepath'] for t in self.tests if not self._is_filename_valid(t['filepath'])]

Great, I think it's clearer this way - thanks :)
Attachment #8771150 - Flags: feedback?(anjanavakil) → feedback+
Comment on attachment 8770870 [details]
Bug 1279005 - Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64200/diff/1-2/
Attachment #8770870 - Attachment description: Bug 1279005 - DRY for test file name validation → Bug 1279005 - Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic.
Attachment #8770870 - Flags: review?(mjzffr)
Attachment #8770870 - Flags: review?(anjanavakil)
Attachment #8771150 - Attachment is obsolete: true
Comment on attachment 8770870 [details]
Bug 1279005 - Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic.;

https://reviewboard.mozilla.org/r/64200/#review61982

This looks good, thank you. Please amend your commit message to end with "; r?maja_zf". Once you do that, I will r+ and land it.
Attachment #8770870 - Flags: review?(mjzffr)
Comment on attachment 8770870 [details]
Bug 1279005 - Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64200/diff/2-3/
Attachment #8770870 - Attachment description: Bug 1279005 - Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic. → Bug 1279005 - Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic.;
Attachment #8770870 - Flags: review?(anjanavakil) → review?(mjzffr)
Comment on attachment 8770870 [details]
Bug 1279005 - Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic.;

https://reviewboard.mozilla.org/r/64200/#review62024
Attachment #8770870 - Flags: review?(mjzffr) → review+
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3909164b79d8
Refactor BaseMarionetteTestRunner: added new method _is_filename_valid() centralizing test's file name validation logic.; r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/3909164b79d8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: