Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10411011:
Add Web Page Replay enabled page cycler tests to pyauto. (Closed)
|
Created:
8 years, 7 months ago by slamm_google Modified:
8 years, 7 months ago CC:
chromium-reviews, anantha, pam+watch_chromium.org, dyu1, cmp, James Simonsen Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd Web Page Replay enabled page cycler tests to pyauto.
NOTRY=true
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138466
Patch Set 1 #
Total comments: 22
Patch Set 2 : Review fixes & first working version. #
Total comments: 6
Patch Set 3 : Look for PC_NO_AUTO and PC_RECORD in env. Check values. #Patch Set 4 : webpagereplay.py with orig webpagereplay_util.py source. #Patch Set 5 : Reapply webpagereplay.py changes. #Patch Set 6 : Disable test initially. Use latest WPR to fix page loads. #Patch Set 7 : gclient sync #Patch Set 8 : Allow up to 3 minutes per WPR iteration. Add options for alternate WPR code. #
Messages
Total messages: 22 (0 generated)
Hey Tony, I took a first crack at this. It is not quite right. I made a base class for the different flavors of page cycler tests and the test runner tries to run it as a test too. Maybe it needs to be a mix-in? Anyway, this is looking pretty good. I need to go back to my real desk to see if it runs. (I have been spending a lot of time on the walkstations.) As part of this, I moved webpagereplay_utils to the same directory as perf.py. Otherwise, I would have to fiddle with sys.path. Did not seem like it needed to be in that other directory anymore. -slamm https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/webpagereplay.py:226: '--no-proxy-server', This list will go away in favor of the one above. This function will need a few more edits -- was focusing on the perf test.
https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1981: self._RunPageCyclerTest('2012Q2', 'Wpr2012Q2') Is this expected to be able to run on ChromeOS? By default, any tests added to this file will start running on ChromeOS once the changes get checked in, unless they're explicitly disabled on ChromeOS in the PYAUTO_TESTS file (in the same directory as the current file). If you need to disable this test there, look for the suite called "PERFORMANCE" in that file. For chromeOS, it includes everything in perf.py except for perf.GPUPerfTest.
Also, I just recently enabled these tests on some of the ChromeQA pyauto bots (Win/Mac/Linux). So the test will start running there too, unless they're explicitly disabled on those platforms. (almost forgot to mention this, since it's a very recent change!). On Thu, May 17, 2012 at 5:28 PM, <dennisjeffrey@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10411011/diff/1/** > chrome/test/functional/perf.py<https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional/perf.py> > File chrome/test/functional/perf.py (right): > > https://chromiumcodereview.**appspot.com/10411011/diff/1/** > chrome/test/functional/perf.**py#newcode1981<https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional/perf.py#newcode1981> > chrome/test/functional/perf.**py:1981: self._RunPageCyclerTest('**2012Q2', > 'Wpr2012Q2') > Is this expected to be able to run on ChromeOS? > > By default, any tests added to this file will start running on ChromeOS > once the changes get checked in, unless they're explicitly disabled on > ChromeOS in the PYAUTO_TESTS file (in the same directory as the current > file). If you need to disable this test there, look for the suite > called "PERFORMANCE" in that file. For chromeOS, it includes everything > in perf.py except for perf.GPUPerfTest. > > https://chromiumcodereview.**appspot.com/10411011/<https://chromiumcodereview... >
+sonnyrao, who recently wrote page_cycler in pyauto.
LGTM I really like your approach here. Just a few stylistic nits inline. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1746: @classmethod nit: blank line above this https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1824: def _GetArithmeticMean(self, values): These math helper functions shouldn't be named as getters and don't could be made static or better yet moved out of this class. Do we have a more generic place for things like this? https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1854: nit: extra line break https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1931: class WebPageReplayPageCyclerTest(BasePageCyclerTest): Please add a class level docstring. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1978: nit: extra line break https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/webpagereplay.py:47: class ReplayServer(object): Probably worth a brief comment explaining how this is intended to be used (e.g. the connection between __enter__/__exit__ and a with-block may not be obvious to people less familiar w/ python. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/webpagereplay.py:188: '--enable-file-cookies', This one isn't necessary since we aren't using file URLs, right? https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/webpagereplay.py:246: def main(): Any reason not to just remove main entirely in this patch?
https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1740: Derived classes must implement _StartUrl(). Methods prefixed with _ are considered private. Derived classes cannot be expected to implement it. Please remove _ Same for some other methods you're overriding.
PTAL. Thanks for the reviews. I have addressed the comments except for the platform issues. I still need to get to those. I have successfully run this on Linux. I was planning to leave it off for Windows (I think it can run there, however, I was worried about the time it takes to run this). ChromeOS comments are inline. I moved some code from webpagereplay.py into perf.py. Another step might be to move all the page cycler code into a new page_cycler.py file. However, I am not sure if you would rather keep it all together in perf.py. -slamm https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1740: Derived classes must implement _StartUrl(). On 2012/05/18 19:19:13, Nirnimesh wrote: > Methods prefixed with _ are considered private. Derived classes cannot be > expected to implement it. Please remove _ > > Same for some other methods you're overriding. Done. Thanks I was unclear on that. I changed _RunPageCyclerTest too. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1746: @classmethod On 2012/05/18 15:37:54, tonyg wrote: > nit: blank line above this Done. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1824: def _GetArithmeticMean(self, values): On 2012/05/18 15:37:54, tonyg wrote: > These math helper functions shouldn't be named as getters and don't could be > made static or better yet moved out of this class. Do we have a more generic > place for things like this? I do not see any collection of functions in this directory or higher up. The closest seems to be ./test_utils.py. I moved the to the top of this file for now. BTW, there is an implementation of the geometric mean in tools/build/scripts/slave/chromium/dom_perf.py. I also updated this geometric mean calculation to deal with large numbers if necessary. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1854: On 2012/05/18 15:37:54, tonyg wrote: > nit: extra line break Done. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1931: class WebPageReplayPageCyclerTest(BasePageCyclerTest): On 2012/05/18 15:37:54, tonyg wrote: > Please add a class level docstring. Done. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1978: On 2012/05/18 15:37:54, tonyg wrote: > nit: extra line break Done. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1981: self._RunPageCyclerTest('2012Q2', 'Wpr2012Q2') On 2012/05/18 00:28:41, dennis_jeffrey wrote: > Is this expected to be able to run on ChromeOS? > > By default, any tests added to this file will start running on ChromeOS once the > changes get checked in, unless they're explicitly disabled on ChromeOS in the > PYAUTO_TESTS file (in the same directory as the current file). If you need to > disable this test there, look for the suite called "PERFORMANCE" in that file. > For chromeOS, it includes everything in perf.py except for perf.GPUPerfTest. Thanks for the heads-up. Does this python code run directly on ChromeOS, or is it run on a separate test machine? In the former case, the code will likely work. In the latter case, the code would need to know the test host (name or IP). Web Page Replay starts proxies for http and https. Chrome is pointed to the proxy with the following options: --host-resolver-rules=MAP * %s' % REPLAY_HOST --testing-fixed-http-port=%s' % HTTP_PORT --testing-fixed-https-port=%s' % HTTPS_PORT In the Chrome case, REPLAY_HOST is 127.0.0.1. BTW, I would actually prefer to have an option like --testing-fixed-dns-port so Chrome/ChromeOS would do DNS lookups against Web Page Replay. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/webpagereplay.py:47: class ReplayServer(object): On 2012/05/18 15:37:54, tonyg wrote: > Probably worth a brief comment explaining how this is intended to be used (e.g. > the connection between __enter__/__exit__ and a with-block may not be obvious to > people less familiar w/ python. Done. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/webpagereplay.py:188: '--enable-file-cookies', On 2012/05/18 15:37:54, tonyg wrote: > This one isn't necessary since we aren't using file URLs, right? The start URL is a file URL, and that is where the page cycler cookies are set. https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/webpagereplay.py:246: def main(): On 2012/05/18 15:37:54, tonyg wrote: > Any reason not to just remove main entirely in this patch? Ah, nice to toss this.
lgtm (but I'm not an owner) https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... chrome/test/functional/perf.py:57: return sum(values) / float(len(values)) Needs a base case for 0-length |values|. https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... chrome/test/functional/perf.py:2007: use_auto = True # TODO(slamm): get from env? Sounds reasonable to get from env. Other things in pyauto work that way. Same for record below. https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... chrome/test/functional/webpagereplay.py:1: #!/usr/bin/env python Is there a way to preserve the diff across the move? Hard to tell what changed.
https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10411011/diff/1/chrome/test/functional... chrome/test/functional/perf.py:1981: self._RunPageCyclerTest('2012Q2', 'Wpr2012Q2') On 2012/05/18 23:59:29, slamm wrote: > On 2012/05/18 00:28:41, dennis_jeffrey wrote: > > Is this expected to be able to run on ChromeOS? > > > > By default, any tests added to this file will start running on ChromeOS once > the > > changes get checked in, unless they're explicitly disabled on ChromeOS in the > > PYAUTO_TESTS file (in the same directory as the current file). If you need to > > disable this test there, look for the suite called "PERFORMANCE" in that file. > > > For chromeOS, it includes everything in perf.py except for perf.GPUPerfTest. > > Thanks for the heads-up. > > Does this python code run directly on ChromeOS, or is it run on a separate test > machine? In the former case, the code will likely work. In the latter case, the > code would need to know the test host (name or IP). > It runs directly on ChromeOS, and as Dennis mentioned it would start getting run nightly -- would that work with whatever defaults we have? > Web Page Replay starts proxies for http and https. Chrome is pointed to the > proxy with the following options: > > --host-resolver-rules=MAP * %s' % REPLAY_HOST > --testing-fixed-http-port=%s' % HTTP_PORT > --testing-fixed-https-port=%s' % HTTPS_PORT > > In the Chrome case, REPLAY_HOST is 127.0.0.1. > > BTW, I would actually prefer to have an option like --testing-fixed-dns-port so > Chrome/ChromeOS would do DNS lookups against Web Page Replay.
Tony, thanks for the additional comments; replies inline. Sonny wrote: > It runs directly on ChromeOS, and as Dennis mentioned it would start getting run > nightly -- would that work with whatever defaults we have? I do not quite understand your question. The tests may work on ChromeOS if the data is there. The Web Page Replay archived data is in a sibling directory of the page cycler pages. Looking at PYAUTO_TESTS, the page cycler tests are disabled for win/mac/linux because it "requires page cycler data files on QA bots." Is that still the case. What would it take to get the data on the mac/linux bots? Could someone help me get this tested on ChromeOS? Should I commit this change with the WPR test turned of everywhere to make it easier to test it and then turn it on? I am looking for what the next step should be. -slamm https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... chrome/test/functional/perf.py:57: return sum(values) / float(len(values)) On 2012/05/19 00:46:36, tonyg wrote: > Needs a base case for 0-length |values|. Done. I also added a check for 'None' values. https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... chrome/test/functional/perf.py:2007: use_auto = True # TODO(slamm): get from env? On 2012/05/19 00:46:36, tonyg wrote: > Sounds reasonable to get from env. Other things in pyauto work that way. > > Same for record below. Done. The code looks for 'PC_NO_AUTO' and 'PC_RECORD'. https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... chrome/test/functional/webpagereplay.py:1: #!/usr/bin/env python On 2012/05/19 00:46:36, tonyg wrote: > Is there a way to preserve the diff across the move? Hard to tell what changed. I uploaded two additional patches to see the diff: Patch 4 gives webpagereplay.py the original webpagereplay_utils.py source.\ Patch 5 gives webpagereplay.py back the latests changes (what was in patch 3) Looked at patch 5 and click on the "delta from patch set" 4.
On 2012/05/21 22:22:06, slamm wrote: > Tony, thanks for the additional comments; replies inline. > > Sonny wrote: > > It runs directly on ChromeOS, and as Dennis mentioned it would start getting > run > > nightly -- would that work with whatever defaults we have? > > I do not quite understand your question. The tests may work on ChromeOS if the > data is there. The Web Page Replay archived data is in a sibling directory of > the page cycler pages. > I know Sonny has been working on setting up test dependencies on ChromeOS to ensure these page cycler tests have access to the page cycler data folder, but I'm not sure if this includes the sibling folder that contains WPR's archived data. Sonny, do you know about this? > Looking at PYAUTO_TESTS, the page cycler tests are disabled for win/mac/linux > because it "requires page cycler data files on QA bots." Is that still the case. > What would it take to get the data on the mac/linux bots? > Yes, I believe this is still the case. The pyauto perf tests are running on several of the bots on the ChromeQA waterfall, and I don't think the dependencies for those test executions have been set up to include the page cycler data yet. I believe it should be a straightforward fix to ensure those slaves sync the page cycler code at the same time they sync the other pyauto-related code, but I haven't looked into it yet. So I think that the test in class WebPageReplayPageCyclerTest won't work on Win/Mac/Linux (on the ChromeQA waterfall) until we make sure the page cycler/WPR dependencies are set up there. > Could someone help me get this tested on ChromeOS? > Should I commit this change with the WPR test turned of everywhere to make it > easier to test it and then turn it on? > > I am looking for what the next step should be. > I can help you try to test this on ChromeOS. We could also disable the WPR test in class WebPageReplayPageCyclerTest (on all platforms) for now until we're sure it's working everywhere. Then it'll just be a small one-liner later to enable it in the PYAUTO_TESTS file. > -slamm > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > File chrome/test/functional/perf.py (right): > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > chrome/test/functional/perf.py:57: return sum(values) / float(len(values)) > On 2012/05/19 00:46:36, tonyg wrote: > > Needs a base case for 0-length |values|. > > Done. I also added a check for 'None' values. > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > chrome/test/functional/perf.py:2007: use_auto = True # TODO(slamm): get from > env? > On 2012/05/19 00:46:36, tonyg wrote: > > Sounds reasonable to get from env. Other things in pyauto work that way. > > > > Same for record below. > > Done. > > The code looks for 'PC_NO_AUTO' and 'PC_RECORD'. > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > File chrome/test/functional/webpagereplay.py (right): > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > chrome/test/functional/webpagereplay.py:1: #!/usr/bin/env python > On 2012/05/19 00:46:36, tonyg wrote: > > Is there a way to preserve the diff across the move? Hard to tell what > changed. > > I uploaded two additional patches to see the diff: > Patch 4 gives webpagereplay.py the original webpagereplay_utils.py source.\ > Patch 5 gives webpagereplay.py back the latests changes (what was in patch 3) > Looked at patch 5 and click on the "delta from patch set" 4.
On 2012/05/21 22:41:58, dennis_jeffrey wrote: > On 2012/05/21 22:22:06, slamm wrote: > > Tony, thanks for the additional comments; replies inline. > > > > Sonny wrote: > > > It runs directly on ChromeOS, and as Dennis mentioned it would start getting > > run > > > nightly -- would that work with whatever defaults we have? > > > > I do not quite understand your question. The tests may work on ChromeOS if the > > data is there. The Web Page Replay archived data is in a sibling directory of > > the page cycler pages. > > > > I know Sonny has been working on setting up test dependencies on ChromeOS to > ensure these page cycler tests have access to the page cycler data folder, but > I'm not sure if this includes the sibling folder that contains WPR's archived > data. Sonny, do you know about this? > Yes we'd have to add the additional directory to our page_cycler dependency in order to get it to run. > > Looking at PYAUTO_TESTS, the page cycler tests are disabled for win/mac/linux > > because it "requires page cycler data files on QA bots." Is that still the > case. > > What would it take to get the data on the mac/linux bots? > > > > Yes, I believe this is still the case. The pyauto perf tests are running on > several of the bots on the ChromeQA waterfall, and I don't think the > dependencies for those test executions have been set up to include the page > cycler data yet. I believe it should be a straightforward fix to ensure those > slaves sync the page cycler code at the same time they sync the other > pyauto-related code, but I haven't looked into it yet. > > So I think that the test in class WebPageReplayPageCyclerTest won't work on > Win/Mac/Linux (on the ChromeQA waterfall) until we make sure the page cycler/WPR > dependencies are set up there. > > > Could someone help me get this tested on ChromeOS? > > Should I commit this change with the WPR test turned of everywhere to make it > > easier to test it and then turn it on? > > > > I am looking for what the next step should be. > > > > I can help you try to test this on ChromeOS. We could also disable the WPR test > in class WebPageReplayPageCyclerTest (on all platforms) for now until we're sure > it's working everywhere. Then it'll just be a small one-liner later to enable > it in the PYAUTO_TESTS file. > That sounds like the right thing to do for right now. We can re-enable it on ChromeOS when we get the data files in place. > > -slamm > > > > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > > File chrome/test/functional/perf.py (right): > > > > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > > chrome/test/functional/perf.py:57: return sum(values) / float(len(values)) > > On 2012/05/19 00:46:36, tonyg wrote: > > > Needs a base case for 0-length |values|. > > > > Done. I also added a check for 'None' values. > > > > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > > chrome/test/functional/perf.py:2007: use_auto = True # TODO(slamm): get from > > env? > > On 2012/05/19 00:46:36, tonyg wrote: > > > Sounds reasonable to get from env. Other things in pyauto work that way. > > > > > > Same for record below. > > > > Done. > > > > The code looks for 'PC_NO_AUTO' and 'PC_RECORD'. > > > > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > > File chrome/test/functional/webpagereplay.py (right): > > > > > https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functio... > > chrome/test/functional/webpagereplay.py:1: #!/usr/bin/env python > > On 2012/05/19 00:46:36, tonyg wrote: > > > Is there a way to preserve the diff across the move? Hard to tell what > > changed. > > > > I uploaded two additional patches to see the diff: > > Patch 4 gives webpagereplay.py the original webpagereplay_utils.py source.\ > > Patch 5 gives webpagereplay.py back the latests changes (what was in patch > 3) > > Looked at patch 5 and click on the "delta from patch set" 4.
On Mon, May 21, 2012 at 4:56 PM, <sonnyrao@chromium.org> wrote: > On 2012/05/21 22:41:58, dennis_jeffrey wrote: > >> On 2012/05/21 22:22:06, slamm wrote: >> > Tony, thanks for the additional comments; replies inline. >> > >> > Sonny wrote: >> > > It runs directly on ChromeOS, and as Dennis mentioned it would start >> > getting > >> > run >> > > nightly -- would that work with whatever defaults we have? >> > >> > I do not quite understand your question. The tests may work on ChromeOS >> if >> > the > >> > data is there. The Web Page Replay archived data is in a sibling >> directory >> > of > >> > the page cycler pages. >> > >> > > I know Sonny has been working on setting up test dependencies on ChromeOS >> to >> ensure these page cycler tests have access to the page cycler data >> folder, but >> I'm not sure if this includes the sibling folder that contains WPR's >> archived >> data. Sonny, do you know about this? >> > > > Yes we'd have to add the additional directory to our page_cycler > dependency in > order to get it to run. > > > > Looking at PYAUTO_TESTS, the page cycler tests are disabled for >> > win/mac/linux > >> > because it "requires page cycler data files on QA bots." Is that still >> the >> case. >> > What would it take to get the data on the mac/linux bots? >> > >> > > Yes, I believe this is still the case. The pyauto perf tests are running >> on >> several of the bots on the ChromeQA waterfall, and I don't think the >> dependencies for those test executions have been set up to include the >> page >> cycler data yet. I believe it should be a straightforward fix to ensure >> those >> slaves sync the page cycler code at the same time they sync the other >> pyauto-related code, but I haven't looked into it yet. >> > > So I think that the test in class WebPageReplayPageCyclerTest won't work >> on >> Win/Mac/Linux (on the ChromeQA waterfall) until we make sure the page >> > cycler/WPR > >> dependencies are set up there. >> > > > Could someone help me get this tested on ChromeOS? >> > Should I commit this change with the WPR test turned of everywhere to >> make >> > it > >> > easier to test it and then turn it on? >> > >> > I am looking for what the next step should be. >> > >> > > I can help you try to test this on ChromeOS. We could also disable the >> WPR >> > test > >> in class WebPageReplayPageCyclerTest (on all platforms) for now until >> we're >> > sure > >> it's working everywhere. Then it'll just be a small one-liner later to >> enable >> it in the PYAUTO_TESTS file. >> > > > That sounds like the right thing to do for right now. We can re-enable it > on > ChromeOS when we get the data files in place. I uploaded a new patch that disables WebPageReplayPageCyclerTest on all platforms. I also bumped revision number for third_party/webpagereplay (tonyg fixed the page loading issues). All good now? -slamm > > > -slamm >> > >> > >> > > https://chromiumcodereview.**appspot.com/10411011/diff/** > 7001/chrome/test/functional/**perf.py<https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functional/perf.py> > >> > File chrome/test/functional/perf.py (right): >> > >> > >> > > https://chromiumcodereview.**appspot.com/10411011/diff/** > 7001/chrome/test/functional/**perf.py#newcode57<https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functional/perf.py#newcode57> > >> > chrome/test/functional/perf.**py:57: return sum(values) / >> float(len(values)) >> > On 2012/05/19 00:46:36, tonyg wrote: >> > > Needs a base case for 0-length |values|. >> > >> > Done. I also added a check for 'None' values. >> > >> > >> > > https://chromiumcodereview.**appspot.com/10411011/diff/** > 7001/chrome/test/functional/**perf.py#newcode2007<https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functional/perf.py#newcode2007> > >> > chrome/test/functional/perf.**py:2007: use_auto = True # TODO(slamm): >> get >> > from > >> > env? >> > On 2012/05/19 00:46:36, tonyg wrote: >> > > Sounds reasonable to get from env. Other things in pyauto work that >> way. >> > > >> > > Same for record below. >> > >> > Done. >> > >> > The code looks for 'PC_NO_AUTO' and 'PC_RECORD'. >> > >> > >> > > https://chromiumcodereview.**appspot.com/10411011/diff/** > 7001/chrome/test/functional/**webpagereplay.py<https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functional/webpagereplay.py> > >> > File chrome/test/functional/**webpagereplay.py (right): >> > >> > >> > > https://chromiumcodereview.**appspot.com/10411011/diff/** > 7001/chrome/test/functional/**webpagereplay.py#newcode1<https://chromiumcodereview.appspot.com/10411011/diff/7001/chrome/test/functional/webpagereplay.py#newcode1> > >> > chrome/test/functional/**webpagereplay.py:1: #!/usr/bin/env python >> > On 2012/05/19 00:46:36, tonyg wrote: >> > > Is there a way to preserve the diff across the move? Hard to tell what >> > changed. >> > >> > I uploaded two additional patches to see the diff: >> > Patch 4 gives webpagereplay.py the original webpagereplay_utils.py >> > source.\ > >> > Patch 5 gives webpagereplay.py back the latests changes (what was in >> patch >> 3) >> > Looked at patch 5 and click on the "delta from patch set" 4. >> > > > > https://chromiumcodereview.**appspot.com/10411011/<https://chromiumcodereview... >
Code I commented on LGTM
LGTM with respect to which tests are enabled/disabled on the different platforms.
On 2012/05/22 00:43:08, dennis_jeffrey wrote: > LGTM with respect to which tests are enabled/disabled on the different > platforms. Thanks for all the reviews! I added a few minor tweaks. 1. Did a gclient sync to check for merge issues. 2. On my machine, one iteration through the pages took longer than the 60 second timeout (takes about 72 seconds). I increased the timeout to 180 seconds. 3. Added PC_REPLAY_DIR env variable to allow for an alternate WPR source tree. 4. Bumped the WPR revision again to have the latest. Sonny, if this looks good to you, would you mind submitting it for me? -slamm
On 2012/05/22 16:56:42, slamm wrote: > On 2012/05/22 00:43:08, dennis_jeffrey wrote: > > LGTM with respect to which tests are enabled/disabled on the different > > platforms. > > Thanks for all the reviews! > > I added a few minor tweaks. > 1. Did a gclient sync to check for merge issues. > 2. On my machine, one iteration through the pages took longer than the 60 > second timeout (takes about 72 seconds). I increased the timeout to 180 seconds. > 3. Added PC_REPLAY_DIR env variable to allow for an alternate WPR source tree. > 4. Bumped the WPR revision again to have the latest. > > Sonny, if this looks good to you, would you mind submitting it for me? > > -slamm Hi, I don't have commit access but you don't need it either, you can just check the box that says "Commit" below the diffs and that will automatically run the trybot and commit it for you.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/slamm@google.com/10411011/16006
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is win_rel, revision is 138435, job name was 10411011-16006 (retry) (retry) (retry) (retry).
This CL doesn't really have anything to try (these tests do not run on try bots). Feel free to use NOTRY=true in the CL description to skip try jobs.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/slamm@google.com/10411011/16006
Change committed as 138466 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
