[webkit-reviews] review granted: [Bug 48280] new-run-webkit-tests: refactor config, filesystem info out of port/base : [Attachment 73034] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 48280: new-run-webkit-tests: refactor config, filesystem info out of
port/base
https://bugs.webkit.org/show_bug.cgi?id=48280

Attachment 73034: Patch
https://bugs.webkit.org/attachment.cgi?id=73034&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73034&action=review

Thank you for the patch.  Looks good.  I appreciate you breaking this up into
smaller pieces.  Smaller still would be even better, but this was totally
manageable.

> WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:55
> +    def isdir(self, path):
> +	   # FIXME: Implement :)
> +	   raise NotImplementedError

Seems we should just leave thse undefined, and we'd get the same behavior for 4
fewer lines in teh file.

> WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:62
> +    def listdir(self, path):
> +	   # FIXME: Implement :)
> +	   raise NotImplementedError

Same here.

> WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:74
> +		   raise OSError()

Do we need to have code=EEXISTS or whatever it is?

> WebKitTools/Scripts/webkitpy/layout_tests/port/config.py:87
> +	   ed)turns whether build was successful or is up-to-date.

typo.

> WebKitTools/Scripts/webkitpy/layout_tests/port/config_unittest.py:46
> +    def make_config(self, output='', files={}, exit_code=0):
> +	   e = executive_mock.MockExecutive(output=output, exit_code=exit_code)

> +	   fs = filesystem_mock.MockFileSystem(files)
> +	   return config.Config(e, fs)

Thank you for the code sharing.

> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:356
>      def _build_path(self, *comps):
> -	   if not self._cached_build_root:
> -	       self._cached_build_root = self._webkit_build_directory([
> -		   "--configuration",
> -		  
self.flag_from_configuration(self.get_option('configuration')),
> -	       ])
> -	   return os.path.join(self._cached_build_root, *comps)
> +	   return self._filesystem.join(self._config.build_directory(
> +	       self.get_option('configuration')), *comps)

Sigh.  I'm about to post a patch to re-write this as part of bug 49051.  I'll
deal with the conflicts. :)