[webkit-reviews] review denied: [Bug 90284] [GTK] WebKit test runner ignores all system environment variables : [Attachment 152729] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 17 12:01:44 PDT 2012


Dirk Pranke <dpranke at chromium.org> has denied Xabier Rodríguez Calvar
<calvaris at igalia.com>'s request for review:
Bug 90284: [GTK] WebKit test runner ignores all system environment variables
https://bugs.webkit.org/show_bug.cgi?id=90284

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152729&action=review


> Tools/Scripts/webkitpy/layout_tests/port/base.py:787
> +	   for string_variable in self._options.additional_env_var:

You should probably use self.get_option('additional_env_var', []) here; that
should eliminate most (if not all) of the concerns about having to ensure
additonal_env_var is defined in the options object.

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:62
> +	   return Port(host, config=port_config, options=port_options)

Why don't you need to propagate the other **kwargs ?

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_unittest.py:37
> +from webkitpy.tool.mocktool import MockOptions

nit: I'm trying to move away from using MockOptions. import optparse and use
optparse.Values() instead, since it's a built-in class in the standard library
that does the same thing.

> Tools/Scripts/webkitpy/tool/mocktool.py:51
> +	   self.ensure_value('additional_env_var', [])

don't do this, it's a bad way of letting additional hidden dependencies creep
in. tests that need this value to be set should be explicit about it (yes,
that's kind of a hassle).


More information about the webkit-reviews mailing list