Bug 79737 – nrwt: implement simple 'virtual test suite' support

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt: implement simple 'virtual test suite' support

[Dirk Pranke]

Created attachment 129173 [details]
Patch

[Adam Barth]

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.

[Dirk Pranke]

(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.)

[Adam Barth]

That sounds like a good plan.  I'll read the patch again now.

[Adam Barth]

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?

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 129495 [details]
update w/ review feedback

[Dirk Pranke]

Committed r109266: <http://trac.webkit.org/changeset/109266>

[Eric Seidel (no email)]

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).