Bug 85516 – webkitpy: Use PlatformInfo whenever possible in server_process.py.

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

[Raphael Kubo da Costa (:rakuco)]

webkitpy: Use PlatformInfo whenever possible in server_process.py.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 140051 [details]
Patch

[Eric Seidel (no email)]

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?

[Raphael Kubo da Costa (:rakuco)]

(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 :-)

[Eric Seidel (no email)]

Then we must not have any testing of ServerProcess?  Otherwise whatever testing we would have would be dependent on the real system?

[Raphael Kubo da Costa (:rakuco)]

(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?

[Dirk Pranke]

(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!

[Raphael Kubo da Costa (:rakuco)]

Created attachment 140067 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

(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.

[Eric Seidel (no email)]

Comment on attachment 140067 [details]
Patch

LGTM.

[Raphael Kubo da Costa (:rakuco)]

Committed r115999: <http://trac.webkit.org/changeset/115999>