Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10825025:
Make it easy to use Web Page Replay with non-page_cycler tests. (Closed)
|
Created:
8 years, 5 months ago by slamm_google Modified:
8 years, 4 months ago CC:
chromium-reviews, dennis_jeffrey, anantha, dyu1, Nirnimesh, tonyg Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake it easy to use Web Page Replay with non-page_cycler tests.
BUG=
TEST=
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148828
Patch Set 1 #Patch Set 2 : Minimize CL diff. Fix errors. Remove dead code. #Patch Set 3 : few more tweaks #Patch Set 4 : minimize CL diff #
Total comments: 2
Patch Set 5 : Fold PerfReplayServer into ReplayServer. Add FormatChromiumPath (was ChromiumPaths class) #Patch Set 6 : Make replay_options the first optional arg. #Patch Set 7 : Rename PageCyclerWebPageReplay to shorter PageCyclerReplay. #
Total comments: 16
Patch Set 8 : Fixes for review comments. #
Total comments: 1
Patch Set 9 : Move FormatChromiumPath to perf.py #Patch Set 10 : Fix webpagereplay.__enter__ (needed to "return self"). #
Total comments: 6
Patch Set 11 : Apply Dennis' suggestions #
Messages
Total messages: 19 (0 generated)
Hi Fang, Here are the changes I had in mind for your other change (https://chromiumcodereview.appspot.com/10803002/). Does this organization make sense to you? And, would this make it easy enough to add the endure tests? -slamm
Hi Steve, Thanks for working on this. I like your restructure and I feel it looks nicer after many things are moved to webpagereplay.py. For endure tests, I think I'll have a class EndureWebPageReplay which is similar to PageCycleWebPageReplay. But in addition, I need to pass an extra flag to repaly server. Please take a look at the inline comment. I put my question there. Thanks for your help! Fang https://chromiumcodereview.appspot.com/10825025/diff/5001/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10825025/diff/5001/chrome/test/functio... chrome/test/functional/perf.py:2214: return webpagereplay.PerfReplayServer(archive_path) For Chrome Endure tests, I need to pass "--inject-scripts=...src/chrome/test/data/chrome_endure/wpr_deterministic.js" to the replay server. Do you think it might be a good idea that PerfReplayServer.__init__ takes additional replay options, e.g. PerfReplayServer.__init__(self, archive, extra_wpr_options)?
https://chromiumcodereview.appspot.com/10825025/diff/5001/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10825025/diff/5001/chrome/test/functio... chrome/test/functional/perf.py:2214: return webpagereplay.PerfReplayServer(archive_path) The latest CL folds PerfReplayServer into ReplayServer. That takes |replay_options| as the first keyword arg: class ReplayServer: ... def __init__(self, archive_path, replay_options=None, replay_dir=None, log_path=None): On 2012/07/26 07:27:10, fdeng1 wrote: > For Chrome Endure tests, I need to pass > "--inject-scripts=...src/chrome/test/data/chrome_endure/wpr_deterministic.js" to > the replay server. Do you think it might be a good idea that > PerfReplayServer.__init__ takes additional replay options, e.g. > PerfReplayServer.__init__(self, archive, extra_wpr_options)?
Hi Steve, The current structure looks good to me. I am trying to patch your change and using it for chrome endure tests. I put some comments about some small issues I came across. Thanks, Fang https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/perf.py:1778: with PageCyclerReplay.ReplayServer(test_name): I think change it to PageCyclerReplay()? https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/perf.py:2248: with PageCyclerReplay.ReplayServer(test_name) as replay_server: ->PageCyclerReplay() https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/perf.py:2250: self._num_iterations = 1 I haven't figure out why, but it complains replay_server is None. And do we need add this piece of code also to PopularSitesScrollTest.test2012Q3? https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:94: replay_options.append('--record') -> self.replay.options.append('--record') https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:96: self._AddDefaultReplayOptions(self) remove the argument "self"? https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:119: 'Archive directory does not exist: %s' % self.archive_dir self.archive_dir -> archive_dir https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:122: 'Archive file path does not exist: %s' % archive_path In above two lines: archive_path -> self.archive_path
https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:94: replay_options.append('--record') sorry, should be self.replay_options.append(...) On 2012/07/26 20:51:19, fdeng1 wrote: > -> self.replay.options.append('--record')
Thanks for the comments. I uploaded fixes. BTW, I am going to be out Friday morning. https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/perf.py:1778: with PageCyclerReplay.ReplayServer(test_name): On 2012/07/26 20:51:19, fdeng1 wrote: > I think change it to PageCyclerReplay()? Thanks for catching that. I changed ReplayServer to a classmethod. https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/perf.py:2250: self._num_iterations = 1 On 2012/07/26 20:51:19, fdeng1 wrote: > I haven't figure out why, but it complains replay_server is None. > > And do we need add this piece of code also to PopularSitesScrollTest.test2012Q3? Not sure about the None error. Try it again with ReplayServer as classmethod. I added the num_iterations fix to PopularSitesScrollTest. https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:43: def FormatChromiumPath(posix_path, **kwargs): How about I move this to perf.py:PageCyclerReplay since it is not used here? https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:94: replay_options.append('--record') On 2012/07/26 20:51:19, fdeng1 wrote: > -> self.replay.options.append('--record') Done. https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:96: self._AddDefaultReplayOptions(self) On 2012/07/26 20:51:19, fdeng1 wrote: > remove the argument "self"? Done. https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:119: 'Archive directory does not exist: %s' % self.archive_dir On 2012/07/26 20:51:19, fdeng1 wrote: > self.archive_dir -> archive_dir I dropped the asserts in favor of raising exceptions.
Hi Steve, Thanks for letting me know. This Cl looks good to me now, except I am still have trouble with this line(perf.py:1779) "if replay_server.is_record_mode:". Are you able to run it without such error? Thanks, Fang https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/perf.py:2250: self._num_iterations = 1 On 2012/07/26 23:46:59, slamm wrote: > On 2012/07/26 20:51:19, fdeng1 wrote: > > I haven't figure out why, but it complains replay_server is None. > > > > And do we need add this piece of code also to > PopularSitesScrollTest.test2012Q3? > > Not sure about the None error. Try it again with ReplayServer as classmethod. > > I added the num_iterations fix to PopularSitesScrollTest. I am still have the same trouble after patching the new changes. fdeng@fdeng:/usr/local/google/chrome/src/chrome/test/functional$ WPR_RECORD=1 python perf.py perf.PopularSitesScrollTest.test2012Q3 Will not try to restart with the correct version of python as DO_NOT_RESTART_PYTHON_FOR_PYAUTO is set. HTTP server started on 127.0.0.1:40036... sending server_data: {"host": "127.0.0.1", "port": 40036} (36 bytes) [ RUN ] perf.PopularSitesScrollTest.test2012Q3: "None" 2012-07-26 17:42:13,884 INFO Starting to wait up to 60.000000s for idle CPU... 2012-07-26 17:42:15,888 INFO Current CPU utilization = 0.007940. 2012-07-26 17:42:15,888 INFO Wait for idle CPU took 2.000000s (utilization = 0.007940). [ ERROR ] perf.PopularSitesScrollTest.test2012Q3 ====================================================================== ERROR: perf.PopularSitesScrollTest.test2012Q3: "None" ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/local/google/chrome/src/chrome/test/functional/perf.py", line 1779, in test2012Q3 if replay_server.is_record_mode: AttributeError: 'NoneType' object has no attribute 'is_record_mode' ---------------------------------------------------------------------- Ran 1 test in 7.328s https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10825025/diff/3002/chrome/test/functio... chrome/test/functional/webpagereplay.py:43: def FormatChromiumPath(posix_path, **kwargs): On 2012/07/26 23:46:59, slamm wrote: > How about I move this to perf.py:PageCyclerReplay since it is not used here? Yes, I agree it makes more sense to be that way. Maybe merge it to "PageCyclerReplay.Path(cls, key, **kwargs)"? The only downside I see is that perf_endure.py:ChromeEndureReplay will be using it too so that we will have some duplicated code. I think it is fine.
Hi Steve, It seems that "return self" needs to be added at the end of __enter__. Besides, Are you expecting to check in this CL before your vacation? Thanks, Fang https://chromiumcodereview.appspot.com/10825025/diff/7005/chrome/test/functio... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10825025/diff/7005/chrome/test/functio... chrome/test/functional/webpagereplay.py:187: self.StartServer() I figure it out. need to add "return self" here so that"with ... as replay_server" will work.
PTAL. I think this is ready to go.
LGTM with a few nits to consider. Thank you! https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... chrome/test/functional/perf.py:58: _BASE_DIR = os.path.abspath(os.path.join( maybe name it CHROME_BASE_DIR to indicate what kind of base directory it is? Or do you think that's implied? https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... chrome/test/functional/perf.py:70: Returns: add a blank line above this https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... chrome/test/functional/webpagereplay.py:31: _BASE_DIR = os.path.abspath(os.path.join( maybe name it _CHROME_BASE_DIR?
Thanks for the suggestions. https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... chrome/test/functional/perf.py:58: _BASE_DIR = os.path.abspath(os.path.join( On 2012/07/27 21:46:56, dennis_jeffrey wrote: > maybe name it CHROME_BASE_DIR to indicate what kind of base directory it is? Or > do you think that's implied? Done. https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... chrome/test/functional/perf.py:70: Returns: On 2012/07/27 21:46:56, dennis_jeffrey wrote: > add a blank line above this Done. https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... File chrome/test/functional/webpagereplay.py (right): https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... chrome/test/functional/webpagereplay.py:31: _BASE_DIR = os.path.abspath(os.path.join( On 2012/07/27 21:46:56, dennis_jeffrey wrote: > maybe name it _CHROME_BASE_DIR? Done.
Still LGTM. Thanks for addressing the nits.
On 2012/07/27 21:54:31, slamm wrote: > Thanks for the suggestions. > > https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... > File chrome/test/functional/perf.py (right): > > https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... > chrome/test/functional/perf.py:58: _BASE_DIR = os.path.abspath(os.path.join( > On 2012/07/27 21:46:56, dennis_jeffrey wrote: > > maybe name it CHROME_BASE_DIR to indicate what kind of base directory it is? > Or > > do you think that's implied? > > Done. > > https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... > chrome/test/functional/perf.py:70: Returns: > On 2012/07/27 21:46:56, dennis_jeffrey wrote: > > add a blank line above this > > Done. > > https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... > File chrome/test/functional/webpagereplay.py (right): > > https://chromiumcodereview.appspot.com/10825025/diff/12005/chrome/test/functi... > chrome/test/functional/webpagereplay.py:31: _BASE_DIR = > os.path.abspath(os.path.join( > On 2012/07/27 21:46:56, dennis_jeffrey wrote: > > maybe name it _CHROME_BASE_DIR? > > Done. LGTM. Thanks for your help Steve! Fang
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/slamm@google.com/10825025/12006
Presubmit check for 10825025-12006 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/test/functional
Presubmit checks took 3.1s to calculate.
rubberstamp LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/slamm@google.com/10825025/12006
Change committed as 148828 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
