Bug 79737 - nrwt: implement simple 'virtual test suite' support
Summary: nrwt: implement simple 'virtual test suite' support
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: 79736
Blocks: 79431
  Show dependency treegraph
 
Reported: 2012-02-27 19:43 PST by Dirk Pranke
Modified: 2012-03-20 00:52 PDT (History)
3 users (show)

See Also:


Attachments
Patch (13.20 KB, patch)
2012-02-27 19:50 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback (12.90 KB, patch)
2012-02-29 12:56 PST, 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-02-27 19:43:13 PST
nrwt: implement simple 'virtual test suite' support
Comment 1 Dirk Pranke 2012-02-27 19:50:18 PST
Created attachment 129173 [details]
Patch
Comment 2 Adam Barth 2012-02-27 23:01:55 PST
Comment on attachment 129173 [details]
Patch

This looks like a good approach.  I wonder if we can put some fo this logic outside of base.py.  That file is kind of growing out of control.
Comment 3 Dirk Pranke 2012-02-28 13:03:14 PST
(In reply to comment #2)
> (From update of attachment 129173 [details])
> This looks like a good approach.  I wonder if we can put some fo this logic outside of base.py.  That file is kind of growing out of control.

As we discussed on IRC on Monday, I think it makes sense to move all of the logic for mapping tests to the filesystem out of base into some sort of 'finder' or 'database' class. I decided to not do that until after I had these patches reviewed, just to avoid confusing things, though.

My current vision for base.py (and the port classes generally) is that they should be composite objects that either assemble the desired behavior from other components (like the http server and the test dabase, above) or just declare preferences (worker_model, default_child_processes, etc.)
Comment 4 Adam Barth 2012-02-28 13:43:10 PST
That sounds like a good plan.  I'll read the patch again now.
Comment 5 Adam Barth 2012-02-28 13:47:21 PST
Comment on attachment 129173 [details]
Patch

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

Thanks!

> Tools/Scripts/webkitpy/layout_tests/port/base.py:1061
> +        class VirtualTestSuite(object):

Can we move this declaration to the top-level?  It seems like folks should just call the constructor rather than calling the virtual_suite method.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:1074
> +        if self._populated_virtual_test_suites is None:

Should we make this function @memoized?
Comment 6 Dirk Pranke 2012-02-28 14:43:04 PST
Comment on attachment 129173 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:1061
>> +        class VirtualTestSuite(object):
> 
> Can we move this declaration to the top-level?  It seems like folks should just call the constructor rather than calling the virtual_suite method.

I actually thought having a member function that constructed the object provided better encapsulation and saved subclasses from having to import another symbol, but I don't feel strongly about it, so I will move this to file scope.

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:1074
>> +        if self._populated_virtual_test_suites is None:
> 
> Should we make this function @memoized?

I'm not as big a fan of using @memoized on functions that don't take any arguments, but I can add it.
Comment 7 Dirk Pranke 2012-02-29 12:56:33 PST
Created attachment 129495 [details]
update w/ review feedback
Comment 8 Dirk Pranke 2012-02-29 14:43:35 PST
Committed r109266: <http://trac.webkit.org/changeset/109266>
Comment 9 Eric Seidel (no email) 2012-03-20 00:52:44 PDT
Comment on attachment 129495 [details]
update w/ review feedback

Cleared review? from attachment 129495 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).