[webkit-reviews] review granted: [Bug 74566] webkitpy: finish refactoring port classes to make a host mandatory : [Attachment 119547] delete unneeded MockSystemHost import

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 74566: webkitpy: finish refactoring port classes to make a host mandatory
https://bugs.webkit.org/show_bug.cgi?id=74566

Attachment 119547: delete unneeded MockSystemHost import
https://bugs.webkit.org/attachment.cgi?id=119547&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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/pass
es/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. :)