Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10803002:
Run Chrome Endure tests with network simulation via Web Page Replay. (Closed)
|
Created:
8 years, 5 months ago by fdeng1 Modified:
8 years, 3 months ago CC:
chromium-reviews, dennis_jeffrey, anantha, dyu1, web-page-replay-dev_googlegroups.com, slamm_google, jochen (gone - plz use gerrit) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRun Chrome Endure tests with network simulation via Web Page Replay.
Previously Chrome Endure tests always connect to a live website which may change over time.
This CL is to simulate the network via Web Page Replay. The idea is to use WPR to record webapp interactions of a test and replay them in the future runs of the test.
Environment variables:
WPR_RECORD: set this environment variable if you wan to record webapp interactions of tests via Web Page Replay.
ENDURE_NO_WPR: set this value if you DO NOT want to run the endure tests against the Web Page Replay server.
Examples:
1. ENDURE_NO_WPR=any_value python perf_endure.py perf_endure.ChromeEndureGmailTest
Tests will connect to live site.
2. WPR_RECORD=any_value python perf_endure.py perf_endure.ChromeEndureGmailTest
Web Page Replay will start in record mode and a Web Page Replay archive will be created.
3. python perf_endure.py perf_endure.ChromeEndureGmailTest
By default, tests will run against Web Page Replay server.
BUG=None
TEST=ran all tests in perf_endure.py in three modes: record, replay, no web page replay
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155473
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : Env var to control usage of WPR #
Total comments: 43
Patch Set 8 : Address Dennis' Comments #
Total comments: 19
Patch Set 9 : Address Dennis and Steve's comments #Patch Set 10 : set self._num_of_iteration=1 when recording #Patch Set 11 : Adjustments after patching Steve's CL #
Total comments: 8
Patch Set 12 : Address Nirnimesh's comments. #Patch Set 13 : Fix so that offline tests will work/Address Nirnimesh's comments #
Total comments: 3
Patch Set 14 : Tests by default do not have WPR support. #
Total comments: 25
Patch Set 15 : Address Dennis' comments #
Total comments: 17
Patch Set 16 : Address Nirnimesh, Dennis, Steve's comments #
Total comments: 14
Patch Set 17 : Address more comments #Patch Set 18 : Merge with master #
Messages
Total messages: 33 (0 generated)
Hi Dennis and Nirnimesh, This is the CL I created to run chrome endure tests with WPR. This CL can only work together with the changes I made to WPR. So I need to figure out how I can upload the changes in third-party/webpagereplay before others can run it. But as for now, I would like you to take a look at it and see what you think. (I sent out a different CL regarding the changes to WPR, but that CL has gone to a different review system.) Thanks, Fang
https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/data/ch... File chrome/test/data/chrome_endure/deterministic.js (right): https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:1: (function () { license header? Add comments about this file. https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... chrome/test/functional/perf_endure.py:583: def ExtraChromeFlags(self): I wish you add this functionality to ChromeEndureBaseTest, rather than just in the gmail test. https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... chrome/test/functional/perf_endure.py:1155: '--host-resolver-rules=MAP * %s' % webpagereplay.REPLAY_HOST, If it works with it, I think you should retain the EXCLUDE localhost part, so that it does not interfere when using the local testserver for serving local data files. https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... chrome/test/functional/perf_endure.py:1158: '--log-level=0', remove this and other flags that are not necessary.
Thank you for the comments Nirnimesh. I'll address them. Fang On Wed, Jul 18, 2012 at 12:41 PM, <nirnimesh@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 1003/chrome/test/data/chrome_**endure/deterministic.js<https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/data/chrome_endure/deterministic.js> > File chrome/test/data/chrome_**endure/deterministic.js (right): > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 1003/chrome/test/data/chrome_**endure/deterministic.js#**newcode1<https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/data/chrome_endure/deterministic.js#newcode1> > chrome/test/data/chrome_**endure/deterministic.js:1: (function () { > license header? > > Add comments about this file. > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 1003/chrome/test/functional/**perf_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functional/perf_endure.py> > File chrome/test/functional/perf_**endure.py (right): > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 1003/chrome/test/functional/**perf_endure.py#newcode583<https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functional/perf_endure.py#newcode583> > chrome/test/functional/perf_**endure.py:583: def ExtraChromeFlags(self): > I wish you add this functionality to ChromeEndureBaseTest, rather than > just in the gmail test. > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 1003/chrome/test/functional/**perf_endure.py#newcode1155<https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functional/perf_endure.py#newcode1155> > chrome/test/functional/perf_**endure.py:1155: '--host-resolver-rules=MAP * > %s' % webpagereplay.REPLAY_HOST, > If it works with it, I think you should retain the EXCLUDE localhost > part, so that it does not interfere when using the local testserver for > serving local data files. > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 1003/chrome/test/functional/**perf_endure.py#newcode1158<https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functional/perf_endure.py#newcode1158> > chrome/test/functional/perf_**endure.py:1158: '--log-level=0', > remove this and other flags that are not necessary. > > https://chromiumcodereview.**appspot.com/10803002/<https://chromiumcodereview... >
Hi Dennis and Nirnimesh, I restructured the perf.WebPageReplay. Now we have perf.BaseWebPageReplay and its subclasses --perf.PerfWebPageReplay and perf_endure.ChromeEndureWebPageReplay. The subclasses are used to allow customized WPR related settings for different types of tests. Please take a look and let me know whether you think this is an appropriate solution and other issues. Gmail, Plus, Doc tests are also modified to run against WPR. Thanks, Fang https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/data/ch... File chrome/test/data/chrome_endure/deterministic.js (right): https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:1: (function () { I added a license header. But I also I noticed that WPR is under Apache License. I am not sure which is the right one to use here? On 2012/07/18 19:41:18, Nirnimesh wrote: > license header? > > Add comments about this file. https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... chrome/test/functional/perf_endure.py:583: def ExtraChromeFlags(self): On 2012/07/18 19:41:18, Nirnimesh wrote: > I wish you add this functionality to ChromeEndureBaseTest, rather than just in > the gmail test. Done. I also moved the code that starts/shuts down WPR to ChromeEndureBaseTest(_StartReplayServerIfNecessary and _StopReplayServerIfNecessary).Would you please take a look? https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... chrome/test/functional/perf_endure.py:1155: '--host-resolver-rules=MAP * %s' % webpagereplay.REPLAY_HOST, I restructured perf.WebPageReplay. Now we have perf.BaseWebPageReplay and two subclasses perf.PerWebPageReplay and perf_endure.ChromeEndureWebPageReplay. This is to allow different settings for different tests. "EXCLUDE localhost" is retained in perf.BaseWebPageReplay. On 2012/07/18 19:41:18, Nirnimesh wrote: > If it works with it, I think you should retain the EXCLUDE localhost part, so > that it does not interfere when using the local testserver for serving local > data files. https://chromiumcodereview.appspot.com/10803002/diff/1003/chrome/test/functio... chrome/test/functional/perf_endure.py:1158: '--log-level=0', On 2012/07/18 19:41:18, Nirnimesh wrote: > remove this and other flags that are not necessary. Done. Please take a look at __init__ in perf.BaseWebPageReplay, perf.PerWebPageReplay and perf_endure.ChromeEndureWebPageReplay. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1729: I restructured the old WebPageReplay to allow different settings for different tests(page cycle tests and endure tests). Now we have BaseWebPageReplay which provides the basic settings for a test to run against WPR. Subclasses such as PerfWebPageReplay and ChromeEndureWebPageReplay provides customized settings for the corresponding tests. (I move the location of WPR related classes before the first test class "PopularSitesScrollTest" which needs WPR support. This is because PopularSitesScrollTest refers to PerfWebPageReplay in its class fields.) https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1842: '--activate-on-launch', This is the original settings from the old WebPageReplay for page cycle tests. I removed --no-first-run because it is already been set, if we add it here, it will be duplicated. I also removed --no-proxy as I believe --testing-fixed-http-port and --testing-fixed-https-port should be sufficient to direct the connections to the right ports. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1850: web_page_replay = PerfWebPageReplay() Is it a good idea to use a class field to store the instance of PerfWebPageReplay?
Yay! I'm really excited to have the Chrome Endure tests start using WebPageReplay! https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... File chrome/test/data/chrome_endure/deterministic.js (right): https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:1: /* could we give this file a new name? Something to indicate that it is related to WebPageReplay. Maybe something like "wpr_deterministic.js". https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:30: */ We should probably use the regular 3-line chromium license header here. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:37: * of this scripts in both record and replay mode. you might want to mention that this script is adapted from some code in WPR itself (you could add a link to the original file where you took this code from). Then you can mention what you've changed (i.e., the time seed). https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:43: * This can be achieved by modifying Web Page Replay to automaticly automaticly --> automatically https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:52: var time_seed = 3204251968254; var time_seed = 3204251968254; # Changed from default WebPageReplay value. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:57: if (random_count > random_count_threshold){ add a space right before { https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:66: if (date_count > date_count_threshold){ add a space right before { https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1740: This class provides a minimum set of settings for running a tests with 'a tests' --> 'tests' https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1770: # Initialize Web Page Replay related Paths 'Paths' --> 'paths.' https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1777: #Initialize extra flags for chrome. add a space after # https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1800: Returns: Add a blank line above this line https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1801: a replay server. capitalize the first letter in the sentence: 'a' --> 'A' https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1850: web_page_replay = PerfWebPageReplay() On 2012/07/24 00:05:50, fdeng1 wrote: > Is it a good idea to use a class field to store the instance of > PerfWebPageReplay? I think this is ok. We could prefix the variable name with an underscore to make it more clear that this variable is only meant to be used internally within this class (class variables are traditionally named starting with an underscore if they're meant to only be used privately within this class and all subclasses). https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:186: if 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ: there are multiple places where we check whether this environment variable is present in os.environ. We should probably only check for it once in the ChromeEndureBaseTest setUp() function, then store the result in a local boolean class variable, and check the value of that variable here. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:552: This method need to be called BEFORE any connection (which are supposed 'need' --> 'needs' https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:574: remove this blank line https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1200: remove this blank line https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1204: # Call parent __init__ probably don't need this comment. it's easy to see that from the code itself https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1207: # Extra paths for Web Page Replay add period at end of sentence https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1215: # Add custumized injected_scripts for Endure tests. 'custumized' --> 'customized'
Hi Dennis, Thanks for the comments. I've addressed them. I observed some interesting differences between the results got while WPR is on and the results got while it is off. I am going to investigate them tomorrow. Fang https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... File chrome/test/data/chrome_endure/deterministic.js (right): https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:1: /* On 2012/07/24 00:39:35, dennis_jeffrey wrote: > could we give this file a new name? Something to indicate that it is related to > WebPageReplay. Maybe something like "wpr_deterministic.js". Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:30: */ On 2012/07/24 00:39:35, dennis_jeffrey wrote: > We should probably use the regular 3-line chromium license header here. Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:37: * of this scripts in both record and replay mode. On 2012/07/24 00:39:35, dennis_jeffrey wrote: > you might want to mention that this script is adapted from some code in WPR > itself (you could add a link to the original file where you took this code > from). Then you can mention what you've changed (i.e., the time seed). Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:43: * This can be achieved by modifying Web Page Replay to automaticly On 2012/07/24 00:39:35, dennis_jeffrey wrote: > automaticly --> automatically Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:52: var time_seed = 3204251968254; On 2012/07/24 00:39:35, dennis_jeffrey wrote: > var time_seed = 3204251968254; # Changed from default WebPageReplay value. Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:57: if (random_count > random_count_threshold){ On 2012/07/24 00:39:35, dennis_jeffrey wrote: > add a space right before { Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/data/ch... chrome/test/data/chrome_endure/deterministic.js:66: if (date_count > date_count_threshold){ On 2012/07/24 00:39:35, dennis_jeffrey wrote: > add a space right before { Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1740: This class provides a minimum set of settings for running a tests with On 2012/07/24 00:39:35, dennis_jeffrey wrote: > 'a tests' --> 'tests' Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1770: # Initialize Web Page Replay related Paths On 2012/07/24 00:39:35, dennis_jeffrey wrote: > 'Paths' --> 'paths.' Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1777: #Initialize extra flags for chrome. On 2012/07/24 00:39:35, dennis_jeffrey wrote: > add a space after # Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1800: Returns: On 2012/07/24 00:39:35, dennis_jeffrey wrote: > Add a blank line above this line Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1801: a replay server. On 2012/07/24 00:39:35, dennis_jeffrey wrote: > capitalize the first letter in the sentence: 'a' --> 'A' Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf.py:1850: web_page_replay = PerfWebPageReplay() On 2012/07/24 00:39:35, dennis_jeffrey wrote: > On 2012/07/24 00:05:50, fdeng1 wrote: > > Is it a good idea to use a class field to store the instance of > > PerfWebPageReplay? > > I think this is ok. We could prefix the variable name with an underscore to > make it more clear that this variable is only meant to be used internally within > this class (class variables are traditionally named starting with an underscore > if they're meant to only be used privately within this class and all > subclasses). Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:186: if 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ: On 2012/07/24 00:39:35, dennis_jeffrey wrote: > there are multiple places where we check whether this environment variable is > present in os.environ. We should probably only check for it once in the > ChromeEndureBaseTest setUp() function, then store the result in a local boolean > class variable, and check the value of that variable here. Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:552: This method need to be called BEFORE any connection (which are supposed On 2012/07/24 00:39:35, dennis_jeffrey wrote: > 'need' --> 'needs' Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:574: On 2012/07/24 00:39:35, dennis_jeffrey wrote: > remove this blank line Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1200: On 2012/07/24 00:39:35, dennis_jeffrey wrote: > remove this blank line Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1204: # Call parent __init__ On 2012/07/24 00:39:35, dennis_jeffrey wrote: > probably don't need this comment. it's easy to see that from the code itself Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1207: # Extra paths for Web Page Replay On 2012/07/24 00:39:35, dennis_jeffrey wrote: > add period at end of sentence Done. https://chromiumcodereview.appspot.com/10803002/diff/2007/chrome/test/functio... chrome/test/functional/perf_endure.py:1215: # Add custumized injected_scripts for Endure tests. On 2012/07/24 00:39:35, dennis_jeffrey wrote: > 'custumized' --> 'customized' Done.
Just 2 more minor comments. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1770: # Initialize Web Page Replay related paths add period at end of sentence https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf_endure.py:68: self._not_use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' in os.environ I think it might be easier to reason about this if we make the variable opposite to be self._use_wpr: self._use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ
I am thinking this can all be reorganized into something simpler. I will share another CL in a bit. (I considering moving more things into webpagereplay.py) For now, I have added a few comments. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1759: Remove extra blank line. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1769: def __init__(self): Move __init__ before Path(). https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1770: # Initialize Web Page Replay related paths How about dropping the comment? And the others too. They do not add extra value. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1793: self._num_iterations = 1 This is a bug from an earlier change. Need to find the right place to set this. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1822: # Call parent __init__ The comments in this method do not add any value and can be dropped. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1839: ' --enable-logging', Remove the extra whitespace from the flag. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf_endure.py:36: import webpagereplay This is unused in this module. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf_endure.py:68: self._not_use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' in os.environ On 2012/07/24 21:03:19, dennis_jeffrey wrote: > I think it might be easier to reason about this if we make the variable opposite > to be self._use_wpr: > > self._use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ How about a shorter environment variable name too. Maybe ENDURE_NO_WPR.
Hi Steve, Thanks for your reply. Do you have any recommendation on how to make all these simpler? I feel it makes code complex to have a new derived class each time we want to customize some settings for a test. But I haven't got any better idea yet. What stuff are you trying to move to webpagereplay.py? Maybe that will help solve the problem here. As for now, I've addressed your comments. I move self._num_iterations=1 to the setUp() functions in PageCyclerNetSimTest and PopularSitesScrollTest. Please take a look. Thanks, Fang https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1759: On 2012/07/25 17:20:48, slamm wrote: > Remove extra blank line. Done. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1769: def __init__(self): On 2012/07/25 17:20:48, slamm wrote: > Move __init__ before Path(). Done. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1770: # Initialize Web Page Replay related paths On 2012/07/25 17:20:48, slamm wrote: > How about dropping the comment? And the others too. They do not add extra value. Done. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1793: self._num_iterations = 1 I moved it the setSep() functions in PageCyclerNetSimTest and PopularSitesScrollTest. How do you think? On 2012/07/25 17:20:48, slamm wrote: > This is a bug from an earlier change. Need to find the right place to set this. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1822: # Call parent __init__ On 2012/07/25 17:20:48, slamm wrote: > The comments in this method do not add any value and can be dropped. Done. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf.py:1839: ' --enable-logging', On 2012/07/25 17:20:48, slamm wrote: > Remove the extra whitespace from the flag. Done. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf_endure.py:36: import webpagereplay On 2012/07/25 17:20:48, slamm wrote: > This is unused in this module. Done. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf_endure.py:68: self._not_use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' in os.environ On 2012/07/25 17:20:48, slamm wrote: > On 2012/07/24 21:03:19, dennis_jeffrey wrote: > > I think it might be easier to reason about this if we make the variable > opposite > > to be self._use_wpr: > > > > self._use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ > > How about a shorter environment variable name too. Maybe ENDURE_NO_WPR. Done. https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functi... chrome/test/functional/perf_endure.py:68: self._not_use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' in os.environ On 2012/07/24 21:03:19, dennis_jeffrey wrote: > I think it might be easier to reason about this if we make the variable opposite > to be self._use_wpr: > > self._use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ Done.
I will post a CL shortly. I hit some unexpected merge conflicts. -slamm On Wed, Jul 25, 2012 at 3:53 PM, <fdeng@chromium.org> wrote: > Hi Steve, > > Thanks for your reply. Do you have any recommendation on how to make all > these > simpler? I feel it makes code complex to have a new derived class each > time we > want to customize some settings for a test. But I haven't got any better > idea > yet. > What stuff are you trying to move to webpagereplay.py? Maybe that will help > solve the problem here. > > As for now, I've addressed your comments. I move self._num_iterations=1 to > the > setUp() functions in > PageCyclerNetSimTest and PopularSitesScrollTest. Please take a look. > > Thanks, > Fang > > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf.py<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf.py> > File chrome/test/functional/perf.py (right): > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf.py#newcode1759<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf.py#newcode1759> > chrome/test/functional/perf.**py:1759: > On 2012/07/25 17:20:48, slamm wrote: > >> Remove extra blank line. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf.py#newcode1769<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf.py#newcode1769> > chrome/test/functional/perf.**py:1769: def __init__(self): > On 2012/07/25 17:20:48, slamm wrote: > >> Move __init__ before Path(). >> > > Done. > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf.py#newcode1770<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf.py#newcode1770> > chrome/test/functional/perf.**py:1770: # Initialize Web Page Replay > related paths > On 2012/07/25 17:20:48, slamm wrote: > >> How about dropping the comment? And the others too. They do not add >> > extra value. > > Done. > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf.py#newcode1793<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf.py#newcode1793> > chrome/test/functional/perf.**py:1793: self._num_iterations = 1 > I moved it the setSep() functions in PageCyclerNetSimTest and > PopularSitesScrollTest. How do you think? > > > On 2012/07/25 17:20:48, slamm wrote: > >> This is a bug from an earlier change. Need to find the right place to >> > set this. > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf.py#newcode1822<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf.py#newcode1822> > chrome/test/functional/perf.**py:1822: # Call parent __init__ > On 2012/07/25 17:20:48, slamm wrote: > >> The comments in this method do not add any value and can be dropped. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf.py#newcode1839<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf.py#newcode1839> > chrome/test/functional/perf.**py:1839: ' --enable-logging', > On 2012/07/25 17:20:48, slamm wrote: > >> Remove the extra whitespace from the flag. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf_endure.py> > File chrome/test/functional/perf_**endure.py (right): > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf_endure.py#newcode36<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf_endure.py#newcode36> > chrome/test/functional/perf_**endure.py:36: import webpagereplay > On 2012/07/25 17:20:48, slamm wrote: > >> This is unused in this module. >> > > Done. > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf_endure.py#newcode68<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf_endure.py#newcode68> > chrome/test/functional/perf_**endure.py:68: self._not_use_wpr = > 'DO_NOT_USE_WPR_FOR_ENDURE' in os.environ > On 2012/07/25 17:20:48, slamm wrote: > >> On 2012/07/24 21:03:19, dennis_jeffrey wrote: >> > I think it might be easier to reason about this if we make the >> > variable > >> opposite >> > to be self._use_wpr: >> > >> > self._use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ >> > > How about a shorter environment variable name too. Maybe >> > ENDURE_NO_WPR. > > Done. > > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 16001/chrome/test/functional/**perf_endure.py#newcode68<https://chromiumcodereview.appspot.com/10803002/diff/16001/chrome/test/functional/perf_endure.py#newcode68> > chrome/test/functional/perf_**endure.py:68: self._not_use_wpr = > 'DO_NOT_USE_WPR_FOR_ENDURE' in os.environ > On 2012/07/24 21:03:19, dennis_jeffrey wrote: > >> I think it might be easier to reason about this if we make the >> > variable opposite > >> to be self._use_wpr: >> > > self._use_wpr = 'DO_NOT_USE_WPR_FOR_ENDURE' not in os.environ >> > > Done. > > https://chromiumcodereview.**appspot.com/10803002/<https://chromiumcodereview... >
Hi Dennis, Nirnimesh, I made some adjustment after patching Steve's CL. Please take a look. Fang
https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:566: logging.info('The Web Page Replay server started.') Too much logging. Please pick between this line and 564 https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:640: self._StartReplayServerIfNecessary(archive_name) I thought this was going to be done in ChromeEndureBaseTest so that it can be used in _any_ endure test. https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:749: self._GmailSetUp(test_description) Why not do this it setUp instead of calling before each test?
https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:566: logging.info('The Web Page Replay server started.') On 2012/08/01 01:03:00, Nirnimesh wrote: > Too much logging. Please pick between this line and 564 Done. https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:640: self._StartReplayServerIfNecessary(archive_name) When starting a replay server, an "archive_name" need to be passed to ChromeEndureReplay.ReplayServer(cls, archive_name). For now, archive_name is unique for each test because each test is using its own archive. I was trying to put self._StartReplayServerIfNecessary(archive_name) is in ChromeEndureBaseTest.setUp() or ChromeEndureGmailTest.setUp(), but I realized I was unable to know which test is about run in the setUp phase and as a result I was unable to pick a unique archive_name for that test. Is there a way to know which test is about to run during the setUp phase? (I was also thinking put all archives into a big one which will be shared by all tests. But the performance of WPR degrades a lot when the archive is big.) On 2012/08/01 01:03:00, Nirnimesh wrote: > I thought this was going to be done in ChromeEndureBaseTest so that it can be > used in _any_ endure test. https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:749: self._GmailSetUp(test_description) Please see the above comment. On 2012/08/01 01:03:00, Nirnimesh wrote: > Why not do this it setUp instead of calling before each test?
https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:640: self._StartReplayServerIfNecessary(archive_name) On 2012/08/01 17:14:57, fdeng1 wrote: > When starting a replay server, an "archive_name" need to be passed to > ChromeEndureReplay.ReplayServer(cls, archive_name). For now, archive_name is > unique for each test because each test is using its own archive. I was trying to > put self._StartReplayServerIfNecessary(archive_name) is in > ChromeEndureBaseTest.setUp() or ChromeEndureGmailTest.setUp(), but I realized I > was unable to know which test is about run in the setUp phase and as a result I > was unable to pick a unique archive_name for that test. Is there a way to know > which test is about to run during the setUp phase? > self.id(), as discussed offline > (I was also thinking put all archives into a big one which will be shared by all > tests. But the performance of WPR degrades a lot when the archive is big.) > > On 2012/08/01 01:03:00, Nirnimesh wrote: > > I thought this was going to be done in ChromeEndureBaseTest so that it can be > > used in _any_ endure test. > We'll likely be able to remove all of these _XSetUp() functions.
https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/20004/chrome/test/functi... chrome/test/functional/perf_endure.py:640: self._StartReplayServerIfNecessary(archive_name) I just talked to Dennis and we came up a plan to combine archives for the same webapp into one single archive, i.e. we will have Gmail.wpr, Plus.wpr, Docs.wpr. I will put _GmailSetUp in setUp() function in a coming patch. On 2012/08/01 17:14:57, fdeng1 wrote: > When starting a replay server, an "archive_name" need to be passed to > ChromeEndureReplay.ReplayServer(cls, archive_name). For now, archive_name is > unique for each test because each test is using its own archive. I was trying to > put self._StartReplayServerIfNecessary(archive_name) is in > ChromeEndureBaseTest.setUp() or ChromeEndureGmailTest.setUp(), but I realized I > was unable to know which test is about run in the setUp phase and as a result I > was unable to pick a unique archive_name for that test. Is there a way to know > which test is about to run during the setUp phase? > > (I was also thinking put all archives into a big one which will be shared by all > tests. But the performance of WPR degrades a lot when the archive is big.) > > On 2012/08/01 01:03:00, Nirnimesh wrote: > > I thought this was going to be done in ChromeEndureBaseTest so that it can be > > used in _any_ endure test. >
Hi Dennis and Nirnimesh, I've addressed Nirnimesh's comments on moving some wpr related code into ChromeEndureBaseTest. I've also add a script for recording purpose as discussed with Dennis. Pleas take a look and let me know any issues and area for improvements. Thank you! Fang https://chromiumcodereview.appspot.com/10803002/diff/26005/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/26005/chrome/test/functi... chrome/test/functional/perf_endure.py:154: self._StopReplayServerIfNecessary() StopReplayServerIfNecessary()/StartReplayServerIfNecessary() are in tearDown/setUp of the base class now. https://chromiumcodereview.appspot.com/10803002/diff/26005/chrome/test/functi... chrome/test/functional/perf_endure.py:172: Two tests are offline and don't need WPR. These tests can override this function to turn off WPR support. https://chromiumcodereview.appspot.com/10803002/diff/26005/chrome/test/functi... chrome/test/functional/perf_endure.py:184: I add a function called _GenArchiveName which uses test class name as archive file name. I feel this can bring some freedom for tests to customize which archive they want to use. For example, we may break one archive into smaller one if it is too big or one archive is shared by multiple tests. Please let me know if there is better way to go here.
https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:164: return False Change this function so that by default, tests will not have WPR support. Tests need to explicitly say they want to use wpr (and then they will need to create the archives). And for those tests which won't use WPR, they simply don't need to care about anything. Is this more preferable than the other way(be default WPR is on)?
https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:69: self._use_wpr = self._NeedReplayServer() As discussed offline, let's follow this behavior: (1) by default, a test will use WPR if it finds the appropriate archive file in the expected location. Otherwise, it will not use WPR. In either case, log a message at the start of the test indicating whether WPR is being used or not. (2) If ENDURE_NO_WPR is specified, then simply do not use WPR. Log a message before the test starts saying that WPR has been turned off for the test. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:70: # Do NOT mannually set WPR_RECORD for the purpose of mannually --> manually https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:164: return False On 2012/08/03 16:47:06, fdeng1 wrote: > Change this function so that by default, tests will not have WPR support. Tests > need to explicitly say they want to use wpr (and then they will need to create > the archives). And for those tests which won't use WPR, they simply don't need > to care about anything. Is this more preferable than the other way(be default > WPR is on)? Let's get rid of this function completely. If a test doesn't need to use WPR, then there will simply be no archive file for it, so by default the test will just say that it didn't find an archive file so it's not using WPR. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:171: make a note that this assumes that we only have 1 archive file per test class. i'm fine with that assumption for now. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... File chrome/test/functional/record_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:5: """Record webapp interactions using Web Page Replay for Endure test. add a blank line right above this https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:7: Example: before giving specific examples, mention the general form of the command to run this script, something like this: ./record_endure.py [options] archive_file_name tests_to_run And mention what happens by default (e.g., each test runs for 30 seconds and the archive file gets stored into a some default location). https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:24: import optparse add a blank line right above this https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:28: import perf add a blank line right above this to separate standard library imports from application-specific imports https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:46: 'out_dir': rather than having 2 inputs to represent the archive file (one optional parameter for the path and one required argument for the archive file name), could we just combine the two into one optional input named "--archive-file" that has both the path and the file name in the string? By default, we can have it create a file "archive.wpr" in the current working directory. Otherwise the user can specify a new file like this: ./record_endure --archive-file=<full path and file name of the wpr file to create> https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:50: 'perf_endure': maybe we can remove this and just assume that perf_endure.py exists in the current working directory. We can just list that as an assumption of this script in the docstring at the top of the file https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:57: usage='%prog [options] archive_name test1 [test2]...', test1 --> test_or_suite_1 test2 --> test_or_suite_2 (just to highlight the fact that they can be individual test names or groups of tests, since pyauto allows for both to be specified on the command line) https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:99: subprocess.call(cmd, env=env) i think it's possible that an archive file might still be created even though one or more pyauto tests fail. We should probably check the return code of pyauto itself to make sure it's 0. If it's not 0, then some pyauto test probably failed and we should alert the user that even though an archive file was created, it might be invalid.
Hi Dennis, I addressed your comments. Please take a look. Thanks, Fang https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:69: self._use_wpr = self._NeedReplayServer() On 2012/08/03 19:39:54, dennis_jeffrey wrote: > As discussed offline, let's follow this behavior: > > (1) by default, a test will use WPR if it finds the appropriate archive file in > the expected location. Otherwise, it will not use WPR. In either case, log a > message at the start of the test indicating whether WPR is being used or not. > > (2) If ENDURE_NO_WPR is specified, then simply do not use WPR. Log a message > before the test starts saying that WPR has been turned off for the test. Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:70: # Do NOT mannually set WPR_RECORD for the purpose of On 2012/08/03 19:39:54, dennis_jeffrey wrote: > mannually --> manually Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:164: return False On 2012/08/03 19:39:54, dennis_jeffrey wrote: > On 2012/08/03 16:47:06, fdeng1 wrote: > > Change this function so that by default, tests will not have WPR support. > Tests > > need to explicitly say they want to use wpr (and then they will need to create > > the archives). And for those tests which won't use WPR, they simply don't need > > to care about anything. Is this more preferable than the other way(be default > > WPR is on)? > > Let's get rid of this function completely. If a test doesn't need to use WPR, > then there will simply be no archive file for it, so by default the test will > just say that it didn't find an archive file so it's not using WPR. Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/perf_endure.py:171: Added a note to doc string On 2012/08/03 19:39:54, dennis_jeffrey wrote: > make a note that this assumes that we only have 1 archive file per test class. > i'm fine with that assumption for now. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... File chrome/test/functional/record_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:5: """Record webapp interactions using Web Page Replay for Endure test. On 2012/08/03 19:39:54, dennis_jeffrey wrote: > add a blank line right above this Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:7: Example: On 2012/08/03 19:39:54, dennis_jeffrey wrote: > before giving specific examples, mention the general form of the command to run > this script, something like this: > > ./record_endure.py [options] archive_file_name tests_to_run > > And mention what happens by default (e.g., each test runs for 30 seconds and the > archive file gets stored into a some default location). Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:24: import optparse On 2012/08/03 19:39:54, dennis_jeffrey wrote: > add a blank line right above this Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:28: import perf On 2012/08/03 19:39:54, dennis_jeffrey wrote: > add a blank line right above this to separate standard library imports from > application-specific imports Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:46: 'out_dir': On 2012/08/03 19:39:54, dennis_jeffrey wrote: > rather than having 2 inputs to represent the archive file (one optional > parameter for the path and one required argument for the archive file name), > could we just combine the two into one optional input named "--archive-file" > that has both the path and the file name in the string? > > By default, we can have it create a file "archive.wpr" in the current working > directory. Otherwise the user can specify a new file like this: > > ./record_endure --archive-file=<full path and file name of the wpr file to > create> Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:50: 'perf_endure': On 2012/08/03 19:39:54, dennis_jeffrey wrote: > maybe we can remove this and just assume that perf_endure.py exists in the > current working directory. We can just list that as an assumption of this > script in the docstring at the top of the file Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:57: usage='%prog [options] archive_name test1 [test2]...', On 2012/08/03 19:39:54, dennis_jeffrey wrote: > test1 --> test_or_suite_1 > test2 --> test_or_suite_2 > > (just to highlight the fact that they can be individual test names or groups of > tests, since pyauto allows for both to be specified on the command line) Done. https://chromiumcodereview.appspot.com/10803002/diff/23007/chrome/test/functi... chrome/test/functional/record_endure.py:99: subprocess.call(cmd, env=env) On 2012/08/03 19:39:54, dennis_jeffrey wrote: > i think it's possible that an archive file might still be created even though > one or more pyauto tests fail. We should probably check the return code of > pyauto itself to make sure it's 0. If it's not 0, then some pyauto test > probably failed and we should alert the user that even though an archive file > was created, it might be invalid. Done.
I had to re-add some comments because one of the recent patches was deleted. :-/ The patch is looking good. Here are a couple comments. http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... chrome/test/functional/perf_endure.py:85: ' archive file %s does not exist.', How about the following? Skipping Web Page Replay since the archive file, %s, does not exist. http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... File chrome/test/functional/record_endure.py (right): http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... chrome/test/functional/record_endure.py:76: 'webpagereplay/wpr_deterministic.js' It is redundant to have this path here. Is there a way to have perf_endure.py run WPR instead of doing it here?
https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:71: logging.info('Web Page Replay is turned off as ENDURE_NO_WPR is set.') how about this to match slamm's recommended comment below: "Skipping Web Page Replay since ENDURE_NO_WPR is set". https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:80: self._use_wpr = True log a message in this case saying something like "Using Web Page Replay". https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... File chrome/test/functional/record_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/record_endure.py:18: 2) ./record_endure.py --test-length=40 \\ just put a single \ at the end of this line, not 2 of them https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/record_endure.py:19: --archive-file="/home/user/ChromeEndureGmailTest.wpr" \\ just put a single \ at the end of this line, not 2 of them https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/record_endure.py:76: 'webpagereplay/wpr_deterministic.js' On 2012/08/06 20:58:44, slamm wrote: > It is redundant to have this path here. > Is there a way to have perf_endure.py run WPR instead of doing it here? We need to be able to start WPR in record mode, run one or more pyauto tests, then stop WPR. This is so that we can use multiple pyauto tests to record a single archive file (e.g, some tests that are very similar probably shouldn't have different archive files, since many things would be duplicated in the archives). This script is really just a wrapper around pyauto to (1) start WPR; (2) run one or more pyauto tests; (3) stop WPR. We need this because perf_endure.py would otherwise start and stop WPR for each individual test. If WPR had an "append" mode for recording archives, we could use that instead. Does it have such a feature, or would that be easy to add? If that was available, we could start and stop WPR for each test but still have the same archive file get updated.
Steve, I am sorry for that, I was cleaning some patches. I didn't notice there was a comment attached to it. Fang On Mon, Aug 6, 2012 at 1:58 PM, <slamm@google.com> wrote: > > I had to re-add some comments because one of the recent patches was > deleted. :-/ > > The patch is looking good. Here are a couple comments. > > > http://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**perf_endure.py<http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py> > File chrome/test/functional/perf_**endure.py (right): > > http://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**perf_endure.py#newcode85<http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py#newcode85> > chrome/test/functional/perf_**endure.py:85: ' archive file %s does not > exist.', > How about the following? > > Skipping Web Page Replay since the archive file, %s, does not exist. > > http://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**record_endure.py<http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py> > File chrome/test/functional/record_**endure.py (right): > > http://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**record_endure.py#newcode76<http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode76> > chrome/test/functional/record_**endure.py:76: > 'webpagereplay/wpr_**deterministic.js' > It is redundant to have this path here. > Is there a way to have perf_endure.py run WPR instead of doing it here? > > http://chromiumcodereview.**appspot.com/10803002/<http://chromiumcodereview.a... >
http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... chrome/test/functional/perf_endure.py:67: # Parse the environment variable for the usage of Web Page Replay. please move the env parsing to a helper method so as not to litter this class with wpr stuff http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... chrome/test/functional/perf_endure.py:171: Use test class name as archive name, e.g. ChromeEndureGmailTest.wpr, While this is nifty to auto-generate the archive name, it hampers discoverability. Say I'm a random developer changing X.wpr and i want to find out all the tests that use it. I'll not find this file in code search. It's fine to just set a convention that the archive file matches the class name. It's not necessary to auto-generate the filename, I think. http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... chrome/test/functional/perf_endure.py:1209: # See the java script file for details. java script -> javascript http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... File chrome/test/functional/record_endure.py (right): http://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functio... chrome/test/functional/record_endure.py:9: ./record_endure.py [options] test_or_suite_1 [test_or_suite_2] ... remove ./ prefix python (so that it works on win)
On Mon, Aug 6, 2012 at 2:33 PM, <dennisjeffrey@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**perf_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py> > File chrome/test/functional/perf_**endure.py (right): > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**perf_endure.py#newcode71<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py#newcode71> > chrome/test/functional/perf_**endure.py:71: logging.info('Web Page Replay > is turned off as ENDURE_NO_WPR is set.') > how about this to match slamm's recommended comment below: > > "Skipping Web Page Replay since ENDURE_NO_WPR is set". > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**perf_endure.py#newcode80<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py#newcode80> > chrome/test/functional/perf_**endure.py:80: self._use_wpr = True > log a message in this case saying something like "Using Web Page > Replay". > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**record_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py> > File chrome/test/functional/record_**endure.py (right): > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**record_endure.py#newcode18<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode18> > chrome/test/functional/record_**endure.py:18: 2) ./record_endure.py > --test-length=40 \\ > just put a single \ at the end of this line, not 2 of them > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**record_endure.py#newcode19<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode19> > chrome/test/functional/record_**endure.py:19: > --archive-file="/home/user/**ChromeEndureGmailTest.wpr" \\ > just put a single \ at the end of this line, not 2 of them > > https://chromiumcodereview.**appspot.com/10803002/diff/** > 27005/chrome/test/functional/**record_endure.py#newcode76<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode76> > chrome/test/functional/record_**endure.py:76: > 'webpagereplay/wpr_**deterministic.js' > > On 2012/08/06 20:58:44, slamm wrote: > >> It is redundant to have this path here. >> Is there a way to have perf_endure.py run WPR instead of doing it >> > here? > > We need to be able to start WPR in record mode, run one or more pyauto > tests, then stop WPR. This is so that we can use multiple pyauto tests > to record a single archive file (e.g, some tests that are very similar > probably shouldn't have different archive files, since many things would > be duplicated in the archives). > > This script is really just a wrapper around pyauto to (1) start WPR; (2) > run one or more pyauto tests; (3) stop WPR. We need this because > perf_endure.py would otherwise start and stop WPR for each individual > test. > > If WPR had an "append" mode for recording archives, we could use that > instead. Does it have such a feature, or would that be easy to add? If > that was available, we could start and stop WPR for each test but still > have the same archive file get updated. > Replay does not have an append function. However, adding an "--append" option to replay.py would be trivial. -slamm https://chromiumcodereview.**appspot.com/10803002/<https://chromiumcodereview... >
Hi, Steve,
I take a look at replay.py, and I am thinking to modify the code in
replay.py(Line282) in the following way to enable "append" feature. How do
you think?
if options.record:
* if options.append and os.path.exist(replay_filename):*
* http_archive = httparchive.HttpArchive.Load(replay_filename)
*
* logging.info("Appending...")*
* else:*
* http_archive = httparchive.HttpArchive()*
* http_archive.AssertWritable(replay_filename)*
else:
http_archive = httparchive.HttpArchive.Load(replay_filename)
logging.info('Loaded %d responses from %s',
len(http_archive), replay_filename)
Fang
On Mon, Aug 6, 2012 at 2:56 PM, Stephen Lamm <slamm@google.com> wrote:
>
>
> On Mon, Aug 6, 2012 at 2:33 PM, <dennisjeffrey@chromium.org> wrote:
>
>>
>> https://chromiumcodereview.**appspot.com/10803002/diff/**
>>
27005/chrome/test/functional/**perf_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py>
>> File chrome/test/functional/perf_**endure.py (right):
>>
>> https://chromiumcodereview.**appspot.com/10803002/diff/**
>>
27005/chrome/test/functional/**perf_endure.py#newcode71<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py#newcode71>
>> chrome/test/functional/perf_**endure.py:71: logging.info('Web Page Replay
>> is turned off as ENDURE_NO_WPR is set.')
>> how about this to match slamm's recommended comment below:
>>
>> "Skipping Web Page Replay since ENDURE_NO_WPR is set".
>>
>> https://chromiumcodereview.**appspot.com/10803002/diff/**
>>
27005/chrome/test/functional/**perf_endure.py#newcode80<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py#newcode80>
>> chrome/test/functional/perf_**endure.py:80: self._use_wpr = True
>> log a message in this case saying something like "Using Web Page
>> Replay".
>>
>> https://chromiumcodereview.**appspot.com/10803002/diff/**
>>
27005/chrome/test/functional/**record_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py>
>> File chrome/test/functional/record_**endure.py (right):
>>
>> https://chromiumcodereview.**appspot.com/10803002/diff/**
>>
27005/chrome/test/functional/**record_endure.py#newcode18<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode18>
>> chrome/test/functional/record_**endure.py:18: 2) ./record_endure.py
>> --test-length=40 \\
>> just put a single \ at the end of this line, not 2 of them
>>
>> https://chromiumcodereview.**appspot.com/10803002/diff/**
>>
27005/chrome/test/functional/**record_endure.py#newcode19<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode19>
>> chrome/test/functional/record_**endure.py:19:
>> --archive-file="/home/user/**ChromeEndureGmailTest.wpr" \\
>> just put a single \ at the end of this line, not 2 of them
>>
>> https://chromiumcodereview.**appspot.com/10803002/diff/**
>>
27005/chrome/test/functional/**record_endure.py#newcode76<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode76>
>> chrome/test/functional/record_**endure.py:76:
>> 'webpagereplay/wpr_**deterministic.js'
>>
>> On 2012/08/06 20:58:44, slamm wrote:
>>
>>> It is redundant to have this path here.
>>> Is there a way to have perf_endure.py run WPR instead of doing it
>>>
>> here?
>>
>> We need to be able to start WPR in record mode, run one or more pyauto
>> tests, then stop WPR. This is so that we can use multiple pyauto tests
>> to record a single archive file (e.g, some tests that are very similar
>> probably shouldn't have different archive files, since many things would
>> be duplicated in the archives).
>>
>> This script is really just a wrapper around pyauto to (1) start WPR; (2)
>> run one or more pyauto tests; (3) stop WPR. We need this because
>> perf_endure.py would otherwise start and stop WPR for each individual
>> test.
>>
>> If WPR had an "append" mode for recording archives, we could use that
>> instead. Does it have such a feature, or would that be easy to add? If
>> that was available, we could start and stop WPR for each test but still
>> have the same archive file get updated.
>>
>
> Replay does not have an append function. However, adding an "--append"
> option to replay.py would be trivial.
>
> -slamm
>
>
https://chromiumcodereview.**appspot.com/10803002/<https://chromiumcodereview...
>>
>
>
I built on this and published a CL for review: http://chromiumcodereview.appspot.com/10829202/ -slamm On Mon, Aug 6, 2012 at 3:25 PM, Fang Deng <fdeng@google.com> wrote: > Hi, Steve, > > I take a look at replay.py, and I am thinking to modify the code in > replay.py(Line282) in the following way to enable "append" feature. How do > you think? > > if options.record: > * if options.append and os.path.exist(replay_filename):* > * http_archive = httparchive.HttpArchive.Load(replay_filename) > * > * logging.info("Appending...")* > * else:* > * http_archive = httparchive.HttpArchive()* > * http_archive.AssertWritable(replay_filename)* > else: > http_archive = httparchive.HttpArchive.Load(replay_filename) > logging.info('Loaded %d responses from %s', > len(http_archive), replay_filename) > > Fang > > On Mon, Aug 6, 2012 at 2:56 PM, Stephen Lamm <slamm@google.com> wrote: > >> >> >> On Mon, Aug 6, 2012 at 2:33 PM, <dennisjeffrey@chromium.org> wrote: >> >>> >>> https://chromiumcodereview.**appspot.com/10803002/diff/** >>> 27005/chrome/test/functional/**perf_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py> >>> File chrome/test/functional/perf_**endure.py (right): >>> >>> https://chromiumcodereview.**appspot.com/10803002/diff/** >>> 27005/chrome/test/functional/**perf_endure.py#newcode71<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py#newcode71> >>> chrome/test/functional/perf_**endure.py:71: logging.info('Web Page >>> Replay >>> is turned off as ENDURE_NO_WPR is set.') >>> how about this to match slamm's recommended comment below: >>> >>> "Skipping Web Page Replay since ENDURE_NO_WPR is set". >>> >>> https://chromiumcodereview.**appspot.com/10803002/diff/** >>> 27005/chrome/test/functional/**perf_endure.py#newcode80<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/perf_endure.py#newcode80> >>> chrome/test/functional/perf_**endure.py:80: self._use_wpr = True >>> log a message in this case saying something like "Using Web Page >>> Replay". >>> >>> https://chromiumcodereview.**appspot.com/10803002/diff/** >>> 27005/chrome/test/functional/**record_endure.py<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py> >>> File chrome/test/functional/record_**endure.py (right): >>> >>> https://chromiumcodereview.**appspot.com/10803002/diff/** >>> 27005/chrome/test/functional/**record_endure.py#newcode18<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode18> >>> chrome/test/functional/record_**endure.py:18: 2) ./record_endure.py >>> --test-length=40 \\ >>> just put a single \ at the end of this line, not 2 of them >>> >>> https://chromiumcodereview.**appspot.com/10803002/diff/** >>> 27005/chrome/test/functional/**record_endure.py#newcode19<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode19> >>> chrome/test/functional/record_**endure.py:19: >>> --archive-file="/home/user/**ChromeEndureGmailTest.wpr" \\ >>> just put a single \ at the end of this line, not 2 of them >>> >>> https://chromiumcodereview.**appspot.com/10803002/diff/** >>> 27005/chrome/test/functional/**record_endure.py#newcode76<https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functional/record_endure.py#newcode76> >>> chrome/test/functional/record_**endure.py:76: >>> 'webpagereplay/wpr_**deterministic.js' >>> >>> On 2012/08/06 20:58:44, slamm wrote: >>> >>>> It is redundant to have this path here. >>>> Is there a way to have perf_endure.py run WPR instead of doing it >>>> >>> here? >>> >>> We need to be able to start WPR in record mode, run one or more pyauto >>> tests, then stop WPR. This is so that we can use multiple pyauto tests >>> to record a single archive file (e.g, some tests that are very similar >>> probably shouldn't have different archive files, since many things would >>> be duplicated in the archives). >>> >>> This script is really just a wrapper around pyauto to (1) start WPR; (2) >>> run one or more pyauto tests; (3) stop WPR. We need this because >>> perf_endure.py would otherwise start and stop WPR for each individual >>> test. >>> >>> If WPR had an "append" mode for recording archives, we could use that >>> instead. Does it have such a feature, or would that be easy to add? If >>> that was available, we could start and stop WPR for each test but still >>> have the same archive file get updated. >>> >> >> Replay does not have an append function. However, adding an "--append" >> option to replay.py would be trivial. >> >> -slamm >> >> https://chromiumcodereview.**appspot.com/10803002/<https://chromiumcodereview... >>> >> >> >
Hi, I've addressed the comments. I get rid of record_endure.py. Now recording mode is controlled by WPR_RECORD. With the patch from Steve's CL, it should be appending to the existing WPR archives. Please take another look. Thanks, Fang https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:67: # Parse the environment variable for the usage of Web Page Replay. On 2012/08/06 21:36:45, Nirnimesh wrote: > please move the env parsing to a helper method so as not to litter this class > with wpr stuff Done. move it to _ParseReplayEnv() https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:71: logging.info('Web Page Replay is turned off as ENDURE_NO_WPR is set.') On 2012/08/06 21:33:46, dennis_jeffrey wrote: > how about this to match slamm's recommended comment below: > > "Skipping Web Page Replay since ENDURE_NO_WPR is set". Done. https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:80: self._use_wpr = True When WPR is successfully launched, _StartReplayServerIfNecessary will call "logging.info('Web Page Replay server has started in %s mode.', mode)". Do you think that is sufficient or maybe one more log here is better? On 2012/08/06 21:33:46, dennis_jeffrey wrote: > log a message in this case saying something like "Using Web Page Replay". https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:85: ' archive file %s does not exist.', On 2012/08/06 20:58:44, slamm wrote: > How about the following? > > Skipping Web Page Replay since the archive file, %s, does not exist. Done. https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:171: Use test class name as archive name, e.g. ChromeEndureGmailTest.wpr, As discussed offline, I override _GetArchiveName in each base test class which returns the right archive name for that class. On 2012/08/06 21:36:45, Nirnimesh wrote: > While this is nifty to auto-generate the archive name, it hampers > discoverability. > Say I'm a random developer changing X.wpr and i want to find out all the tests > that use it. I'll not find this file in code search. > > It's fine to just set a convention that the archive file matches the class name. > It's not necessary to auto-generate the filename, I think. https://chromiumcodereview.appspot.com/10803002/diff/27005/chrome/test/functi... chrome/test/functional/perf_endure.py:1209: # See the java script file for details. On 2012/08/06 21:36:45, Nirnimesh wrote: > java script -> javascript Done. https://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functi... chrome/test/functional/perf_endure.py:184: self._use_wpr = True In record mode, we just need an valid archive file path. It is ok if the archive doesn't exist. https://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functi... chrome/test/functional/perf_endure.py:192: self._use_wpr = True In replay mode, we need an valid archive file path and the file must exist.
LGTM. Thanks for all the iterations. https://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functi... chrome/test/functional/perf_endure.py:21: it will append to the old one. remove 'please'
LGTM with a nit. Please hold off on submitting until all of the following are true: (1) slamm@ has approved this CL (2) gregsimon@ agrees to let this change go live (3) slamm@'s recent WPR change to add append mode is pulled into the chrome tree via the proper DEPS change. https://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functi... chrome/test/functional/perf_endure.py:197: self._archive_path+' ' if self._archive_path else '') add a space right before and right after the +
LGTM with some minor comments. http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:74: # It must be parsed before perf.BasePerfTest.setUp() Better? # The Web Page Replay environment variables must be parsed before # perf.BasePerfTest.setUp(). http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:605: to go through the Web Page Replay server) occurs. Does this need to be said? http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:1237: """Creat a replay server.""" Creat -> Create http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:1242: raise webpagereplay.ReplayNotFoundError('injected scripts', scripts) Instead of using a custom exception from webpagereplay, how about either creating an exception for this module, or raise something like IOError?
Dennis, Nirnimesh and Steve, Thanks for your comments. I addressed them. As Dennis said, I will hold for a while before all the perquisites are met. Thanks a lot. Fang http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:21: it will append to the old one. On 2012/08/07 17:17:34, Nirnimesh wrote: > remove 'please' Done. http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:74: # It must be parsed before perf.BasePerfTest.setUp() On 2012/08/07 20:00:43, slamm wrote: > Better? > > # The Web Page Replay environment variables must be parsed before > # perf.BasePerfTest.setUp(). Done. http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:197: self._archive_path+' ' if self._archive_path else '') On 2012/08/07 19:42:09, dennis_jeffrey wrote: > add a space right before and right after the + Done. http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:605: to go through the Web Page Replay server) occurs. Deleted. On 2012/08/07 20:00:43, slamm wrote: > Does this need to be said? http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:1237: """Creat a replay server.""" On 2012/08/07 20:00:43, slamm wrote: > Creat -> Create Done. http://chromiumcodereview.appspot.com/10803002/diff/22010/chrome/test/functio... chrome/test/functional/perf_endure.py:1242: raise webpagereplay.ReplayNotFoundError('injected scripts', scripts) Change to IOError On 2012/08/07 20:00:43, slamm wrote: > Instead of using a custom exception from webpagereplay, how about either > creating an exception for this module, or raise something like IOError?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fdeng@chromium.org/10803002/37001
Change committed as 155473 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
