Bug 85419 - webkitpy: Remove direct sys.platform usage in run_webkit_tests_integrationtest.
Summary: webkitpy: Remove direct sys.platform usage in run_webkit_tests_integrationtest.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raphael Kubo da Costa (:rakuco)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-02 14:06 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2012-05-02 14:54 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.23 KB, patch)
2012-05-02 14:09 PDT, Raphael Kubo da Costa (:rakuco)
dpranke: review+
Details | Formatted Diff | Diff
Use a SystemHost instead of PlatformInfo (2.46 KB, patch)
2012-05-02 14:35 PDT, Raphael Kubo da Costa (:rakuco)
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2012-05-02 14:06:07 PDT
webkitpy: Remove direct sys.platform usage in run_webkit_tests_integrationtest.
Comment 1 Raphael Kubo da Costa (:rakuco) 2012-05-02 14:09:00 PDT
Created attachment 139884 [details]
Patch
Comment 2 Dirk Pranke 2012-05-02 14:13:26 PDT
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).
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-05-02 14:15:23 PDT
Committed r115887: <http://trac.webkit.org/changeset/115887>
Comment 4 Tony Chang 2012-05-02 14:15:37 PDT
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.
Comment 5 Dirk Pranke 2012-05-02 14:28:40 PDT
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?
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-05-02 14:35:40 PDT
Reopening to attach new patch.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-05-02 14:35:49 PDT
Created attachment 139886 [details]
Use a SystemHost instead of PlatformInfo
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-05-02 14:36:58 PDT
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.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-05-02 14:54:54 PDT
Committed r115894: <http://trac.webkit.org/changeset/115894>