Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9965045:
Add Web Page Replay support. (Closed)
|
Created:
8 years, 8 months ago by slamm_google Modified:
8 years, 7 months ago CC:
chromium-reviews, nsylvain+cc_chromium.org, cmp+cc_chromium.org Visibility:
Public. |
DescriptionAdd Web Page Replay support.
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 1
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 3
Patch Set 11 : #Messages
Total messages: 29 (0 generated)
This is CL #3 of 3:
1. "webpagereplay/2012Q2/start.html" test and supporting files.
(Google internal tree)
2. performance_ui_test changes and Chrome extension to load pages:
https://chromiumcodereview.appspot.com/9956045
3. runtest.py and WPR import:
https://chromiumcodereview.appspot.com/9965045
On 2012/04/04 22:35:21, slamm wrote: > This is CL #3 of 3: > > 1. "webpagereplay/2012Q2/start.html" test and supporting files. > (Google internal tree) > 2. performance_ui_test changes and Chrome extension to load pages: > https://chromiumcodereview.appspot.com/9956045 > 3. runtest.py and WPR import: > https://chromiumcodereview.appspot.com/9965045 Caveat: I've never added third party code. For some projects, we just specify them in DEPS instead of copying the whole thing into third_party. Can we do that here too? It seems like there are lots of other Google Code projects specified that way. We own both repos, so it's not like we need a local branch or anything.
On Wed, Apr 4, 2012 at 4:06 PM, <simonjam@chromium.org> wrote: > On 2012/04/04 22:35:21, slamm wrote: > >> This is CL #3 of 3: >> > > 1. "webpagereplay/2012Q2/start.**html" test and supporting files. >> (Google internal tree) >> 2. performance_ui_test changes and Chrome extension to load pages: >> https://chromiumcodereview.**appspot.com/9956045<https://chromiumcodereview.a... >> 3. runtest.py and WPR import: >> https://chromiumcodereview.**appspot.com/9965045<https://chromiumcodereview.a... >> > > Caveat: I've never added third party code. > > For some projects, we just specify them in DEPS instead of copying the > whole > thing into third_party. Can we do that here too? It seems like there are > lots of > other Google Code projects specified that way. We own both repos, so it's > not > like we need a local branch or anything. > > https://chromiumcodereview.**appspot.com/9965045/<https://chromiumcodereview.... That seems like a much better way to go. I do not plan on having any Chrome-specific changes. -slamm
On Wed, Apr 4, 2012 at 4:32 PM, Stephen Lamm <slamm@google.com> wrote: > On Wed, Apr 4, 2012 at 4:06 PM, <simonjam@chromium.org> wrote: > >> On 2012/04/04 22:35:21, slamm wrote: >> >>> This is CL #3 of 3: >>> >> >> 1. "webpagereplay/2012Q2/start.**html" test and supporting files. >>> (Google internal tree) >>> 2. performance_ui_test changes and Chrome extension to load pages: >>> https://chromiumcodereview.**appspot.com/9956045<https://chromiumcodereview.a... >>> 3. runtest.py and WPR import: >>> https://chromiumcodereview.**appspot.com/9965045<https://chromiumcodereview.a... >>> >> >> Caveat: I've never added third party code. >> >> For some projects, we just specify them in DEPS instead of copying the >> whole >> thing into third_party. Can we do that here too? It seems like there are >> lots of >> other Google Code projects specified that way. We own both repos, so it's >> not >> like we need a local branch or anything. >> >> https://chromiumcodereview.**appspot.com/9965045/<https://chromiumcodereview.... > > > That seems like a much better way to go. I do not plan on having any > Chrome-specific changes. > I uploaded the change to DEPS. -slamm
LGTM https://chromiumcodereview.appspot.com/9965045/diff/10001/scripts/slave/runte... File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/9965045/diff/10001/scripts/slave/runte... scripts/slave/runtest.py:175: def start_wpr_server(platform, build_dir, test_exe_path, data_dir, platform is unused. https://chromiumcodereview.appspot.com/9965045/diff/10001/scripts/slave/runte... scripts/slave/runtest.py:567: sys.stderr.write('--enable-wpr requires --with-httpd for the document root.') 80 chars
On Wed, Apr 4, 2012 at 5:34 PM, <simonjam@chromium.org> wrote: > LGTM > > > https://chromiumcodereview.**appspot.com/9965045/diff/** > 10001/scripts/slave/runtest.py<https://chromiumcodereview.appspot.com/9965045/diff/10001/scripts/slave/runtest.py> > File scripts/slave/runtest.py (right): > > https://chromiumcodereview.**appspot.com/9965045/diff/** > 10001/scripts/slave/runtest.**py#newcode175<https://chromiumcodereview.appspot.com/9965045/diff/10001/scripts/slave/runtest.py#newcode175> > scripts/slave/runtest.py:175: def start_wpr_server(platform, build_dir, > test_exe_path, data_dir, > platform is unused. > Thanks. I originally thought I would need it for different archives on different platforms. I can always add it back if that proves to be true. Thanks for the quick reviews. I uploaded the updated CL. -slamm > https://chromiumcodereview.**appspot.com/9965045/diff/** > 10001/scripts/slave/runtest.**py#newcode567<https://chromiumcodereview.appspot.com/9965045/diff/10001/scripts/slave/runtest.py#newcode567> > scripts/slave/runtest.py:567: sys.stderr.write('--enable-wpr requires > --with-httpd for the document root.') > 80 chars > > https://chromiumcodereview.**appspot.com/9965045/<https://chromiumcodereview.... >
Hi Stephen, I can't do lgtm-, but please hold off on landing this just yet. I would really like to see this code stored in chrome/trunk/src/ instead of chrome/trunk/tools/build/. I would expect that the web page replay tests are going to be run from files in chrome/trunk/src/, and the server code is very light and can be invoked from those tests directly. If it was in chrome/trunk/src/, then developers could easily get a local instance of the server running without needing another repo or checkout. Did you consider adding this test to chrome/trunk/src/ and decide against that? Thanks, Chase https://chromiumcodereview.appspot.com/9965045/diff/13002/scripts/slave/webpa... File scripts/slave/webpagereplay.py (right): https://chromiumcodereview.appspot.com/9965045/diff/13002/scripts/slave/webpa... scripts/slave/webpagereplay.py:12: class ReplayLauncher: change this to ReplayLauncher(object)
On Thu, Apr 5, 2012 at 6:07 PM, <cmp@google.com> wrote: > Hi Stephen, > > I can't do lgtm-, but please hold off on landing this just yet. I would > really > like to see this code stored in chrome/trunk/src/ instead of > chrome/trunk/tools/build/. I would expect that the web page replay tests > are > going to be run from files in chrome/trunk/src/, and the server code is > very > light and can be invoked from those tests directly. If it was in > chrome/trunk/src/, then developers could easily get a local instance of the > server running without needing another repo or checkout. > > Did you consider adding this test to chrome/trunk/src/ and decide against > that? > > Thanks, > Chase > Would you prefer something like src/tools/python/google/webpagereplay_utils.py? Perhaps, the main() of that new file could start-up for adhoc testing. I could even make the main() launch Chromium with the right options. If we left things as there currently stand, the commands for running WPR from the command-line are not too bad: https://sites.google.com/a/google.com/page-cyclers-wpr<https://sites.google.c... / (see Quickstart) The real problem with public testing is that start.html, report.html, etc. are all in the private tree. The existing page cycler forked the files to the public tree. One thing I could do there, is only put the "data.wpr" file in the private tree. I could put the url list (test.js) and the other files in the public tree. I am unsure if we would be concern about revealing the url list. -slamm https://chromiumcodereview.**appspot.com/9965045/diff/**13002/scripts/slave/ > **webpagereplay.py<https://chromiumcodereview.appspot.com/9965045/diff/13002/scripts/slave/webpagereplay.py> > File scripts/slave/webpagereplay.py (right): > > https://chromiumcodereview.**appspot.com/9965045/diff/** > 13002/scripts/slave/**webpagereplay.py#newcode12<https://chromiumcodereview.appspot.com/9965045/diff/13002/scripts/slave/webpagereplay.py#newcode12> > scripts/slave/webpagereplay.**py:12: class ReplayLauncher: > change this to ReplayLauncher(object) > > https://chromiumcodereview.**appspot.com/9965045/<https://chromiumcodereview.... >
On 2012/04/06 19:13:00, slamm wrote: > Would you prefer something like > src/tools/python/google/webpagereplay_utils.py? That sounds good, it appears there's precedent for that. > Perhaps, the main() of that new file could start-up for adhoc testing. I > could even > make the main() launch Chromium with the right options. Right, making it easier for devs to reproduce the test environment will help when inevitably someone introduces a regression and needs a quick path to repeat locally. > If we left things as there currently stand, the commands for running WPR > from the command-line are not too bad: > https://sites.google.com/a/google.com/page-cyclers-wpr%3Chttps://sites.google...> > / > (see Quickstart) > > The real problem with public testing is that start.html, report.html, etc. > are all in the private tree. > The existing page cycler forked the files to the public tree. One thing I > could do there, is only put the "data.wpr" file > in the private tree. I could put the url list (test.js) and the other files > in the public tree. I am unsure if we would be > concern about revealing the url list. I agree. One possibility is to keep all of the private files in the internal checkout and to add a corresponding simpler version of a snapshot in the public tree. For example, a blank page that loads a blank png would be enough for someone to see how the web page replay test works, and that could be added to the public tree. We do this for some page cyclers already (they get stored in chrome/trunk/src/tools/page_cycler/). If you are able to rely on existing page cycler data for these tests, would it make sense to just point people at the page cycler data we already have? (Sample, for example.) Chase > -slamm > > > https://chromiumcodereview.**appspot.com/9965045/diff/**13002/scripts/slave/ > > > **webpagereplay.py<https://chromiumcodereview.appspot.com/9965045/diff/13002/scripts/slave/webpagereplay.py> > > File scripts/slave/webpagereplay.py (right): > > > > https://chromiumcodereview.**appspot.com/9965045/diff/** > > > 13002/scripts/slave/**webpagereplay.py#newcode12<https://chromiumcodereview.appspot.com/9965045/diff/13002/scripts/slave/webpagereplay.py#newcode12> > > scripts/slave/webpagereplay.**py:12: class ReplayLauncher: > > change this to ReplayLauncher(object) > > > > > https://chromiumcodereview.**appspot.com/9965045/%3Chttps://chromiumcoderevie...> > >
PTAL Issue 9956045 which has all the supporting pieces (except for the data in issue 4111018) is committed. Do the buildbots pick this up once its committed, or do they have a manual release process?
On 2012/04/18 11:41:13, slamm wrote: > PTAL The DEPS file is still a part of this change. Isn't the result of our discussion above that the code would be stored in chrome/trunk/src/ and so it would not need to be a part of DEPS? Just want to make sure we're on the same page. > Issue 9956045 which has all the supporting pieces (except for the data in issue > 4111018) is committed. > > Do the buildbots pick this up once its committed, or do they have a manual > release process? Right, each of the bots have an update_scripts step which will pull any changes into the bot's checkout for that build. So steps that run later on in that build will run with your changes.
On Wed, Apr 18, 2012 at 11:27 AM, <cmp@chromium.org> wrote: > On 2012/04/18 11:41:13, slamm wrote: > >> PTAL >> > > The DEPS file is still a part of this change. Isn't the result of our > discussion above that the code would be stored in chrome/trunk/src/ and so > it > would not need to be a part of DEPS? Just want to make sure we're on the > same > page. You are right. Do you have another third_party directory you would suggest? src/third_party? -slamm > Issue 9956045 which has all the supporting pieces (except for the data in >> > issue > >> 4111018) is committed. >> > > Do the buildbots pick this up once its committed, or do they have a manual >> release process? >> > > Right, each of the bots have an update_scripts step which will pull any > changes > into the bot's checkout for that build. So steps that run later on in that > build will run with your changes. > > https://chromiumcodereview.**appspot.com/9965045/<https://chromiumcodereview.... >
On 2012/04/18 19:15:35, slamm wrote: > You are right. Do you have another third_party directory you would > suggest? src/third_party? src/third_party is what I was thinking, yeah.
On 2012/04/18 19:59:36, cmp wrote: > On 2012/04/18 19:15:35, slamm wrote: > > You are right. Do you have another third_party directory you would > > suggest? src/third_party? > > src/third_party is what I was thinking, yeah. I updated this CL and made a new one for the chrome/src tree: https://chromiumcodereview.appspot.com/10020064 -slamm
https://chromiumcodereview.appspot.com/9965045/diff/28003/scripts/slave/runte... File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/9965045/diff/28003/scripts/slave/runte... scripts/slave/runtest.py:180: import google.webpagereplay_utils it seems like sys.path needs to be manipulated so that 'src/third_party/webpagereplay/.../' is correctly in it. wdyt?
sys.path already get edited. More inline... https://chromiumcodereview.appspot.com/9965045/diff/28003/scripts/slave/runte... File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/9965045/diff/28003/scripts/slave/runte... scripts/slave/runtest.py:180: import google.webpagereplay_utils On 2012/04/18 20:37:22, cmp wrote: > it seems like sys.path needs to be manipulated so that > 'src/third_party/webpagereplay/.../' is correctly in it. wdyt? Line 31 does that manipulation: sys.path.insert(0, os.path.abspath('src/tools/python')) It was already needed for start_http_server() which imports google.httpd_utils and google.platform_utils.
lgtm https://chromiumcodereview.appspot.com/9965045/diff/28003/scripts/slave/runte... File scripts/slave/runtest.py (right): https://chromiumcodereview.appspot.com/9965045/diff/28003/scripts/slave/runte... scripts/slave/runtest.py:180: import google.webpagereplay_utils Thanks, I didn't see that.
And now, the moment we have all been waiting for... WPR firing up from runtest.py. Would one of you commit this?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/slamm@google.com/9965045/28003
Presubmit check for 9965045-28003 failed and returned exit status 1.
Running presubmit commit checks ...
************* Module slave.runtest
F0401:180,2:start_wpr_server: Unable to import 'google.webpagereplay_utils'
** Presubmit ERRORS **
Fix pylint errors first.
Missing LGTM from an OWNER for files in these directories:
build
Presubmit checks took 121.0s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
Hey Steve, this import issue will need to be resolved first. Please take a look.
I uploaded a patch to disable that pylint check. (I copied the command from line 133 for the http server.) -slamm On Mon, May 14, 2012 at 12:28 PM, <cmp@chromium.org> wrote: > Hey Steve, this import issue will need to be resolved first. Please take a > look. > > https://chromiumcodereview.**appspot.com/9965045/<https://chromiumcodereview.... >
Sorry for chiming in so late, but I don't understand why the buildbot script needs to be so closely coupled to the test. I'd like the test itself to be responsible for starting WPR so that we can run locally with something like: $ ./out/Debug/performance_ui_tests --gtest_filter=PageCyclerWebPageReplayTest.2012Q2 With this approach, we'd have to start the buildbot script or use that separate python wrapper to run locally. Also with this approach we'd have to keep the 2012Q2 string in sync between the buildbot script and the test. Ideally we'd never have to touch the buildbot script when updating the test.
This seems like a good idea. Earlier, I had expected that WPR would need sudo. Now that we are working around that, it makes more sense to launch WPR as part of the test. This also eliminates the duplicate Chrome command-line options that we talked about before. Step to take now, - Remove webpagereplay_util - Change page_cycler_test.cc to launch WPR for each WPR-enabled test. BTW, four of the pages in 2012Q2 no longer finish loading on Linux and Mac: - http://espn.go.com/ - http://www.youtube.com/ - http://www.amazon.com/Kindle-Fire-Amazon-Tablet/dp/B0051VVOB2/ - http://www.msn.com/ I was planning to investigate that before turning on the test. Another issue that I have noticed is high variance among the page load times. Would someone like to help me trace one or two of the pages? -slamm On Tue, May 15, 2012 at 10:02 AM, <tonyg@chromium.org> wrote: > Sorry for chiming in so late, but I don't understand why the buildbot > script > needs to be so closely coupled to the test. > > I'd like the test itself to be responsible for starting WPR so that we can > run > locally with something like: > $ ./out/Debug/performance_ui_**tests > --gtest_filter=**PageCyclerWebPageReplayTest.**2012Q2 > > With this approach, we'd have to start the buildbot script or use that > separate > python wrapper to run locally. Also with this approach we'd have to keep > the > 2012Q2 string in sync between the buildbot script and the test. Ideally > we'd > never have to touch the buildbot script when updating the test. > > http://chromiumcodereview.**appspot.com/9965045/<http://chromiumcodereview.ap... >
+1 to making WPR be launched by page_cycler_test.cc directly. I agree that will make repro'ing issues much easier for devs.
Any pointers on how to launch the WPR process? -slamm On Tue, May 15, 2012 at 10:35 AM, <cmp@chromium.org> wrote: > +1 to making WPR be launched by page_cycler_test.cc directly. I agree > that will > make repro'ing issues much easier for devs. > > http://chromiumcodereview.**appspot.com/9965045/<http://chromiumcodereview.ap... >
I went into src/chrome/test/ and ran 'git gs http' to (hopefully) find an example of this today. I found that chrome/test/ui/ppapi_uitest.cc has a RunHTTPTestServer() method. That seems like a good way to manage some separate server. Hopefully there's not MTOWTDI syndrome in src/ for this. And Tony may have a better idea. WARNING! The ppapi tests are infamously flaky. It may be because of how this HTTP server start is implemented, though I assume it's not. Just keep that in mind as you use it and watch out for introducing the possibility of flakiness with managing the server. Ideally, whatever you do should be something that can be easily ported over to PyAuto at some point in the future (assuming that your WPR tests will end up in PyAuto, not sure what's planned).
On Tue, May 15, 2012 at 10:48 AM, <cmp@chromium.org> wrote: > I went into src/chrome/test/ and ran 'git gs http' to (hopefully) find an > example of this today. I found that chrome/test/ui/ppapi_uitest.cc has a > RunHTTPTestServer() method. That seems like a good way to manage some > separate > server. Hopefully there's not MTOWTDI syndrome in src/ for this. And Tony > may > have a better idea. > > WARNING! The ppapi tests are infamously flaky. It may be because of how > this > HTTP server start is implemented, though I assume it's not. Just keep that > in > mind as you use it and watch out for introducing the possibility of > flakiness > with managing the server. > > Ideally, whatever you do should be something that can be easily ported over > to > PyAuto at some point in the future (assuming that your WPR tests will end up > in > PyAuto, not sure what's planned). Could they just be added in pyauto now instead of the native test? Then the existing python harness can be used without extra work. > > http://chromiumcodereview.appspot.com/9965045/
> Another issue that I have noticed is high variance among the page load > times. Would someone like to help me trace one or two of the pages? Sure, which one would you like me to look at? And for now, what command are you using to run locally? |
