Bug 85419 – webkitpy: Remove direct sys.platform usage in run_webkit_tests_integrationtest.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Raphael Kubo da Costa (:rakuco)]

webkitpy: Remove direct sys.platform usage in run_webkit_tests_integrationtest.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 139884 [details]
Patch

[Dirk Pranke]

Comment on attachment 139884 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:268
> +        self.SHOULD_TEST_PROCESSES = not self._platform.is_win()

I probably wouldn't use upper case for this field, since it's not really a constant (it's not set at import-time like it was before).

[Raphael Kubo da Costa (:rakuco)]

Committed r115887: <http://trac.webkit.org/changeset/115887>

[Tony Chang]

Comment on attachment 139884 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:264
> +        self._platform = PlatformInfo(sys, platform, Executive())

Nit: If we make sys and platform have defaults values, we wouldn't have to import the modules everywhere that uses PlatformInfo.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:267
> +        # FIXME: remove this when we fix test-webkitpy to work
> +        # properly on cygwin (bug 63846).

Nit: Capitalize Remove.

[Dirk Pranke]

Comment on attachment 139884 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:264
>> +        self._platform = PlatformInfo(sys, platform, Executive())
> 
> Nit: If we make sys and platform have defaults values, we wouldn't have to import the modules everywhere that uses PlatformInfo.

Good catch. Can you change this to a SystemHost() instead (since that has defaults) and then use self._platform = SystemHost().platform ? (and add a comment as to why this is a real platform and not a mock?

[Raphael Kubo da Costa (:rakuco)]

Reopening to attach new patch.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 139886 [details]
Use a SystemHost instead of PlatformInfo

[Raphael Kubo da Costa (:rakuco)]

I've fixed the capitalization issues in <http://trac.webkit.org/changeset/115888>, and sent another patch to implement Tony and Dirk's suggestion of not using a PlatformInfo directly.

[Raphael Kubo da Costa (:rakuco)]

Committed r115894: <http://trac.webkit.org/changeset/115894>