Better test file name validation in BaseMarionetteTestRunner.add_test

RESOLVED FIXED in Firefox 50

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: vakila, Assigned: njasm, Mentored)

Tracking

Version 3
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
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
Reporter

Comment 1

3 years ago
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)
Reporter

Updated

3 years ago
Depends on: 1275269
Reporter

Updated

3 years ago
Assignee: anjanavakil → nobody
Whiteboard: [good first bug][lang=py]
Reporter

Updated

3 years ago
Mentor: anjanavakil, mjzffr
Assignee

Comment 3

3 years ago
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)
Assignee

Updated

3 years ago
Flags: needinfo?(anjanavakil)
Reporter

Comment 4

3 years ago
(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)
Reporter

Updated

3 years ago
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)
Assignee

Comment 6

3 years ago
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)
Assignee

Updated

3 years ago
Attachment #8770740 - Flags: review?(mjzffr)
Assignee

Updated

3 years ago
Attachment #8770870 - Flags: review?(anjanavakil)
Reporter

Comment 8

3 years ago
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.
Reporter

Updated

3 years ago
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)
Assignee

Comment 10

3 years ago
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!
Reporter

Comment 11

3 years ago
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!
Reporter

Comment 12

3 years ago
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!
Reporter

Updated

3 years ago
Attachment #8770870 - Flags: review?(mjzffr)
Attachment #8770870 - Flags: review?(anjanavakil)
Reporter

Comment 13

3 years ago
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.
Assignee

Updated

3 years ago
Attachment #8771150 - Flags: feedback?(anjanavakil)
Reporter

Comment 15

3 years ago
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 :)
Reporter

Updated

3 years ago
Attachment #8771150 - Flags: feedback?(anjanavakil) → feedback+
Assignee

Comment 16

3 years ago
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)
Assignee

Updated

3 years ago
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)
Assignee

Comment 18

3 years ago
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+

Comment 20

3 years ago
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

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3909164b79d8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.