Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8802030:
Introduce new PyAuto test to measure EPT, TTP metrics. (Closed)
|
Created:
9 years ago by DaleCurtis Modified:
9 years ago CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, John Grabowski, Nirnimesh, acolwell+watch_chromium.org, annacc+watch_chromium.org, dennis_jeffrey, anantha, dyu1, Paweł Hajdan Jr., vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, shadi, cmp, imasaki1 Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionIntroduce new PyAuto test to measure EPP, TTP metrics.
Test gathers the EPP and TTP metrics for video playback under
constrained network conditions. The test uses the Constrained Network
Server (CNS) to accomplish this.
- Starts CNS.
- Iterates over test matrix of network constraints.
- Reports results in a manner consumable by chromium.perf_av.
- Stops CNS.
BUG=106257
TEST=Ran test locally.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114071
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 37
Patch Set 3 : Code review fixes. #
Total comments: 20
Patch Set 4 : Code review fixes. #Patch Set 5 : Move roller.webm to another cl. #Patch Set 6 : Fixup www-data path. #
Messages
Total messages: 15 (0 generated)
PTAL. Do I need to talk to someone about getting permission on dancing.webm?
I'm not sure about dancing.webm. Where did you get it from? http://codereview.chromium.org/8802030/diff/10/chrome/test/data/media/html/me... File chrome/test/data/media/html/media_cn.html (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/data/media/html/me... chrome/test/data/media/html/media_cn.html:22: // let the PyAuto controller we haven't recorded these values yet. s/let/indicate/ http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... File chrome/test/functional/media/media_cn_perf.py (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:42: # Test constraints are all possible combination of the above settings. Each Leave 2 blank spaces after every period, ie before 'Each' (See Comments section in http://www.python.org/dev/peps/pep-0008/) http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:43: # tuple must be of the form (Bandwidth, Latency, Packet Loss). s/tuple/Tuple http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:58: # Number of threads to use during testing. TODO(dalecurtis): Should be set on move TODO to next line http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:62: # File name of video to collect metrics for. TODO(dalecurtis): Should be set on move TODO to next line http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:100: """Returns the tab index for the tab belonging to this thread.""" s/thread/url/ ? http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:128: # Listen for magic exit values. s/Listen/Check/ http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:142: # Make the test URL unique so we can figure out our tab index later. why not save the tab index? http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:151: # Wait until the necessary metrics have been collected. Okay to not lock 2 spaces after . http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:152: # here since pyauto.WaitUntil doesn't call into Chrome. but _HaveMetrics calls into chrome.. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:157: # Record results. TODO(dalecurtis): Support reference builds. move todo to next line http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:176: cmd = ['python', os.path.join(pyauto_paths.GetSourceDir(), _CNS_PATH), s/python/sys.executable http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:195: pyauto.PyUITest.tearDown(self) destruct in the opposite order. ie tearDown before CNS Kill http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:202: # PyAuto doesn't support threads, so we synchronize all automation calls. s/synchronize/guard around/ ? http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:209: threads.append(TestWorker(self, tasks, automation_lock, test_url)) Won't having multiple threads affect the numbers you get? http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/p... File chrome/test/functional/media/pyauto_media.py (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/p... chrome/test/functional/media/pyauto_media.py:5: """PyAuto media test base. Handles PyAuto initialization and path setup. Please leave 2 spaces after every period. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/p... chrome/test/functional/media/pyauto_media.py:58: class Main(pyauto_functional.Main): This class is a no-op. It can be replaced with: Main = pyauto_functional.Main
Whoops, that should be roller.webm not dancing.webm. It's a known problematic YouTube HTML5 video: http://www.youtube.com/watch?v=I3i1wNPpAJM http://codereview.chromium.org/8802030/diff/10/chrome/test/data/media/html/me... File chrome/test/data/media/html/media_cn.html (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/data/media/html/me... chrome/test/data/media/html/media_cn.html:22: // let the PyAuto controller we haven't recorded these values yet. On 2011/12/06 22:15:05, Nirnimesh wrote: > s/let/indicate/ Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... File chrome/test/functional/media/media_cn_perf.py (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:42: # Test constraints are all possible combination of the above settings. Each On 2011/12/06 22:15:05, Nirnimesh wrote: > Leave 2 blank spaces after every period, ie before 'Each' > (See Comments section in http://www.python.org/dev/peps/pep-0008/) Ugh, if you insist. In protest, I'd like to point out that it's inconsistent with the majority of the PyAuto code and it's a deprecated style: http://www.slate.com/articles/technology/technology/2011/01/space_invaders.html http://en.wikipedia.org/wiki/Space_(punctuation)#Spaces_between_sentences http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:43: # tuple must be of the form (Bandwidth, Latency, Packet Loss). On 2011/12/06 22:15:05, Nirnimesh wrote: > s/tuple/Tuple Tuple is not a proper noun and this is a continuation of the previous line. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:58: # Number of threads to use during testing. TODO(dalecurtis): Should be set on On 2011/12/06 22:15:05, Nirnimesh wrote: > move TODO to next line Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:62: # File name of video to collect metrics for. TODO(dalecurtis): Should be set on On 2011/12/06 22:15:05, Nirnimesh wrote: > move TODO to next line Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:100: """Returns the tab index for the tab belonging to this thread.""" On 2011/12/06 22:15:05, Nirnimesh wrote: > s/thread/url/ ? Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:128: # Listen for magic exit values. On 2011/12/06 22:15:05, Nirnimesh wrote: > s/Listen/Check/ Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:142: # Make the test URL unique so we can figure out our tab index later. On 2011/12/06 22:15:05, Nirnimesh wrote: > why not save the tab index? When we talked via chat you said the tab_index changes when tabs are closed. Is this not correct? Another thread may have closed a tab in between critical sections. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:151: # Wait until the necessary metrics have been collected. Okay to not lock On 2011/12/06 22:15:05, Nirnimesh wrote: > 2 spaces after . Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:152: # here since pyauto.WaitUntil doesn't call into Chrome. On 2011/12/06 22:15:05, Nirnimesh wrote: > but _HaveMetrics calls into chrome.. ...and makes those calls with the lock. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:157: # Record results. TODO(dalecurtis): Support reference builds. On 2011/12/06 22:15:05, Nirnimesh wrote: > move todo to next line Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:176: cmd = ['python', os.path.join(pyauto_paths.GetSourceDir(), _CNS_PATH), On 2011/12/06 22:15:05, Nirnimesh wrote: > s/python/sys.executable Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:195: pyauto.PyUITest.tearDown(self) On 2011/12/06 22:15:05, Nirnimesh wrote: > destruct in the opposite order. > ie tearDown before CNS Kill Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:202: # PyAuto doesn't support threads, so we synchronize all automation calls. On 2011/12/06 22:15:05, Nirnimesh wrote: > s/synchronize/guard around/ ? Synchronize works fine: http://en.wikipedia.org/wiki/Synchronization_(computer_science)#Thread_or_pro... http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:209: threads.append(TestWorker(self, tasks, automation_lock, test_url)) On 2011/12/06 22:15:05, Nirnimesh wrote: > Won't having multiple threads affect the numbers you get? Not in the limited testing I've done thus far. We're limiting network traffic enough that we should be fine with low numbers of threads. Once the CNS portion is completed I'll do some more experiments. Worst case we just set threads = 1. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/p... File chrome/test/functional/media/pyauto_media.py (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/p... chrome/test/functional/media/pyauto_media.py:5: """PyAuto media test base. Handles PyAuto initialization and path setup. On 2011/12/06 22:15:05, Nirnimesh wrote: > Please leave 2 spaces after every period. Done. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/p... chrome/test/functional/media/pyauto_media.py:58: class Main(pyauto_functional.Main): On 2011/12/06 22:15:05, Nirnimesh wrote: > This class is a no-op. It can be replaced with: > > Main = pyauto_functional.Main Done.
If we don't own the video, please checkin the file separately in the private repo. PyAuto tests use private data files from chrome/test/data/pyauto_private http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... File chrome/test/functional/media/media_cn_perf.py (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:42: # Test constraints are all possible combination of the above settings. Each On 2011/12/06 23:42:54, DaleCurtis wrote: > On 2011/12/06 22:15:05, Nirnimesh wrote: > > Leave 2 blank spaces after every period, ie before 'Each' > > (See Comments section in http://www.python.org/dev/peps/pep-0008/) > > Ugh, if you insist. In protest, I'd like to point out that it's inconsistent > with the majority of the PyAuto code and it's a deprecated style: > > http://www.slate.com/articles/technology/technology/2011/01/space_invaders.html > http://en.wikipedia.org/wiki/Space_%28punctuation%29#Spaces_between_sentences I don't like it either. All my initial pyauto code has one space after period, but I was pointed to the style guide (http://www.chromium.org/developers/coding-style which points to PEP-8) which specifically asks for 2 spaces. So I use 2 spaces for all new python code. However, I don't insist. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:142: # Make the test URL unique so we can figure out our tab index later. On 2011/12/06 23:42:54, DaleCurtis wrote: > On 2011/12/06 22:15:05, Nirnimesh wrote: > > why not save the tab index? > > When we talked via chat you said the tab_index changes when tabs are closed. Is > this not correct? Another thread may have closed a tab in between critical > sections. I see. Yes, that's correct. To that end, why do you need to close the tabs?
Okay, I'll discuss with local team and move into private repo if necessary. http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... File chrome/test/functional/media/media_cn_perf.py (right): http://codereview.chromium.org/8802030/diff/10/chrome/test/functional/media/m... chrome/test/functional/media/media_cn_perf.py:142: # Make the test URL unique so we can figure out our tab index later. On 2011/12/06 23:58:36, Nirnimesh wrote: > On 2011/12/06 23:42:54, DaleCurtis wrote: > > On 2011/12/06 22:15:05, Nirnimesh wrote: > > > why not save the tab index? > > > > When we talked via chat you said the tab_index changes when tabs are closed. > Is > > this not correct? Another thread may have closed a tab in between critical > > sections. > > I see. Yes, that's correct. > > To that end, why do you need to close the tabs? I could probably get away with not doing so, but with 48 potential tabs open, it seemed risky when it's trivial to protect against.
LGTM, after you sort out the location for roller.webm
http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_cn.html (right): http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... chrome/test/data/media/html/media_cn.html:1: <!-- Used by media_cn_perf to record EPT, TTP values for a specified video. --> "cn" is China's country-level domain and confusing here. s/cn/cns/ everywhere? Or maybe simplify by expanding out to "constrained_network"? http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... chrome/test/data/media/html/media_cn.html:16: var loadTime missing semicolon. http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... chrome/test/data/media/html/media_cn.html:24: var extra_play_time = -1, time_to_playback = -1; s/play_time/play_percentage/ ? http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... File chrome/test/functional/media/media_cn_perf.py (right): http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:1: #!/usr/bin/env python Ditto filename comment http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:14: more information -- http://goo.gl/EdAWU Move this to a public doc, or remove pointer from here? http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:58: # TODO(dalecurtis): Should be set on the command line. Why? http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:70: _CNS_PORT = 9000 Is there a way to get a known-unused port on the bot slaves? http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:100: for tab in self._pyauto.GetBrowserInfo()['windows'][0]['tabs']: No lock? http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:105: """Returns true if metrics are ready. Set self.{_ept,_ttp} < 0 pre-run.""" Why not set to -1 in the ctor instead of "pre-run"? http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:128: if (settings, name) == (None, None): So one worker thread gets the magic and exits, but what about the other two? I don't understand how your test ever exits (I expect thread.join() at l.220 to wait forever, as the .get() call above waits for the queue to get another task appended to it)
http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... File chrome/test/data/media/html/media_cn.html (right): http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... chrome/test/data/media/html/media_cn.html:1: <!-- Used by media_cn_perf to record EPT, TTP values for a specified video. --> On 2011/12/07 01:33:08, Ami Fischman wrote: > "cn" is China's country-level domain and confusing here. s/cn/cns/ everywhere? > Or maybe simplify by expanding out to "constrained_network"? Done. http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... chrome/test/data/media/html/media_cn.html:16: var loadTime On 2011/12/07 01:33:08, Ami Fischman wrote: > missing semicolon. Done. http://codereview.chromium.org/8802030/diff/6001/chrome/test/data/media/html/... chrome/test/data/media/html/media_cn.html:24: var extra_play_time = -1, time_to_playback = -1; On 2011/12/07 01:33:08, Ami Fischman wrote: > s/play_time/play_percentage/ ? Done. http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... File chrome/test/functional/media/media_cn_perf.py (right): http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:1: #!/usr/bin/env python On 2011/12/07 01:33:08, Ami Fischman wrote: > Ditto filename comment Done. http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:14: more information -- http://goo.gl/EdAWU On 2011/12/07 01:33:08, Ami Fischman wrote: > Move this to a public doc, or remove pointer from here? Removed it, relevant information is already included in this file. http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:58: # TODO(dalecurtis): Should be set on the command line. On 2011/12/07 01:33:08, Ami Fischman wrote: > Why? Good point. I was looking at this as more of a utility than a test. Removed. http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:70: _CNS_PORT = 9000 On 2011/12/07 01:33:08, Ami Fischman wrote: > Is there a way to get a known-unused port on the bot slaves? Short of trying to open a socket on each port and iterating, probably not. I'll see what I can find out. http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:100: for tab in self._pyauto.GetBrowserInfo()['windows'][0]['tabs']: On 2011/12/07 01:33:08, Ami Fischman wrote: > No lock? Already locked in _HaveMetrics() I'll add a comment. http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:105: """Returns true if metrics are ready. Set self.{_ept,_ttp} < 0 pre-run.""" On 2011/12/07 01:33:08, Ami Fischman wrote: > Why not set to -1 in the ctor instead of "pre-run"? It needs to be set for each call. I suspect there might be a better pattern to use here, but couldn't think of one. http://codereview.chromium.org/8802030/diff/6001/chrome/test/functional/media... chrome/test/functional/media/media_cn_perf.py:128: if (settings, name) == (None, None): On 2011/12/07 01:33:08, Ami Fischman wrote: > So one worker thread gets the magic and exits, but what about the other two? I > don't understand how your test ever exits (I expect thread.join() at l.220 to > wait forever, as the .get() call above waits for the queue to get another task > appended to it) A magic exit pair is added to the Queue for each thread. Since each thread only gets one value before exiting, it's guaranteed that all threads will receive the magic exit. Again, I suspect there might be a better pattern here, but after implementing Python thread pools at least half a dozen times, this is the best evolution I've come up with.
I think the only remaining issue is the location of roller.webm, which I think needs to go in the private repo.
Committed roller.webm to private repo. Updated www-data path in this CL. Committing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/8802030/27001
Try job failure for 8802030-27001 (retry) on linux_rel for step "ui_tests". It's a second try, previously, step "ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
On 2011/12/09 23:10:42, I haz the power (commit-bot) wrote: > Try job failure for 8802030-27001 (retry) on linux_rel for step "ui_tests". > It's a second try, previously, step "ui_tests" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Seems to be flake elsewhere as my test shouldn't even be running yet. Will recommit in a bit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/8802030/27001
Change committed as 114071 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
