Bug 92693 – nrwt: split test-finding code out from manager.py

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: split test-finding code out from manager.py

[Dirk Pranke]

Created attachment 155402 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 155402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155402&action=review

r=me provided the naming nits are addressed.

> Tools/Scripts/webkitpy/layout_tests/controllers/finder.py:38
> +class Finder(object):

"Finder" is a very generic name. I would have named this TestsFinder or TestsEnumerator.

> Tools/Scripts/webkitpy/layout_tests/controllers/finder.py:64
> +    def _read_test_files(self, filenames, test_path_separator):

_read_test_files is obnoxious name for what it does. Maybe _lint_test_files?

> Tools/Scripts/webkitpy/layout_tests/controllers/finder.py:73
> +                    line = test_expectations.strip_comments(line)

Why are we calling a function in test_expectations!?

[Dirk Pranke]

Created attachment 155410 [details]
update names, move strip_comments after review feedback

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 155402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155402&action=review
> 
> r=me provided the naming nits are addressed.
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/finder.py:38
> > +class Finder(object):
> 
> "Finder" is a very generic name. I would have named this TestsFinder or TestsEnumerator.
> 

Thanks for the review!

I've renamed the class to LayoutTestFinder. We have too many kinds of tests for TestFinder to work.

Also, as discussed it would be good to rename the file to layout_test_finder to match the class name, but I'll do that at the end of this series of patches to avoid having to rebase six more patches :).

> > Tools/Scripts/webkitpy/layout_tests/controllers/finder.py:64
> > +    def _read_test_files(self, filenames, test_path_separator):
> 
> _read_test_files is obnoxious name for what it does. Maybe _lint_test_files?
>

As discussed on IRC, this isn't linting anything, this is reading in a text file containing a list of test names (as specified via --test-file) and parsing it. Renamed to _read_test_names_from_file.
 
> > Tools/Scripts/webkitpy/layout_tests/controllers/finder.py:73
> > +                    line = test_expectations.strip_comments(line)
> 
> Why are we calling a function in test_expectations!?

This used to be shared between manager and test_expectations, but apparently they rewrote the calling code in test expectations but didn't bother to move this to manager; I've moved it to Finder now.

[Dirk Pranke]

Committed r124131: <http://trac.webkit.org/changeset/124131>