Bug 85516 - webkitpy: Use PlatformInfo whenever possible in server_process.py.
: webkitpy: Use PlatformInfo whenever possible in server_process.py.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Raphael Kubo da Costa (:rakuco)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-03 11:05 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2012-05-03 12:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.64 KB, patch)
2012-05-03 11:08 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2012-05-03 12:26 PDT, Raphael Kubo da Costa (:rakuco)
eric: review+
eric: commit‑queue+
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-03 11:05:49 PDT
webkitpy: Use PlatformInfo whenever possible in server_process.py.
Comment 1 Raphael Kubo da Costa (:rakuco) 2012-05-03 11:08:13 PDT
Created attachment 140051 [details]
Patch
Comment 2 Eric Seidel 2012-05-03 11:09:29 PDT
Comment on attachment 140051 [details]
Patch

It seems callers should pass ServerProcess a host (or at least have that option) instead of it creating a new one. This makes it unmockable.  Was no one using the executive parameter?  Did you run test-webkitpy?
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-05-03 11:12:15 PDT
(In reply to comment #2)
> (From update of attachment 140051 [details])
> It seems callers should pass ServerProcess a host (or at least have that option) instead of it creating a new one. This makes it unmockable.  Was no one using the executive parameter?  Did you run test-webkitpy?

That was my first thought too, but since one was using the executive parameter I didn't see much value in adding it. And yes, I always run test-webkitpy before sending webkitpy patches :-)
Comment 4 Eric Seidel 2012-05-03 11:21:45 PDT
Then we must not have any testing of ServerProcess?  Otherwise whatever testing we would have would be dependent on the real system?
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-05-03 11:32:59 PDT
(In reply to comment #4)
> Then we must not have any testing of ServerProcess?  Otherwise whatever testing we would have would be dependent on the real system?

We do have server_process_unittest.py. These lines the patches replace were indeed depending on the real system, as they were accessing sys.platform directly. What could still be done to improve things further is get the Host object from port_obj in ServerProcess' constructor (it would require some work in the unit tests, though). Would you prefer it that way?
Comment 6 Dirk Pranke 2012-05-03 11:54:03 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Then we must not have any testing of ServerProcess?  Otherwise whatever testing we would have would be dependent on the real system?
> 
> We do have server_process_unittest.py. These lines the patches replace were indeed depending on the real system, as they were accessing sys.platform directly. What could still be done to improve things further is get the Host object from port_obj in ServerProcess' constructor (it would require some work in the unit tests, though). Would you prefer it that way?

We should just get the host from port_obj and fix the tests :) I had noticed yesterday that it looked like the Executive wasn't being used, but I hadn't gotten around to doing the cleanup you're doing, so thanks!
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-05-03 12:26:56 PDT
Created attachment 140067 [details]
Patch
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-05-03 12:28:05 PDT
(In reply to comment #7)
> Created an attachment (id=140067) [details]
> Patch

This new version gets a SystemHost from the Port object passed to ServerProcess' constructor. The tests have been adjusted -- even though they currently don't really call the methods in ServerProcess which depend on the platform, the infrastructure is now there.
Comment 9 Eric Seidel 2012-05-03 12:55:41 PDT
Comment on attachment 140067 [details]
Patch

LGTM.
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-05-03 12:58:47 PDT
Committed r115999: <http://trac.webkit.org/changeset/115999>