Bug 92693 - nrwt: split test-finding code out from manager.py
Summary: nrwt: split test-finding code out from manager.py
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 89267 92694
  Show dependency treegraph
 
Reported: 2012-07-30 17:10 PDT by Dirk Pranke
Modified: 2012-07-30 18:02 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.29 KB, patch)
2012-07-30 17:11 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update names, move strip_comments after review feedback (10.07 KB, patch)
2012-07-30 17:56 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>