Bug 92693

Summary: nrwt: split test-finding code out from manager.py
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ojan, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89267, 92694    
Attachments:
Description Flags
Patch
none
update names, move strip_comments after review feedback none

Description Dirk Pranke 2012-07-30 17:10:03 PDT
nrwt: split test-finding code out from manager.py
Comment 1 Dirk Pranke 2012-07-30 17:11:14 PDT
Created attachment 155402 [details]
Patch
Comment 2 Ryosuke Niwa 2012-07-30 17:31:56 PDT
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!?
Comment 3 Dirk Pranke 2012-07-30 17:56:26 PDT
Created attachment 155410 [details]
update names, move strip_comments after review feedback
Comment 4 Dirk Pranke 2012-07-30 18:00:46 PDT
(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.
Comment 5 Dirk Pranke 2012-07-30 18:02:24 PDT
Committed r124131: <http://trac.webkit.org/changeset/124131>