Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10456042:
Add Web Page Replay tests to main perf dashboard. (Closed)
|
Created:
8 years, 6 months ago by slamm_google Modified:
8 years, 6 months ago CC:
chromium-reviews, nsylvain+cc_chromium.org, cmp+cc_chromium.org Visibility:
Public. |
DescriptionAdd page cycler tests with network simulation to main perf dashboard.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140601
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #
Messages
Total messages: 15 (0 generated)
Does this look like the best way to get Web Page Replay tests on the main perf dashboard? I am unsure how to test this change. -slamm
http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... File scripts/master/factory/chromium_commands.py (right): http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... scripts/master/factory/chromium_commands.py:684: def AddPyAutoFunctionalTest(self, test_name, timeout=1200, Most of this diff is making the code a little easier to read. All that is needed for Web Page Replay is the new test_args parameter. I can make this change as a "minimal diff" if needed.
Looks great, just some small suggestions. To test, you can follow the steps here: http://dev.chromium.org/developers/testing/chromium-build-infrastructure/gett... Basically: $ cd build $ echo Bleh > site_config/.bot_password $ cd masters/master.chromium.perf $ make start $ cd ../../slave $ TESTING_MASTER_HOST=localhost TESTING_MASTER=ChromiumPerf TESTING_SLAVENAME=<<SOME_HOSTNAME_THAT_YOU_MODIFIED>> make restart Then hit http://localhost:8213/ http://codereview.chromium.org/10456042/diff/6004/masters/master.chromium.per... File masters/master.chromium.perf/master.cfg (right): http://codereview.chromium.org/10456042/diff/6004/masters/master.chromium.per... masters/master.chromium.perf/master.cfg:365: 'web_page_replay_2012Q2'], Since web page replay can be used to back tests other than those that measure page load time, what do you think about a different name? In my mind, this is something like "page_cycler_2012Q2-netsim". http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... File scripts/master/factory/chromium_commands.py (right): http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... scripts/master/factory/chromium_commands.py:289: def AddWebPageReplayTest(self, test_name, wpr_test, A selfish suggestion: I'm going to be adding some other pyauto perf test steps here. We could share the same method if you tweak the abstraction slightly and do: AddPyAutoPerfTest(self, step_name, pyauto_suite, pyauto_test, ...): pyauto_test = 'perf.%s.%s' % (pyauto_suite, pyauto_test) ... Then: if R('page_cycler_2012Q2-netsim'): f.AddPyAutoPerfTest('page_cycler_2012Q2-netsim', 'WebPageReplayPageCyclerTest', 'test2012Q2', fp)
Just a few comments. I also think this is the first time a pyauto-based test is being added to chromium.perf. We should make sure Chase is ok with it too ;-) http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... File scripts/master/factory/chromium_commands.py (right): http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... scripts/master/factory/chromium_commands.py:289: def AddWebPageReplayTest(self, test_name, wpr_test, On 2012/05/31 01:59:56, tonyg_google wrote: > A selfish suggestion: I'm going to be adding some other pyauto perf test steps > here. We could share the same method if you tweak the abstraction slightly and > do: > > AddPyAutoPerfTest(self, step_name, pyauto_suite, pyauto_test, ...): > pyauto_test = 'perf.%s.%s' % (pyauto_suite, pyauto_test) > ... > > Then: > > if R('page_cycler_2012Q2-netsim'): > f.AddPyAutoPerfTest('page_cycler_2012Q2-netsim', > 'WebPageReplayPageCyclerTest', 'test2012Q2', fp) I think we could just get rid of this function and call AddPyAutoFunctionalTest() directly, given the changes to that function below. http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... scripts/master/factory/chromium_commands.py:708: pyauto_script = J(src_base, 'src', 'chrome', 'test', 'functional', Cool! I really like how the "src_base" parameter is being handled now. http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chro... scripts/master/factory/chromium_commands.py:723: if suite: nit: I recommend adding a blank line right above this, to separate the platform checks above from the "suite" and "test_args" checks below.
On 2012/05/31 17:18:53, dennis_jeffrey wrote: > Just a few comments. I also think this is the first time a pyauto-based test is > being added to chromium.perf. We should make sure Chase is ok with it too ;-) Yeah, I'm fine with that. You (Dennis, or someone on that old thread from May 24) should still take a look at what I wrote re: letting chromium-dev know about switching any perf tests to pyauto. FWIW I don't think this CL crosses that threshold of needing to let chromium-dev know, but maybe someone could see it as such. This situation is getting sticky..
I am not having much luck testing this. First try: timed out on page_cycler_moz: http://sama.mtv.corp.google.com:9050/builders/Linux%20Perf%20%281%29/builds/0 Commented out all the tests except the Web Page Replay one and it gave an error about being unable to open the display: http://sama.mtv.corp.google.com:9050/builders/Linux%20Perf%20%281%29/builds/2... I do not see any errors that look like they are specific to my changes. Would someone be able to take over my CL after I make the suggested changes? -slamm On Wed, May 30, 2012 at 6:59 PM, <tonyg@google.com> wrote: > Looks great, just some small suggestions. > > To test, you can follow the steps here: > http://dev.chromium.org/**developers/testing/chromium-** > build-infrastructure/getting-**the-buildbot-source/** > configuring-your-buildbot<http://dev.chromium.org/developers/testing/chromium-build-infrastructure/getting-the-buildbot-source/configuring-your-buildbot> > > Basically: > $ cd build > $ echo Bleh > site_config/.bot_password > $ cd masters/master.chromium.perf > $ make start > $ cd ../../slave > $ TESTING_MASTER_HOST=localhost TESTING_MASTER=ChromiumPerf > TESTING_SLAVENAME=<<SOME_**HOSTNAME_THAT_YOU_MODIFIED>> make restart > > Then hit http://localhost:8213/ > > > http://codereview.chromium.**org/10456042/diff/6004/** > masters/master.chromium.perf/**master.cfg<http://codereview.chromium.org/10456042/diff/6004/masters/master.chromium.perf/master.cfg> > File masters/master.chromium.perf/**master.cfg (right): > > http://codereview.chromium.**org/10456042/diff/6004/** > masters/master.chromium.perf/**master.cfg#newcode365<http://codereview.chromium.org/10456042/diff/6004/masters/master.chromium.perf/master.cfg#newcode365> > masters/master.chromium.perf/**master.cfg:365: 'web_page_replay_2012Q2'], > Since web page replay can be used to back tests other than those that > measure page load time, what do you think about a different name? In my > mind, this is something like "page_cycler_2012Q2-netsim". > > > http://codereview.chromium.**org/10456042/diff/6004/** > scripts/master/factory/**chromium_commands.py<http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chromium_commands.py> > File scripts/master/factory/**chromium_commands.py (right): > > http://codereview.chromium.**org/10456042/diff/6004/** > scripts/master/factory/**chromium_commands.py#**newcode289<http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chromium_commands.py#newcode289> > scripts/master/factory/**chromium_commands.py:289: def > AddWebPageReplayTest(self, test_name, wpr_test, > A selfish suggestion: I'm going to be adding some other pyauto perf test > steps here. We could share the same method if you tweak the abstraction > slightly and do: > > AddPyAutoPerfTest(self, step_name, pyauto_suite, pyauto_test, ...): > pyauto_test = 'perf.%s.%s' % (pyauto_suite, pyauto_test) > ... > > Then: > > if R('page_cycler_2012Q2-netsim')**: > f.AddPyAutoPerfTest('page_**cycler_2012Q2-netsim', > 'WebPageReplayPageCyclerTest', 'test2012Q2', fp) > > http://codereview.chromium.**org/10456042/<http://codereview.chromium.org/104... >
On Thu, May 31, 2012 at 5:07 PM, Stephen Lamm <slamm@google.com> wrote: > I am not having much luck testing this. > > First try: timed out on page_cycler_moz: > > http://sama.mtv.corp.google.com:9050/builders/Linux%20Perf%20%281%29/builds/0 > > It looks like there was a problem finding the page cycler data files: Missing test directory /usr/local/google/home/slamm/chrome/tools/build/slave/chromium-rel-linux-1/build/src/data/page_cycler/moz Maybe they weren't synced during the update step for some reason? > Commented out all the tests except the Web Page Replay one and it gave an > error about being unable to open the display: > > http://sama.mtv.corp.google.com:9050/builders/Linux%20Perf%20%281%29/builds/2... > > > Does it need to run through xvfb? Was the slave running the test a VM or an actual machine? > I do not see any errors that look like they are specific to my changes. > Would someone be able to take over my CL after I make the suggested > changes? > > -slamm > > On Wed, May 30, 2012 at 6:59 PM, <tonyg@google.com> wrote: > >> Looks great, just some small suggestions. >> >> To test, you can follow the steps here: >> http://dev.chromium.org/**developers/testing/chromium-** >> build-infrastructure/getting-**the-buildbot-source/** >> configuring-your-buildbot<http://dev.chromium.org/developers/testing/chromium-build-infrastructure/getting-the-buildbot-source/configuring-your-buildbot> >> >> Basically: >> $ cd build >> $ echo Bleh > site_config/.bot_password >> $ cd masters/master.chromium.perf >> $ make start >> $ cd ../../slave >> $ TESTING_MASTER_HOST=localhost TESTING_MASTER=ChromiumPerf >> TESTING_SLAVENAME=<<SOME_**HOSTNAME_THAT_YOU_MODIFIED>> make restart >> >> Then hit http://localhost:8213/ >> >> >> http://codereview.chromium.**org/10456042/diff/6004/** >> masters/master.chromium.perf/**master.cfg<http://codereview.chromium.org/10456042/diff/6004/masters/master.chromium.perf/master.cfg> >> File masters/master.chromium.perf/**master.cfg (right): >> >> http://codereview.chromium.**org/10456042/diff/6004/** >> masters/master.chromium.perf/**master.cfg#newcode365<http://codereview.chromium.org/10456042/diff/6004/masters/master.chromium.perf/master.cfg#newcode365> >> masters/master.chromium.perf/**master.cfg:365: 'web_page_replay_2012Q2'], >> Since web page replay can be used to back tests other than those that >> measure page load time, what do you think about a different name? In my >> mind, this is something like "page_cycler_2012Q2-netsim". >> >> >> http://codereview.chromium.**org/10456042/diff/6004/** >> scripts/master/factory/**chromium_commands.py<http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chromium_commands.py> >> File scripts/master/factory/**chromium_commands.py (right): >> >> http://codereview.chromium.**org/10456042/diff/6004/** >> scripts/master/factory/**chromium_commands.py#**newcode289<http://codereview.chromium.org/10456042/diff/6004/scripts/master/factory/chromium_commands.py#newcode289> >> scripts/master/factory/**chromium_commands.py:289: def >> AddWebPageReplayTest(self, test_name, wpr_test, >> A selfish suggestion: I'm going to be adding some other pyauto perf test >> steps here. We could share the same method if you tweak the abstraction >> slightly and do: >> >> AddPyAutoPerfTest(self, step_name, pyauto_suite, pyauto_test, ...): >> pyauto_test = 'perf.%s.%s' % (pyauto_suite, pyauto_test) >> ... >> >> Then: >> >> if R('page_cycler_2012Q2-netsim')**: >> f.AddPyAutoPerfTest('page_**cycler_2012Q2-netsim', >> 'WebPageReplayPageCyclerTest', 'test2012Q2', fp) >> >> http://codereview.chromium.**org/10456042/<http://codereview.chromium.org/104... >> > >
On Thu, May 31, 2012 at 2:55 PM, <cmp@chromium.org> wrote: > On 2012/05/31 17:18:53, dennis_jeffrey wrote: > >> Just a few comments. I also think this is the first time a pyauto-based >> test >> > is > >> being added to chromium.perf. We should make sure Chase is ok with it >> too ;-) >> > > Yeah, I'm fine with that. You (Dennis, or someone on that old thread from > May > 24) should still take a look at what I wrote re: letting chromium-dev know > about > switching any perf tests to pyauto. FWIW I don't think this CL crosses > that > threshold of needing to let chromium-dev know, but maybe someone could see > it as > such. This situation is getting sticky.. > > Yes, I agree that chromium-dev needs to be notified regarding switching any perf tests to pyauto. In this case, I think it's a new perf test that is not replacing anything, so also agree that this CL doesn't cross the threshold of notifying chromium-dev. I (or maybe Tony?) can take care of doing this when needed. It would be good to get nirnimesh@'s input too, when making the case for it. The other point brought up in that thread is whether the pyauto-based versions of the tests can still collect the full set of info made available through the UIPerfTest harness. I still need to look into that. I assume and am hoping that we can make this info available to pyauto through automation hooks, if it's not already available. > http://codereview.chromium.**org/10456042/<http://codereview.chromium.org/104... >
Ready for another look. The new test passes on my machine. This change now depends on another: https://chromiumcodereview.appspot.com/10500010/ https://chromiumcodereview.appspot.com/10456042/diff/6004/masters/master.chro... File masters/master.chromium.perf/master.cfg (right): https://chromiumcodereview.appspot.com/10456042/diff/6004/masters/master.chro... masters/master.chromium.perf/master.cfg:365: 'web_page_replay_2012Q2'], On 2012/05/31 01:59:56, tonyg_google wrote: > Since web page replay can be used to back tests other than those that measure > page load time, what do you think about a different name? In my mind, this is > something like "page_cycler_2012Q2-netsim". Thanks for the name suggestion. That is much better. I also added a CL (https://chromiumcodereview.appspot.com/10500010/) to change perf.WebPageReplayPageCyclerTest to perf.PageCyclerNetSimTest. Done. https://chromiumcodereview.appspot.com/10456042/diff/6004/scripts/master/fact... File scripts/master/factory/chromium_commands.py (right): https://chromiumcodereview.appspot.com/10456042/diff/6004/scripts/master/fact... scripts/master/factory/chromium_commands.py:289: def AddWebPageReplayTest(self, test_name, wpr_test, On 2012/05/31 17:18:53, dennis_jeffrey wrote: > On 2012/05/31 01:59:56, tonyg_google wrote: > > A selfish suggestion: I'm going to be adding some other pyauto perf test steps > > here. We could share the same method if you tweak the abstraction slightly and > > do: > > > > AddPyAutoPerfTest(self, step_name, pyauto_suite, pyauto_test, ...): > > pyauto_test = 'perf.%s.%s' % (pyauto_suite, pyauto_test) > > ... > > > > Then: > > > > if R('page_cycler_2012Q2-netsim'): > > f.AddPyAutoPerfTest('page_cycler_2012Q2-netsim', > > 'WebPageReplayPageCyclerTest', 'test2012Q2', fp) > > I think we could just get rid of this function and call > AddPyAutoFunctionalTest() directly, given the changes to that function below. Done. https://chromiumcodereview.appspot.com/10456042/diff/6004/scripts/master/fact... scripts/master/factory/chromium_commands.py:723: if suite: On 2012/05/31 17:18:53, dennis_jeffrey wrote: > nit: I recommend adding a blank line right above this, to separate the platform > checks above from the "suite" and "test_args" checks below. Done.
lgtm
LGTM
Chase, does this look good to you too? -slamm
lgtm, yep
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/slamm@google.com/10456042/17002
Change committed as 140601 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
