Constrained Network test does not fail fast under fatal conditions.

Constrained Network test does not fail fast under fatal conditions.

This CL does the following:
- adds a timeout for each perf test, so that if a video does not load the test will time-out. (Currently set to 10 sec for playing event to be fired).
- adds sudo commands to traffic control.
- adds CNS logs to pyauto logs.
- s/kbps/kbit/ where applicable.

BUG=109322
TEST={cns, traffic_control} unit tests, local tests.


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

[shadi, 2012-01-06 22:51:52]

Dale, can you please review these changes?

[DaleCurtis, 2012-01-06 23:19:41]

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_constrained_network_perf.py (right):

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:32: import
urllib2
This should go under threading.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:107: def
_HaveTTPMetric(self, unique_url):
These methods are very similar, you should create a generic _HaveMetric() method
which takes a value name.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:112:
tab_index=tab))
Bad indent. Please run gpylint before publishing.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:162:
self._HaveTTPMetric, args=[unique_url], retry_sleep=1, timeout= 5,
Bad spacing. Again gpylint is your friend here :)

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:166: if self._ttp
>= 0:
Should we log an error? Skip logging of epp, ttp below?

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:188: def
__init__(self, process):
Need blank line above this. gpylint... :)

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:240:
self.tearDown()
This should be called automatically. Did you find it not being called?

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:241:
self.fail('Cannot connect to CNS server.')
Instead of having a new fail here, why not just return False and in the setUp
phase above do something like:

if self._CanAccessServer():
  return

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
File media/tools/constrained_network_server/cns.py (right):

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/cns.py:323:
logging.basicConfig(level=log_level, format='%(message)s')
Need a more descriptive message here. One that is more similar to cherrypy.log.
Since multiple requests will be coming in at once, it will be impossible to
differentiate them w/o more detail.

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/cns.py:326: def _CheckRootAccess():
This is not complete and should instead be in the traffic_control library.

Something like: CheckRequirements() throws an exception if a requirement is
missing.

By not complete I mean you need to check if 'sudo -n [tc|iptables]' passes w/o
input (as mentioned in the ticket).

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
File media/tools/constrained_network_server/traffic_control_test.py (right):

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control_test.py:78: class_detail
= ('class htb 1:%x root prio 0 rate %dKbit ceil %dKbit' %
s/Kbit/kbit/ ?

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
File media/tools/constrained_network_server/traffic_control_unittest.py (right):

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control_unittest.py:128: 'flowid
1:1 match c3500000/ffff0000 at 20')
Can you explain this change? It's way different than the previous version :)

[DaleCurtis, 2012-01-06 23:40:00]

Also, please provide a more detailed description as well as some text for the
TEST= line.

[shadi, 2012-01-07 04:56:19]

Dale, please review? :)

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_constrained_network_perf.py (right):

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:32: import
urllib2
On 2012/01/06 23:19:42, DaleCurtis wrote:
> This should go under threading.

Done.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:107: def
_HaveTTPMetric(self, unique_url):
On 2012/01/06 23:19:42, DaleCurtis wrote:
> These methods are very similar, you should create a generic _HaveMetric()
method
> which takes a value name.

Done.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:112:
tab_index=tab))
On 2012/01/06 23:19:42, DaleCurtis wrote:
> Bad indent. Please run gpylint before publishing.

Done.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:162:
self._HaveTTPMetric, args=[unique_url], retry_sleep=1, timeout= 5,
On 2012/01/06 23:19:42, DaleCurtis wrote:
> Bad spacing. Again gpylint is your friend here :)

Done.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:166: if self._ttp
>= 0:
On 2012/01/06 23:19:42, DaleCurtis wrote:
> Should we log an error? Skip logging of epp, ttp below?

Done.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:188: def
__init__(self, process):
On 2012/01/06 23:19:42, DaleCurtis wrote:
> Need blank line above this. gpylint... :)

Done.

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:240:
self.tearDown()
If setup() fails, tearDown() doesn't get called automatically. 

http://docs.python.org/library/unittest.html#unittest.TestCase.tearDown

On 2012/01/06 23:19:42, DaleCurtis wrote:
> This should be called automatically. Did you find it not being called?

http://codereview.chromium.org/9127009/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:241:
self.fail('Cannot connect to CNS server.')
On 2012/01/06 23:19:42, DaleCurtis wrote:
> Instead of having a new fail here, why not just return False and in the setUp
> phase above do something like:
> 
> if self._CanAccessServer():
>   return

Done.

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
File media/tools/constrained_network_server/cns.py (right):

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/cns.py:323:
logging.basicConfig(level=log_level, format='%(message)s')
cherrypy.log only shows date. I added thread number to the format, the date is
printed automatically.
On 2012/01/06 23:19:42, DaleCurtis wrote:
> Need a more descriptive message here. One that is more similar to
cherrypy.log.
> Since multiple requests will be coming in at once, it will be impossible to
> differentiate them w/o more detail.

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/cns.py:326: def _CheckRootAccess():
Check http://codereview.chromium.org/9125022/
On 2012/01/06 23:19:42, DaleCurtis wrote:
> This is not complete and should instead be in the traffic_control library.
> 
> Something like: CheckRequirements() throws an exception if a requirement is
> missing.
> 
> By not complete I mean you need to check if 'sudo -n [tc|iptables]' passes w/o
> input (as mentioned in the ticket).

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
File media/tools/constrained_network_server/traffic_control_test.py (right):

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control_test.py:78: class_detail
= ('class htb 1:%x root prio 0 rate %dKbit ceil %dKbit' %
They seem both to work.
On 2012/01/06 23:19:42, DaleCurtis wrote:
> s/Kbit/kbit/ ?

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
File media/tools/constrained_network_server/traffic_control_unittest.py (right):

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/traffic_control_unittest.py:128: 'flowid
1:1 match c3500000/ffff0000 at 20')
I updated it with exact output example.

I also added a test for a case pointed out previously by Ami.
On 2012/01/06 23:19:42, DaleCurtis wrote:
> Can you explain this change? It's way different than the previous version :)

[DaleCurtis, 2012-01-09 19:21:19]

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
File media/tools/constrained_network_server/cns.py (right):

http://codereview.chromium.org/9127009/diff/2001/media/tools/constrained_netw...
media/tools/constrained_network_server/cns.py:323:
logging.basicConfig(level=log_level, format='%(message)s')
On 2012/01/07 04:56:19, shadi wrote:
> cherrypy.log only shows date. I added thread number to the format, the date is
> printed automatically.
> On 2012/01/06 23:19:42, DaleCurtis wrote:
> > Need a more descriptive message here. One that is more similar to
> cherrypy.log.
> > Since multiple requests will be coming in at once, it will be impossible to
> > differentiate them w/o more detail.
> 

Ah, I'm thinking of cherrypy.request.log() I think. The logs I've seen show the
request IP and some other information in addition to the timestamp.

We should probably be using that instead to make sure logs can be read easily.

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
File chrome/test/functional/media/media_constrained_network_perf.py (right):

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:109: """Checks if
unique_url page has varriable value ready. Set to < 0 pre-run.
s/varriable/variable.

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:234: else:
No need for else:

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:243:
urllib2.urlopen(test_url)
Have you tested to make sure this actually catches an error? I thought you might
need to do a url.read() or something first.

Either way, shouldn't it be "if urllib2.urlopen(...)" return True. The docs say
it can return None sometimes.

[shadi, 2012-01-09 23:07:36]

I'm moving proper cheerypy.log format to a new issue.

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
File chrome/test/functional/media/media_constrained_network_perf.py (right):

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:109: """Checks if
unique_url page has varriable value ready. Set to < 0 pre-run.
On 2012/01/09 19:21:19, DaleCurtis wrote:
> s/varriable/variable.

Done.

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:234: else:
On 2012/01/09 19:21:19, DaleCurtis wrote:
> No need for else:

Done.

http://codereview.chromium.org/9127009/diff/7001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:243:
urllib2.urlopen(test_url)
I have tested with a file not found or wrong url. If the server returns http
error, urlopen will raise an error.
On 2012/01/09 19:21:19, DaleCurtis wrote:
> Have you tested to make sure this actually catches an error? I thought you
might
> need to do a url.read() or something first.
> 
> Either way, shouldn't it be "if urllib2.urlopen(...)" return True. The docs
say
> it can return None sometimes.

[DaleCurtis, 2012-01-10 00:20:17]

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
File chrome/test/functional/media/media_constrained_network_perf.py (right):

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:242: return not
urllib2.urlopen(test_url) is None
Use "is not None", it's more readable.

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:243: except
BaseException as e:
except Exception, e:

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:244:
logging.error(e)
Use logging.exception.

[shadi, 2012-01-10 00:45:46]

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
File chrome/test/functional/media/media_constrained_network_perf.py (right):

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:242: return not
urllib2.urlopen(test_url) is None
On 2012/01/10 00:20:18, DaleCurtis wrote:
> Use "is not None", it's more readable.

Done.

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:243: except
BaseException as e:
On 2012/01/10 00:20:18, DaleCurtis wrote:
> except Exception, e:

Done.

http://codereview.chromium.org/9127009/diff/9001/chrome/test/functional/media...
chrome/test/functional/media/media_constrained_network_perf.py:244:
logging.error(e)
On 2012/01/10 00:20:18, DaleCurtis wrote:
> Use logging.exception.

Done.

[DaleCurtis, 2012-01-10 00:48:33]

http://codereview.chromium.org/9127009/diff/15001/media/tools/constrained_net...
File media/tools/constrained_network_server/traffic_control_test.py (right):

http://codereview.chromium.org/9127009/diff/15001/media/tools/constrained_net...
media/tools/constrained_network_server/traffic_control_test.py:78: class_detail
= ('class htb 1:%x root prio 0 rate %dKbit ceil %dKbit' %
Why did you change this back?

[shadi, 2012-01-10 00:54:42]

http://codereview.chromium.org/9127009/diff/15001/media/tools/constrained_net...
File media/tools/constrained_network_server/traffic_control_test.py (right):

http://codereview.chromium.org/9127009/diff/15001/media/tools/constrained_net...
media/tools/constrained_network_server/traffic_control_test.py:78: class_detail
= ('class htb 1:%x root prio 0 rate %dKbit ceil %dKbit' %
This is exactly the way tc displays the property of a class. The test would fail
otherwise. For adding a class, kbit or Kbit are both acceptable.
On 2012/01/10 00:48:33, DaleCurtis wrote:
> Why did you change this back?

[DaleCurtis, 2012-01-10 00:55:45]

LGTM, +fischman for commit approval.

[Ami GONE FROM CHROMIUM, 2012-01-10 06:15:10]

LGTM

[commit-bot: I haz the power, 2012-01-10 21:07:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shadi@chromium.org/9127009/15001

[commit-bot: I haz the power, 2012-01-10 22:27:54]

Try job failure for 9127009-15001 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-10 22:36:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shadi@chromium.org/9127009/15001

[commit-bot: I haz the power, 2012-01-11 00:11:50]

Change committed as 117125