Bug 74566 – webkitpy: finish refactoring port classes to make a host mandatory

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

webkitpy: finish refactoring port classes to make a host mandatory

[Dirk Pranke]

Created attachment 119344 [details]
Patch

[Dirk Pranke]

this is the last (4 of 4) of the patches ... this can't easily be broken up any smaller because of the heavy dependency of the integrationtests and rebaselining tests on how mock filesystems are created and populated.

[Eric Seidel (no email)]

Comment on attachment 119344 [details]
Patch

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

LGTM.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:55
> +from webkitpy.common.system.systemhost_mock import MockSystemHost

I don't think we want to import testing code into production code.  I suspect this import isn't even needed. :)

[Dirk Pranke]

Comment on attachment 119344 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:55
>> +from webkitpy.common.system.systemhost_mock import MockSystemHost
> 
> I don't think we want to import testing code into production code.  I suspect this import isn't even needed. :)

I think you're right and this can be deleted now.

[Dirk Pranke]

Created attachment 119547 [details]
delete unneeded MockSystemHost import

[Eric Seidel (no email)]

Comment on attachment 119547 [details]
delete unneeded MockSystemHost import

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

LGTM.

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:54
> +        if 'executive' in kwargs:
> +            host.executive = kwargs['executive']

I thought you were going with the executive=None route?  Port doesn't care about the executive parameter anymore.

> Tools/Scripts/webkitpy/layout_tests/port/test.py:-306
> -def unit_test_filesystem(files=None):
> -    files = files or {}
> -    fs = MockFileSystem(files, dirs=set(['/mock-checkout']))  # Make sure at least the checkout_root exists as a directory.
> -    add_unit_tests_to_mock_filesystem(fs)
> -    return fs

Yay!

> Tools/Scripts/webkitpy/layout_tests/port/test.py:315
> +        self._expectations_path = LAYOUT_TEST_DIR + '/platform/test/test_expectations.txt'

Probably should be using fs.join, but really doesn't matter. :)

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:404
> +        host = MockHost()
> +        self.assertTrue(passing_run(host=host))
> +        self.assertEquals(host.filesystem.read_text_file('/tmp/layout-test-results/passes/error-stderr.txt'),

I think passing_run() wants to be a method on some helper object which can be created with a Host.  But obviously that's a later change.  Free funtions aren't very pythonic IMO.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:411
> +        tests_run = get_tests_run(['--test-list=%s' % filename], tests_included=True, flatten_batches=True, host=host)

likewise get_tests_run would be on the same object as passing_run.  It would just be run() and then you'd assert on some sort of passed, tests_run, members, etc. :)  Again, not for this patch (and maybe not ever!), just musing.

> Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests_unittest.py:212
> -        filesystem.maybe_make_directory(mock_scm.checkout_root)
> +        filesystem.maybe_make_directory(host.scm().checkout_root)

I'm pretty sure that the default filesystem already includes checkout_root. :)

[Dirk Pranke]

(In reply to comment #6)
> > Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:54
> > +        if 'executive' in kwargs:
> > +            host.executive = kwargs['executive']
> 
> I thought you were going with the executive=None route?  Port doesn't care about the executive parameter anymore.
> 

I fix this in the patch in bug 74562 (which will land before this), but I hadn't rebased this patch against the latest version of that patch; it looks like these lines are bad. Will fix.

> > Tools/Scripts/webkitpy/layout_tests/port/test.py:315
> > +        self._expectations_path = LAYOUT_TEST_DIR + '/platform/test/test_expectations.txt'
> 
> Probably should be using fs.join, but really doesn't matter. :)
>

Perhaps; we pervasively assume string concatenations and os.sep == '/' in this file (which is safe when we have MockHosts); fixing it everywhere would probably be more trouble than it's worth.
 
> > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:404
> > +        host = MockHost()
> > +        self.assertTrue(passing_run(host=host))
> > +        self.assertEquals(host.filesystem.read_text_file('/tmp/layout-test-results/passes/error-stderr.txt'),
> 
> I think passing_run() wants to be a method on some helper object which can be created with a Host.  But obviously that's a later change.  Free funtions aren't very pythonic IMO.

I think you and I disagree on the badness of free functions; I have nothing against them as long as they're pure (Port.factory.get() being my favorite example of you and I disagreeing). That said, you're right in this case a helper object for this set of functions might be cleaner.

> > Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests_unittest.py:212
> > -        filesystem.maybe_make_directory(mock_scm.checkout_root)
> > +        filesystem.maybe_make_directory(host.scm().checkout_root)
> 
> I'm pretty sure that the default filesystem already includes checkout_root. :)

You might be right; I'll double check.

Thanks for the review!

[Dirk Pranke]

Created attachment 119900 [details]
update w/ review feedback

[Dirk Pranke]

Committed r103254: <http://trac.webkit.org/changeset/103254>