Summary: | webkitpy: Use PlatformInfo whenever possible in server_process.py. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Raphael Kubo da Costa (:rakuco) <rakuco> | ||||||
Component: | Tools / Tests | Assignee: | Raphael Kubo da Costa (:rakuco) <rakuco> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dpranke, eric, ojan, tony, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Raphael Kubo da Costa (:rakuco)
2012-05-03 11:05:49 PDT
Created attachment 140051 [details]
Patch
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?
(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 :-) Then we must not have any testing of ServerProcess? Otherwise whatever testing we would have would be dependent on the real system? (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? (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! Created attachment 140067 [details]
Patch
(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 on attachment 140067 [details]
Patch
LGTM.
Committed r115999: <http://trac.webkit.org/changeset/115999> |