Removing flake in TemporaryHTTPServer.

Removing flake in TemporaryHTTPServer.

Note that I think ReplayServer (wpr_server.py) could still potentially be flaky since it makes use of webpagereplay.py (which hardcodes ports). We may eventually want to subclass it or something to make it get dynamic ports like we do elsewhere. In fact, I ran into the problem today while working on this patch - if a test doesn't exit properly, the server may be left up blocking these hardcoded ports. I've made a new bug for this, since I think it's a bit out of the scope of this issue (crbug.com/157459).

BUG=154229
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168902

[hartmanng, 2012-10-23 21:56:13]

This should fix all 3 of the flake sources pointed out in
http://code.google.com/p/chromium/issues/detail?id=154229 .

[dtu, 2012-10-23 22:15:04]

https://chromiumcodereview.appspot.com/11231087/diff/1/tools/chrome_remote_co...
File tools/chrome_remote_control/chrome_remote_control/cros_interface.py
(right):

https://chromiumcodereview.appspot.com/11231087/diff/1/tools/chrome_remote_co...
tools/chrome_remote_control/chrome_remote_control/cros_interface.py:371: def
RunRemotePython(self, pycmd):
Only test images have Python. Ideally we'd like CRC to run on a CrOS machine out
of the box, but we haven't figured out yet whether that's doable, since we also
do things like SSH and SCP. nduca?

[hartmanng, 2012-11-05 21:12:29]

nduca, please take a look

[nduca, 2012-11-06 04:28:01]

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/cros_browser_backend.py
(right):

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/cros_browser_backend.py:174:
try:
ssh to the target and run wget with some args. It'll fail if the server isn't
up.

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/cros_browser_backend.py:175:
tmp = socket.socket()
There should be a cros-interface method here... IsHTTPServerRunningOnPort or
something. IsServerUp should just call that.

And, unit test! :)

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/cros_interface.py
(right):

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/cros_interface.py:373: 
GetRemotePort should come with a unit test.

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/wpr_server.py (right):

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/wpr_server.py:31:
self._http_forwarder = browser_backend.CreateForwarder(
Add a // Note: This can cause flake if blahblah. See crbug.com/xxxx for details.

[nduca, 2012-11-06 04:28:14]

dev images have ssh and scp, and the have /bin/sh, netstat (/usr/sbin? i forget
where), and wget. These should be sufficient for resolving the flake.

You should be able to get a free port by ssh'ing to the target, doing netstat
-l, pulling that result over to the client, parsingit and picking something
that's free. Consult with @sleffler if you need ideas, he knows this in and out.

[hartmanng, 2012-11-19 18:49:13]

I think this should work better, the only remote commands I added are netstat
and wget. And added unit tests :)

PTAL

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/cros_browser_backend.py
(right):

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/cros_browser_backend.py:174:
try:
On 2012/11/06 04:28:01, nduca wrote:
> ssh to the target and run wget with some args. It'll fail if the server isn't
> up.

Done.

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/cros_browser_backend.py:175:
tmp = socket.socket()
On 2012/11/06 04:28:01, nduca wrote:
> There should be a cros-interface method here... IsHTTPServerRunningOnPort or
> something. IsServerUp should just call that.
> 
> And, unit test! :)

Done.

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/cros_interface.py
(right):

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/cros_interface.py:371: def
RunRemotePython(self, pycmd):
On 2012/10/23 22:15:04, Dave Tu wrote:
> Only test images have Python. Ideally we'd like CRC to run on a CrOS machine
out
> of the box, but we haven't figured out yet whether that's doable, since we
also
> do things like SSH and SCP. nduca?

Done.

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/cros_interface.py:373: 
On 2012/11/06 04:28:01, nduca wrote:
> GetRemotePort should come with a unit test.

Done.

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/wpr_server.py (right):

http://codereview.chromium.org/11231087/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/wpr_server.py:31:
self._http_forwarder = browser_backend.CreateForwarder(
On 2012/11/06 04:28:01, nduca wrote:
> Add a // Note: This can cause flake if blahblah. See crbug.com/xxxx for
details.

Done.

[nduca, 2012-11-19 20:20:04]

Looks like the right approach. Some comments...

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/cros_browser_backend.py (left):

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
tools/telemetry/telemetry/cros_browser_backend.py:133: self._host_port =
ports[0][0]
what guarantees that the port is available on both sides?

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/cros_interface.py (right):

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
tools/telemetry/telemetry/cros_interface.py:385: return sorted(ports_in_use)[-1]
+ 1
Are you sure this'll work? In particular, is this only considering listening
sockets? What if the socket is connected already...

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/cros_interface_unittest.py (right):

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
tools/telemetry/telemetry/cros_interface_unittest.py:15: # CrOSInterface
subclass which runs commands locally instead of over ssh.
I'd rather you use the real cros interface and simply skip the test if you dont
have a cros device active.

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
tools/telemetry/telemetry/cros_interface_unittest.py:122: cri =
LocalCrOSInterface()
I dont get what this test is doing.... you're certainly geting a remote port,
but your socket calls are local so they're a nop. To test this, you need to get
a remote port, create a server locally, forward that server to that remote port,
then get another remote port and verify that the new remote port isnt the old
one. That may be a pain test to write, so if it is, I'm okay if you wanna not
test this.

http://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry/...
tools/telemetry/telemetry/cros_interface_unittest.py:133: sock.listen(0)
Same comment as before --- rather you start a server and do the IsRunning check
on the real other side.

[hartmanng, 2012-11-20 16:37:04]

PTAL

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
File tools/telemetry/telemetry/cros_browser_backend.py (left):

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_browser_backend.py:133: self._host_port =
ports[0][0]
On 2012/11/19 20:20:04, nduca wrote:
> what guarantees that the port is available on both sides?

Ports are passed into this function in pairs, and in general it's up to the
caller to pass in correct ports (for example, in the CrOSBrowserBackend
constructor, we pass in the known remote_debugging_port to forward to). I didn't
realize CrOSBrowserBackend.CreateForwarder was just passing in the local port
twice though, so I fixed that. If you don't have a known remote port, passing in
None will now cause the SSHForwarder constructor to get one properly using
cri.GetRemotePort().

Note that, for now, wpr_server needs to pass in a static remote port, which is a
source of flake (and I'll try to address it in a separate patch).

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
File tools/telemetry/telemetry/cros_interface.py (right):

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_interface.py:385: return sorted(ports_in_use)[-1]
+ 1
On 2012/11/19 20:20:04, nduca wrote:
> Are you sure this'll work? In particular, is this only considering listening
> sockets? What if the socket is connected already...

Yep, changed to 'netstat -a' instead of 'netstat -l', that should list all
connected and listening sockets.

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
File tools/telemetry/telemetry/cros_interface_unittest.py (right):

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_interface_unittest.py:15: # CrOSInterface
subclass which runs commands locally instead of over ssh.
On 2012/11/19 20:20:04, nduca wrote:
> I'd rather you use the real cros interface and simply skip the test if you
dont
> have a cros device active.

Done.

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_interface_unittest.py:122: cri =
LocalCrOSInterface()
On 2012/11/19 20:20:04, nduca wrote:
> I dont get what this test is doing.... you're certainly geting a remote port,
> but your socket calls are local so they're a nop. To test this, you need to
get
> a remote port, create a server locally, forward that server to that remote
port,
> then get another remote port and verify that the new remote port isnt the old
> one. That may be a pain test to write, so if it is, I'm okay if you wanna not
> test this.

The idea was just to make sure the port GetRemotePort returns isn't already in
use. If it was, sock.bind(('', port)) would raise an exception and the test
would fail.

The new test was a bit of a pain to write, but I think I have something usable
now. The testGetRemotePort and testIsHTTPServerRunningOnPort were fairly similar
and inter-dependent (since it's difficult to reliably test that we can start a
server on a port without GetRemotePort). So I've merged the two tests for now.
It's not quite ideal, but it should certainly tell us if something breaks. Let
me know if it's ok.

https://codereview.chromium.org/11231087/diff/14006/tools/telemetry/telemetry...
tools/telemetry/telemetry/cros_interface_unittest.py:133: sock.listen(0)
On 2012/11/19 20:20:04, nduca wrote:
> Same comment as before --- rather you start a server and do the IsRunning
check
> on the real other side.

Done.

[nduca, 2012-11-20 19:34:00]

lgtm

[commit-bot: I haz the power, 2012-11-20 19:48:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11231087/15005

[commit-bot: I haz the power, 2012-11-20 21:09:26]

Retried try job too often for step(s) browser_tests, content_browsertests

[commit-bot: I haz the power, 2012-11-20 21:15:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11231087/15005

[commit-bot: I haz the power, 2012-11-20 23:00:57]

Change committed as 168902

[marja, 2012-11-22 13:33:40]

This broke some telemetry tests.

E.g.,
ERROR: testFailure
(telemetry.multi_page_benchmark_unittest.MultiPageBenchmarkUnitTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/multi_page_benchmark_unittest.py",
line 73, in testFailure
    all_results = self.RunBenchmark(benchmark, ps)
  File
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/multi_page_benchmark_unittest_base.py",
line 51, in RunBenchmark
    runner.Run(options, possible_browser, benchmark, results)
  File "/media/ssdvol/chromium/src/tools/telemetry/telemetry/page_runner.py",
line 112, in Run
    self._RunPage(options, page, state.tab, test, results)
  File "/media/ssdvol/chromium/src/tools/telemetry/telemetry/page_runner.py",
line 129, in _RunPage
    did_prepare = self.PreparePage(page, tab, page_state, results)
  File "/media/ssdvol/chromium/src/tools/telemetry/telemetry/page_runner.py",
line 193, in PreparePage
    target_side_url = tab.browser.http_server.UrlOf(filename)
  File
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/temporary_http_server.py",
line 69, in UrlOf
    return urlparse.urljoin(self.url, path)
  File
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/temporary_http_server.py",
line 66, in url
    return self._forwarder.url
  File
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/desktop_browser_backend.py",
line 107, in url
    return 'http://localhost:%i' % self._host_port
TypeError: not all arguments converted during string formatting

[hartmanng, 2012-11-22 14:02:41]

On 2012/11/22 13:33:40, marja wrote:
> This broke some telemetry tests.
> 
> E.g.,
> ERROR: testFailure
> (telemetry.multi_page_benchmark_unittest.MultiPageBenchmarkUnitTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File
>
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/multi_page_benchmark_unittest.py",
> line 73, in testFailure
>     all_results = self.RunBenchmark(benchmark, ps)
>   File
>
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/multi_page_benchmark_unittest_base.py",
> line 51, in RunBenchmark
>     runner.Run(options, possible_browser, benchmark, results)
>   File "/media/ssdvol/chromium/src/tools/telemetry/telemetry/page_runner.py",
> line 112, in Run
>     self._RunPage(options, page, state.tab, test, results)
>   File "/media/ssdvol/chromium/src/tools/telemetry/telemetry/page_runner.py",
> line 129, in _RunPage
>     did_prepare = self.PreparePage(page, tab, page_state, results)
>   File "/media/ssdvol/chromium/src/tools/telemetry/telemetry/page_runner.py",
> line 193, in PreparePage
>     target_side_url = tab.browser.http_server.UrlOf(filename)
>   File
>
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/temporary_http_server.py",
> line 69, in UrlOf
>     return urlparse.urljoin(self.url, path)
>   File
>
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/temporary_http_server.py",
> line 66, in url
>     return self._forwarder.url
>   File
>
"/media/ssdvol/chromium/src/tools/telemetry/telemetry/desktop_browser_backend.py",
> line 107, in url
>     return 'http://localhost:%i' % self._host_port
> TypeError: not all arguments converted during string formatting

Sorry about this (and thanks for letting me know). I have a fix up here
https://codereview.chromium.org/11418131