Bug 85516 - webkitpy: Use PlatformInfo whenever possible in server_process.py.
: webkitpy: Use PlatformInfo whenever possible in server_process.py.
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-03 11:05 PST by
Modified: 2012-05-03 12:58 PST (History)


Attachments
Patch (3.64 KB, patch)
2012-05-03 11:08 PST, Raphael Kubo da Costa (:rakuco)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2012-05-03 12:26 PST, Raphael Kubo da Costa (:rakuco)
eric: review+
eric: commit‑queue+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-03 11:05:49 PST
webkitpy: Use PlatformInfo whenever possible in server_process.py.
------- Comment #1 From 2012-05-03 11:08:13 PST -------
Created an attachment (id=140051) [details]
Patch
------- Comment #2 From 2012-05-03 11:09:29 PST -------
(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?
------- Comment #3 From 2012-05-03 11:12:15 PST -------
(In reply to comment #2)
> (From update of attachment 140051 [details] [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 From 2012-05-03 11:21:45 PST -------
Then we must not have any testing of ServerProcess?  Otherwise whatever testing we would have would be dependent on the real system?
------- Comment #5 From 2012-05-03 11:32:59 PST -------
(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 From 2012-05-03 11:54:03 PST -------
(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 From 2012-05-03 12:26:56 PST -------
Created an attachment (id=140067) [details]
Patch
------- Comment #8 From 2012-05-03 12:28:05 PST -------
(In reply to comment #7)
> Created an attachment (id=140067) [details] [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 From 2012-05-03 12:55:41 PST -------
(From update of attachment 140067 [details])
LGTM.
------- Comment #10 From 2012-05-03 12:58:47 PST -------
Committed r115999: <http://trac.webkit.org/changeset/115999>