Fix bug checking forwarding ports. Give better debug message (include protocol).

Fix bug checking forwarding ports. Give better debug message (include protocol).

BUG=414501
Committed: https://crrev.com/5bb07b869cd0e684aec51661fea6736364d21122
Cr-Commit-Position: refs/heads/master@{#296109}

[slamm, 2014-09-16 22:13:31]

slamm@chromium.org changed reviewers:
+ nednguyen@google.com

[chrishenry, 2014-09-16 23:39:48]

chrishenry@google.com changed reviewers:
+ chrishenry@google.com

[chrishenry, 2014-09-16 23:39:49]

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py (right):

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:30: """Check
local and remote ports are equal. Also, can connect to TCP ports.
Summary line should be 1 sentence.

Can you say why they have to be equal?

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:33:
TimeoutException if cannot connect to local TCP ports.
Should this be wrapped?

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:49: def
_WaitForConnect(self, address, timeout):
WaitForConnectionEstablished?

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:51: return not
socket.socket().connect_ex(address)
Do you need to close this? If not, probably want to say why.

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py
(right):

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py:16:
super(TestDoNothingForwarder, self).__init__(port_pairs)
I think you can just have this looks like so:

class Test...(...):
  connected_addresses = []

  def _WaitForConnect(...):
    self.connected_addresses.append(...)

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py:25:
http=forwarders.PortPair(80, 80),
nit: 4-align

[slamm, 2014-09-22 22:03:22]

PTAL.

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py (right):

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:30: """Check
local and remote ports are equal. Also, can connect to TCP ports.
On 2014/09/16 23:39:49, chrishenry wrote:
> Summary line should be 1 sentence.
> 
> Can you say why they have to be equal?

Style guide: "A one line summary of the module or script, terminated by a
period."

I also took "one line" to mean text under 80 chars. Still it works better as one
sentence.

Done.

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:33:
TimeoutException if cannot connect to local TCP ports.
On 2014/09/16 23:39:49, chrishenry wrote:
> Should this be wrapped?

Done.

I added "ConnectError". Would it makes sense to added an "Error" base class for
that. I see that in the Python module, however, I have never seen anyone make
use of base class errors.

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:49: def
_WaitForConnect(self, address, timeout):
On 2014/09/16 23:39:49, chrishenry wrote:
> WaitForConnectionEstablished?

Done.

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:51: return not
socket.socket().connect_ex(address)
On 2014/09/16 23:39:49, chrishenry wrote:
> Do you need to close this? If not, probably want to say why.

It had worked without the close. However, I have wrapped the socket with
contextmanager.closing() which automatically closes it.

Note, Python 3 lets you use a socket in a with-statement:

with socket.socket() as s:
  return s.connect_ex(address) == 0

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py
(right):

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py:16:
super(TestDoNothingForwarder, self).__init__(port_pairs)
With that, connected_addresses does not get cleared between tests.

https://codereview.chromium.org/572793002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py:25:
http=forwarders.PortPair(80, 80),
On 2014/09/16 23:39:49, chrishenry wrote:
> nit: 4-align

Done.

[nednguyen, 2014-09-22 22:27:59]

lgtm

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py (right):

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:27:
"""Initialize DoNothingForwarder.
This doesn't add any useful information. 

Maybe move the documentation of _CheckPortPairs to DoNothingForwarder class doc
(https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments)

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py
(right):

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py:12:
"""Override _WaitForConnect to avoid actual socket connection."""
Maybe better to stub out the socket (see telemetry/unittest/system_stub.py).
It's up to you, I am fine with this.

[chrishenry, 2014-09-22 22:33:48]

lgtm

https://codereview.chromium.org/572793002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py (right):

https://codereview.chromium.org/572793002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:47: for
protocol, port_pair in self._port_pairs._asdict().items():
I don't quite like accessing _dict. Is there really no better way?

[slamm, 2014-09-22 23:30:41]

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py (right):

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:27:
"""Initialize DoNothingForwarder.
On 2014/09/22 22:27:58, nednguyen wrote:
> This doesn't add any useful information. 
> 
> Maybe move the documentation of _CheckPortPairs to DoNothingForwarder class
doc
> (https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments)

Done.

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py
(right):

https://codereview.chromium.org/572793002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py:12:
"""Override _WaitForConnect to avoid actual socket connection."""
On 2014/09/22 22:27:58, nednguyen wrote:
> Maybe better to stub out the socket (see telemetry/unittest/system_stub.py).
> It's up to you, I am fine with this.

Looked at it and felt like it added more complexity than it was worth.

https://codereview.chromium.org/572793002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py (right):

https://codereview.chromium.org/572793002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py:47: for
protocol, port_pair in self._port_pairs._asdict().items():
On 2014/09/22 22:33:48, chrishenry wrote:
> I don't quite like accessing _dict. Is there really no better way?

I added a comment:

    # namedtuple._asdict() is a public method. The method starts with an
    # underscore to avoid conflicts with attribute names. pylint: disable=W0212

[slamm, 2014-09-22 23:35:30]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-09-22 23:36:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/572793002/140001

[commit-bot: I haz the power, 2014-09-23 00:44:37]

Committed patchset #8 (id:140001) as d78c12e2d44b85344b85398152e72cc800ff9211

[commit-bot: I haz the power, 2014-09-23 00:45:14]

Patchset 8 (id:??) landed as
https://crrev.com/5bb07b869cd0e684aec51661fea6736364d21122
Cr-Commit-Position: refs/heads/master@{#296109}