Adding a test for measuring memory usage.

Telemetry multi page testing FW: Add a proof-of-concept memory test.

We currently measure memory usage in page cycler tests, but we'd
need more realistic and longer running tests to get better
memory statistics.

The test works against Web Page Replay data, and uses Telemetry to
execute actions on pages.

Actual tests still need to be added.

BUG=158323
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167908

[marja, 2012-10-30 15:14:25]

tonyg, could you have a look at this CL?

This requires http://codereview.chromium.org/11340037/ to work.

And this steals the Interaction class from nduca's CL,
http://codereview.chromium.org/11114020/ , but does *not* refactor the scrolling
test to use it.

[marja, 2012-11-01 01:49:24]

tonyg: ping, can you have a look at this CL?

[tonyg, 2012-11-02 18:57:34]

A few suggestions inline. I'd really like Nat to review too.

http://codereview.chromium.org/11273081/diff/2002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/interaction.py (right):

http://codereview.chromium.org/11273081/diff/2002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/interaction.py:26:
_interaction_classes = {}
nit: line break after this

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
File
tools/chrome_remote_control/chrome_remote_control/test_helper_interaction.py
(right):

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/test_helper_interaction.py:20:
'test_helpers.js')
indentation is off here.

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/test_helpers.js (right):

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/test_helpers.js:14: var
found_element = findElementWithText(element.childNodes[i], text);
s/found_element/foundElement/

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/test_helpers.js:22: function
ClickLinkWithText(text) {
Use a leading lower case letter here and for readyStateComplete()

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
File tools/perf/page_sets/top_25_usage.json (right):

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:1: {
What do you have planned here? Can we add the interactions to the existing
top_25.json page set rather than creating a new one?

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:16: "completion":
"window.__ReadyStateComplete()"
How many interactions do you have planned? If it's only a handful, I really like
this factoring better:
"interactions": [
  {
    "type": "ClickLink",
    "link_text": "Women for Obama"
  }
]

Then click_link_interaction.py is responsible for running just the JS it needs
and waiting for ready state completion.

http://codereview.chromium.org/11273081/diff/7001/tools/perf/perf_tools/memor...
File tools/perf/perf_tools/memory_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/7001/tools/perf/perf_tools/memor...
tools/perf/perf_tools/memory_benchmark.py:27: results.Add(name,
histogram['units'], data, 'histogram')
Since the 4th parameter is optional, please name it here:
data_type='histogram'

[nduca, 2012-11-02 20:38:46]

Can we split this out into two cls,  an interaction patch and the memory usage
patch? Or am I being too cranketty? Totally call me on it if I am!

[marja, 2012-11-05 14:43:25]

Thanks for comments!

I split out the interaction part of this CL, and it is here:

http://codereview.chromium.org/11369075/

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
File
tools/chrome_remote_control/chrome_remote_control/test_helper_interaction.py
(right):

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/test_helper_interaction.py:20:
'test_helpers.js')
On 2012/11/02 18:57:34, tonyg wrote:
> indentation is off here.

Done.

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/test_helpers.js (right):

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/test_helpers.js:14: var
found_element = findElementWithText(element.childNodes[i], text);
On 2012/11/02 18:57:34, tonyg wrote:
> s/found_element/foundElement/

Done.

http://codereview.chromium.org/11273081/diff/7001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/test_helpers.js:22: function
ClickLinkWithText(text) {
On 2012/11/02 18:57:34, tonyg wrote:
> Use a leading lower case letter here and for readyStateComplete()

Done.

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
File tools/perf/page_sets/top_25_usage.json (right):

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:1: {
On 2012/11/02 18:57:34, tonyg wrote:
> What do you have planned here? Can we add the interactions to the existing
> top_25.json page set rather than creating a new one?

If I do that, I also need to recreate the .wpr file, and then the scrolling
benchmark results before and after the .wpr page recreation won't be comparable.
I added a different file to keep the scrolling benchmark unaffected by my
changes. And this would happen every time I add more tests to this file.

-> I can merge this with the top_25.json if you say it's ok to affect the
scrolling benchmark.

I suggest I add top_25_usage.json (and leave top_25.json and the scrolling
benchmark intact), and when we're done with adding the memory tests, I migrate
the scroll benchmark to use it too. Then we'll have only one discontinuity point
for the scrolling benchmark. Wdyt?

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:16: "completion":
"window.__ReadyStateComplete()"
On 2012/11/02 18:57:34, tonyg wrote:
> How many interactions do you have planned? If it's only a handful, I really
like
> this factoring better:
> "interactions": [
>   {
>     "type": "ClickLink",
>     "link_text": "Women for Obama"
>   }
> ]
> 
> Then click_link_interaction.py is responsible for running just the JS it needs
> and waiting for ready state completion.

Done. In the latest version, I'm just passing the json to the javascript side,
and there deciding which function to call, and reading the needed params.

http://codereview.chromium.org/11273081/diff/7001/tools/perf/perf_tools/memor...
File tools/perf/perf_tools/memory_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/7001/tools/perf/perf_tools/memor...
tools/perf/perf_tools/memory_benchmark.py:27: results.Add(name,
histogram['units'], data, 'histogram')
On 2012/11/02 18:57:34, tonyg wrote:
> Since the 4th parameter is optional, please name it here:
> data_type='histogram'

Done.

[marja, 2012-11-05 15:14:48]

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
File tools/perf/page_sets/top_25_usage.json (right):

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:16: "completion":
"window.__ReadyStateComplete()"
On 2012/11/05 14:43:25, marja wrote:
> On 2012/11/02 18:57:34, tonyg wrote:
> > How many interactions do you have planned? If it's only a handful, I really
> like
> > this factoring better:
> > "interactions": [
> >   {
> >     "type": "ClickLink",
> >     "link_text": "Women for Obama"
> >   }
> > ]
> > 
> > Then click_link_interaction.py is responsible for running just the JS it
needs
> > and waiting for ready state completion.
> 
> Done. In the latest version, I'm just passing the json to the javascript side,
> and there deciding which function to call, and reading the needed params.

Oops, I forgot to add that I don't have a clear vision about how many
interactions we'll have. At least link clicking and scrolling.

I tried to do this without having to dispatch "ClickLink" to "get click_link.js
and execute a function on it", so now all interactions would be in the same
file... but this is probably not the right thing.

I'll try to figure out something more robust.

[marja, 2012-11-05 15:26:44]

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
File tools/perf/page_sets/top_25_usage.json (right):

http://codereview.chromium.org/11273081/diff/7001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:16: "completion":
"window.__ReadyStateComplete()"
On 2012/11/05 15:14:48, marja wrote:
> On 2012/11/05 14:43:25, marja wrote:
> > On 2012/11/02 18:57:34, tonyg wrote:
> > > How many interactions do you have planned? If it's only a handful, I
really
> > like
> > > this factoring better:
> > > "interactions": [
> > >   {
> > >     "type": "ClickLink",
> > >     "link_text": "Women for Obama"
> > >   }
> > > ]
> > > 
> > > Then click_link_interaction.py is responsible for running just the JS it
> needs
> > > and waiting for ready state completion.
> > 
> > Done. In the latest version, I'm just passing the json to the javascript
side,
> > and there deciding which function to call, and reading the needed params.
> 
> Oops, I forgot to add that I don't have a clear vision about how many
> interactions we'll have. At least link clicking and scrolling.
> 
> I tried to do this without having to dispatch "ClickLink" to "get
click_link.js
> and execute a function on it", so now all interactions would be in the same
> file... but this is probably not the right thing.
> 
> I'll try to figure out something more robust.

I updated the interaction CL. Now I dispatch "clickLink" to "load
interaction_clickLink.js and call clickLink(json) on it". This causes minor file
naming pain, but if we can take it, then the system is more robust (no need to
add dispatching rules to the python side).

[nduca, 2012-11-06 05:10:02]

http://codereview.chromium.org/11273081/diff/16001/tools/perf/perf_tools/memo...
File tools/perf/perf_tools/memory_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/16001/tools/perf/perf_tools/memo...
tools/perf/perf_tools/memory_benchmark.py:32: def DoNextInteraction(self, page,
tab):
I'd like to see interactions be part of the multi_page_runner rather than part
of the benchmark.

[nduca, 2012-11-08 22:05:45]

https://codereview.chromium.org/11273081/diff/16001/tools/perf/perf_tools/mem...
File tools/perf/perf_tools/memory_benchmark.py (right):

https://codereview.chromium.org/11273081/diff/16001/tools/perf/perf_tools/mem...
tools/perf/perf_tools/memory_benchmark.py:30:
options.extra_browser_args.append('--dom-automation')
is this flag considered to be long-term supported? Does it work in clank? Is
this something slated to be deleted once we purge our architecture of pyauto/ui
tests?

[marja, 2012-11-09 14:17:35]

Hi again nduca@,

I said I'd to the scroll refactoring next, but I happened to have the memory
test handy since I was testing the interactions against it. Can you have a look
at the latest patch set?

I also did other refactorings:

The interactions need to customize the browser options, too. But only those
interactions, that the benchmark will actually execute (e.g., if the
stress_memory won't execute a scroll, the scroll doesn't need to add the browser
options).

I thought I'd need a separate interaction runner object, but now it seems it
could work quite elegantly without one. The design I have in mind is kind of
fluctuating all the time, and I'm discovering this testing framework as I go...
Sorry if this seems like random hacking. It probably is random hacking ;)

https://codereview.chromium.org/11273081/diff/16001/tools/perf/perf_tools/mem...
File tools/perf/perf_tools/memory_benchmark.py (right):

https://codereview.chromium.org/11273081/diff/16001/tools/perf/perf_tools/mem...
tools/perf/perf_tools/memory_benchmark.py:30:
options.extra_browser_args.append('--dom-automation')
On 2012/11/08 22:05:45, nduca wrote:
> is this flag considered to be long-term supported? Does it work in clank? Is
> this something slated to be deleted once we purge our architecture of
pyauto/ui
> tests?

The --dom-automation command line switch is ultimately processed by
RenderViewImpl, so it should be supported by clank as well. It's not necessarily
going to exist forever, but we need to somehow keep the histogram fetching
working (for the normal page cyclers which already produce memory histograms),
and afaics, the approach will be the same in the future (i.e., pass chrome a
command line switch which makes it export a JavaScript objects through which the
histograms can be retrieved).

https://codereview.chromium.org/11273081/diff/16001/tools/perf/perf_tools/mem...
tools/perf/perf_tools/memory_benchmark.py:32: def DoNextInteraction(self, page,
tab):
On 2012/11/06 05:10:02, nduca wrote:
> I'd like to see interactions be part of the multi_page_runner rather than part
> of the benchmark.

Now that we have compound interactions, running the interactions is simpler. Can
you have a look at the latest version?

If the benchmark is the one that executes the interaction, it can know which one
of them to execute, i.e., memory_benchmark.py hard-codes that the interaction to
execute is "stress_memory". If I move the interaction execution to some other
place, I'd need to retrieve the "what interaction to execute" from the test, and
pass it around...

[nduca, 2012-11-09 19:10:08]

I think you have a rebase issue... you've got stuff in this patch that doesn't
look like you intended...

[nduca, 2012-11-09 19:39:06]

Oops, I misread the patch. Now I get it. But, I think you need to move the
page_set<->interaction glue up one or two levels...

https://codereview.chromium.org/11273081/diff/20001/tools/perf/perf_tools/mem...
File tools/perf/perf_tools/memory_benchmark.py (right):

https://codereview.chromium.org/11273081/diff/20001/tools/perf/perf_tools/mem...
tools/perf/perf_tools/memory_benchmark.py:24: def CustomizeBrowserOptions(self,
pages, options):
I'd rather you not make this change. If you do the interactions up on
multi_page_benchmark or page_runner you dont have to modify customize
interactions.

https://codereview.chromium.org/11273081/diff/20001/tools/perf/perf_tools/mem...
tools/perf/perf_tools/memory_benchmark.py:34: if hasattr(page, 'stress_memory'):
This stuff is why you want the page_runner to understand interactions. We'd load
all these other pages only to discover that we didn't want them. :) That'd be
super bad for bot cycle time

[marja, 2012-11-12 12:23:31]

Hi,

Thanks for comments again! Here is a new version..

I also noticed a problem: now that the same page set is supposed to support many
benchmarks (which execute different actions on the page), we need to be able to
record such a page set, i.e., we need to be able to run multiple benchmarks so
that the recorded data goes to the same .wpr.
http://codereview.chromium.org/11364201 enables running multiple benchmarks at
the same time.

https://codereview.chromium.org/11273081/diff/20001/tools/perf/perf_tools/mem...
File tools/perf/perf_tools/memory_benchmark.py (right):

https://codereview.chromium.org/11273081/diff/20001/tools/perf/perf_tools/mem...
tools/perf/perf_tools/memory_benchmark.py:24: def CustomizeBrowserOptions(self,
pages, options):
On 2012/11/09 19:39:06, nduca wrote:
> I'd rather you not make this change. If you do the interactions up on
> multi_page_benchmark or page_runner you dont have to modify customize
> interactions.

Ok, I moved the interactions to MultiPageBenchmark. But still the test needs to
customize the browser options based on the interactions it will run, so, it
needs the page set to customize the browser options. So I added a
CustomizeBrowserOptionsForPageSet() there. The subclasses don't need to override
it.

https://codereview.chromium.org/11273081/diff/20001/tools/perf/perf_tools/mem...
tools/perf/perf_tools/memory_benchmark.py:34: if hasattr(page, 'stress_memory'):
On 2012/11/09 19:39:06, nduca wrote:
> This stuff is why you want the page_runner to understand interactions. We'd
load
> all these other pages only to discover that we didn't want them. :) That'd be
> super bad for bot cycle time

K, I added CanRunForPage to PageTest, and the PageRunner checks it, and doesn't
load pages which the test can't run. (Just having no interactions for the test
doesn't mean that a test can't run, so I needed a separate way to detect
that...)

[nduca, 2012-11-12 20:58:12]

I'd really like to see this moved up one level, to page_runner.


Think of it like this:

PageTest --- a test that is run over a page. Either runs or throws an exception.
PageRunner --- runs a PageTest over PageSet


MultipageBenchmark is supposed to be just a subclass of PageTest in which the
PageTEst produces a number as part of its results, instead of just "passed."

Interactions are not part of a benchmark per se. For example, "click such and
such a link and verify that memory didn't increase" is a test not a benchmark,
but still wants to use an interaction.

Hopefully this context helps. I'd make the PageTest constructor accept an
interactions_to_run list. Then, have the page runner run those interactions on
all the pages in its page set that support that interaction. Roughly what you've
done, just higher up in the stack.

http://codereview.chromium.org/11273081/diff/21009/tools/perf/perf_tools/memo...
File tools/perf/perf_tools/memory_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/21009/tools/perf/perf_tools/memo...
tools/perf/perf_tools/memory_benchmark.py:28: def GetInteractionName(self):
why not an init-time constructor?

if it was init time, then you coudl throw an exception at that point if the
interaction class didn't exist

http://codereview.chromium.org/11273081/diff/21009/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/21009/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark.py:153: def
CustomizeBrowserOptionsForPageSet(self, pages, options):
I think you should move this all the way up to page_runner...

[marja, 2012-11-12 21:35:25]

Thanks for comments; this one I didn't quite understand...

http://codereview.chromium.org/11273081/diff/21009/tools/perf/perf_tools/memo...
File tools/perf/perf_tools/memory_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/21009/tools/perf/perf_tools/memo...
tools/perf/perf_tools/memory_benchmark.py:28: def GetInteractionName(self):
On 2012/11/12 20:58:12, nduca wrote:
> why not an init-time constructor?
> 
> if it was init time, then you coudl throw an exception at that point if the
> interaction class didn't exist

Hmm? Init-time constructor of which class? Do you want to keep the information
that the interaction to run is "stress_memory" in this file, or do you want me
to move it to somewhere else?

[nduca, 2012-11-12 22:55:43]

maybe PageTest.__init__(interaction_to_run)?

[marja, 2012-11-13 09:47:40]

Thanks for comments again!

http://codereview.chromium.org/11273081/diff/21009/tools/perf/perf_tools/memo...
File tools/perf/perf_tools/memory_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/21009/tools/perf/perf_tools/memo...
tools/perf/perf_tools/memory_benchmark.py:28: def GetInteractionName(self):
On 2012/11/12 21:35:26, marja wrote:
> On 2012/11/12 20:58:12, nduca wrote:
> > why not an init-time constructor?
> > 
> > if it was init time, then you coudl throw an exception at that point if the
> > interaction class didn't exist
> 
> Hmm? Init-time constructor of which class? Do you want to keep the information
> that the interaction to run is "stress_memory" in this file, or do you want me
> to move it to somewhere else?

Ok, made it a ctor parameter for PageTest. But we can't thrown an exception at
that time:

We're passing 'stress_memory' to PageTest, and in the json for a page, we either
have

'stress_memory': {'action': 'foo'}

or not, and it's completely ok to not have stress_memory for any page; also in
the ctor of PageTest we don't know the specific pages yet, so we can't throw an
exception if 'foo' is not an action.

http://codereview.chromium.org/11273081/diff/21009/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/21009/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark.py:153: def
CustomizeBrowserOptionsForPageSet(self, pages, options):
On 2012/11/12 20:58:12, nduca wrote:
> I think you should move this all the way up to page_runner...

Ok. And I also need to create PageRunner earlier (because the browser is now
created before it, and after creating the browser it's too late to change the
options). And I needed to give the PageRunner the test to execute in the ctor,
because the browser options depend on both the page set and the test to execute.
The resulting code is a bit awkward, but the previous version was too :)

[nduca, 2012-11-14 06:48:31]

This looks really promising. One more round I hope and i'll be down to nitting
you. :)

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:90:
_Discover(os.path.dirname(__file__), page_interaction.PageInteraction)
I think you can avoid having to restructure page runner and init stuff by making
discover more automatic.

To break the a circular references problem, I'm pretty sure it can be fixed by
moving the database of supported interactions to something separate from
interaction.py 

eg interaction.py

and all_interactions.py

and all_interactions.py just runs discover.

page_test constructor asks all_interactions if the interaction is supported and
throws if not.

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:93:
runner.CustomizeBrowserOptions(options)
you're leaking runner object with these changes.... the with x as y: construct
was causing Close() on y whenever it finished. Thats key.

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark_unittest.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_unittest.py:48:
_interaction_called = False
I gather you're trying to get around python2's local variable binding
strangeness.  The current pattern we're using is to make a local variable on the
specific test:

  interaction_called = [False]

then in the mock interaction, just say
   interaction_called[0] = True

that'll work around python's strangeness.

While you're at it, can you remove same instance of this you added to
inspector_page_unittest.py?

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:105: if not
self._test.CanRunForPage(page):
How about just checking if the self._test.interaction_to_run in page directly
here? I dont think the test needs to actually have a hook to this decision...

[nduca, 2012-11-14 08:36:53]

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_test.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:71: self.DidRunInteraction(page, tab)
should we return here?

[nduca, 2012-11-14 09:50:18]

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:79: interaction =
test.GetInteraction(page)
test.interaction <-- use a @property decorator

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:111: return
lets call results.AddSkippedPage(page, "Test said it couldn't run") call here

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_test.py (left):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:29: def AddOptions(self, parser):
Lets rename this to be AddCommandLineOptiions

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:38: """Override to add test-specific
options to the BrowserOptions object"""
to the command line options parser

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_test.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:30: self._interaction_name_to_run =
interaction_name_to_run
you can verify that it exists, now, right?

[nduca, 2012-11-14 10:02:40]

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/all_page_interactions.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/all_page_interactions.py:9:
discover.Discover(os.path.dirname(__file__), page_interaction.PageInteraction)
doesn't discover return all the class names discvoered? If so, we can remove the
manual registration in each of the PageInteraction subclasses, and move the list
of interactions right to here.

E.g.

__interaction_classes = discover()

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark.py:150: super(MultiPageBenchmark,
self).__init__('_RunTest', interaction_name)
this needs to be optional, right?

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:56: runner =
page_runner.PageRunner(ps)
dont need this line now, right?

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:65: results =
multi_page_benchmark.CsvBenchmarkResults(csv.writer(sys.stdout))
is this line a conflict or something?

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:66: runner.Run(options,
possible_browser, benchmark, results)
indentation problem?

[marja, 2012-11-14 13:16:56]

Thanks again for comments!

I noticed that the discovery is quite ugly. It basically imports every file in
that directory, even the test files. And I ended up problems in tests again,
when a test tries to do the discovery (with an import inside a func), and that
ends up importing multi_page_benchmark.py and that ends up importing
all_page_interactions.py again. I added a "file name suffix" thing to restrict
which files it imports.. maybe it's nicer to not import the test files when
we're running the production code?

About the stress_memory_interaction change: So, now the page set says:
        {
            "name": "Facebook",
            "url": "http://www.facebook.com/barackobama",
            "stress_memory": {
                "action": "compound_interaction",
                "actions": [
                    {
                        "action": "click_to_navigate_interaction",
                        "selector":
"a[href=\"http://www.facebook.com/WomenforObama\"]"
                    },
		    ...
                ]
            }
        },

And you want to change it to:

            "stress_memory": {
                "action": "stress_memory",
                "actions": [
                    {
                        "action": "click_to_navigate_interaction",
                        "selector":
"a[href=\"http://www.facebook.com/WomenforObama\"]"
                    },
		    ...
                ]
            }

or

            "stress_memory": {
                "actions": [
                    {
                        "action": "click_to_navigate_interaction",
                        "selector":
"a[href=\"http://www.facebook.com/WomenforObama\"]"
                    },
		    ...
                ]
            }

?

I'd like the value of the "stress_memory" to follow the format for actions, so
that can be parsed as an action, and we don't need extra code for parsing it,
thus the "action" line.

I still don't see the point, we can't do any more checks without knowing the
page. It's not an error to write

            "stress_memory_foobar_doesn't_exist": {
                "actions": [
                    {
                        "action": "click_to_navigate_interaction",
                        "selector":
"a[href=\"http://www.facebook.com/WomenforObama\"]"
                    },
		    ...
                ]
            }

because then the memory test would just skip the page. And without the page, we
can't check any errors which occur inside the value of "stress_memory".

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:90:
_Discover(os.path.dirname(__file__), page_interaction.PageInteraction)
On 2012/11/14 06:48:31, nduca wrote:
> I think you can avoid having to restructure page runner and init stuff by
making
> discover more automatic.
> 
> To break the a circular references problem, I'm pretty sure it can be fixed by
> moving the database of supported interactions to something separate from
> interaction.py 
> 
> eg interaction.py
> 
> and all_interactions.py
> 
> and all_interactions.py just runs discover.
> 
> page_test constructor asks all_interactions if the interaction is supported
and
> throws if not.

-> Moved _Discover to discover.py, added automatic discover to
all_page_interactions.py.

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:93:
runner.CustomizeBrowserOptions(options)
On 2012/11/14 06:48:31, nduca wrote:
> you're leaking runner object with these changes.... the with x as y: construct
> was causing Close() on y whenever it finished. Thats key.

Done.

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark_unittest.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_unittest.py:48:
_interaction_called = False
On 2012/11/14 06:48:31, nduca wrote:
> I gather you're trying to get around python2's local variable binding
> strangeness.  The current pattern we're using is to make a local variable on
the
> specific test:
> 
>   interaction_called = [False]
> 
> then in the mock interaction, just say
>    interaction_called[0] = True
> 
> that'll work around python's strangeness.
> 
> While you're at it, can you remove same instance of this you added to
> inspector_page_unittest.py?

Done (both).

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:105: if not
self._test.CanRunForPage(page):
On 2012/11/14 06:48:31, nduca wrote:
> How about just checking if the self._test.interaction_to_run in page directly
> here? I dont think the test needs to actually have a hook to this decision...

Discussion -> that won't be enough, we need some way to not load pages which
don't run an interaction but do a test.

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_test.py (right):

http://codereview.chromium.org/11273081/diff/17013/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:71: self.DidRunInteraction(page, tab)
On 2012/11/14 08:36:53, nduca wrote:
> should we return here?

This was a sidetrack; the results would stay uninitialized if I do that.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/all_page_interactions.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/all_page_interactions.py:9:
discover.Discover(os.path.dirname(__file__), page_interaction.PageInteraction)
On 2012/11/14 10:02:40, nduca wrote:
> doesn't discover return all the class names discvoered? If so, we can remove
the
> manual registration in each of the PageInteraction subclasses, and move the
list
> of interactions right to here.
> 
> E.g.
> 
> __interaction_classes = discover()

Done.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark.py:150: super(MultiPageBenchmark,
self).__init__('_RunTest', interaction_name)
On 2012/11/14 10:02:40, nduca wrote:
> this needs to be optional, right?

?

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:56: runner =
page_runner.PageRunner(ps)
On 2012/11/14 10:02:40, nduca wrote:
> dont need this line now, right?

Done.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:65: results =
multi_page_benchmark.CsvBenchmarkResults(csv.writer(sys.stdout))
On 2012/11/14 10:02:40, nduca wrote:
> is this line a conflict or something?

Just an accidental reordering -> fixed.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:66: runner.Run(options,
possible_browser, benchmark, results)
On 2012/11/14 10:02:40, nduca wrote:
> indentation problem?

Done.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:79: interaction =
test.GetInteraction(page)
On 2012/11/14 09:50:18, nduca wrote:
> test.interaction <-- use a @property decorator

Done.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:111: return
On 2012/11/14 09:50:18, nduca wrote:
> lets call results.AddSkippedPage(page, "Test said it couldn't run") call here

Done.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_test.py (left):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:29: def AddOptions(self, parser):
On 2012/11/14 09:50:18, nduca wrote:
> Lets rename this to be AddCommandLineOptiions

Done, but this is unrelated to any of the changes of this CL... I'd like to keep
the CLs small and not do too much of this kind of changes.

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:38: """Override to add test-specific
options to the BrowserOptions object"""
On 2012/11/14 09:50:18, nduca wrote:
> to the command line options parser

Hmm, I don't understand this comment. You want to change the comment to
"Override to add test-specific options to the command line options parser"? But
that's wrong, this func doesn't do that. Or is this misaligned, and you meant to
comment the function above?

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_test.py (right):

http://codereview.chromium.org/11273081/diff/10012/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_test.py:30: self._interaction_name_to_run =
interaction_name_to_run
On 2012/11/14 09:50:18, nduca wrote:
> you can verify that it exists, now, right?

Not at the moment, this is "stress_memory", and the value for this key in the
json is the actual interaction.

[tonyg, 2012-11-14 22:02:53]

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
File tools/telemetry/telemetry/compound_interaction_unittest.py (right):

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
tools/telemetry/telemetry/compound_interaction_unittest.py:26: from telemetry
import all_page_interactions
Can this go at the top?

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:24: benchmarks =
discover.Discover(benchmark_dir, 'benchmark',
Currently, it isn't true that all benchmarks end in _benchmark.py. Renaming them
at this point would break the automated tests running on the bots. Would it be
easier to just have suffix be optional and not pass it here?

[nduca, 2012-11-15 06:28:15]

You're not gonna believe it @marja! LGTM [with tonyg's and my minor nit
addressed].

Beautiful work, and thank you so much for putting up with so many review rounds.
That was very kind of you.

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_runner.py (right):

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_runner.py:79: interaction =
test.GetInteraction(page)
Can this just be test.CustomizeBrowserOptionsForPage(page) and we keep the
interaction inside the page? I'm sure that I told you differently in a previous
review, but now that I see it like this, I notice that other than here,
interactions are sealed inside page_test.

[marja, 2012-11-15 12:55:02]

Yay! Thanks for review!

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
File tools/telemetry/telemetry/compound_interaction_unittest.py (right):

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
tools/telemetry/telemetry/compound_interaction_unittest.py:26: from telemetry
import all_page_interactions
On 2012/11/14 22:02:53, tonyg wrote:
> Can this go at the top?

Done; it works because the interactions are imported so selectively.

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:24: benchmarks =
discover.Discover(benchmark_dir, 'benchmark',
On 2012/11/14 22:02:53, tonyg wrote:
> Currently, it isn't true that all benchmarks end in _benchmark.py. Renaming
them
> at this point would break the automated tests running on the bots. Would it be
> easier to just have suffix be optional and not pass it here?

Done; passing an empty suffix here. This will result in importing all the files,
incl. test files here, and we might want to do something about it later. (But
this was the situation before this CL too.)

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_runner.py (right):

https://codereview.chromium.org/11273081/diff/10015/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_runner.py:79: interaction =
test.GetInteraction(page)
On 2012/11/15 06:28:15, nduca wrote:
> Can this just be test.CustomizeBrowserOptionsForPage(page) and we keep the
> interaction inside the page? I'm sure that I told you differently in a
previous
> review, but now that I see it like this, I notice that other than here,
> interactions are sealed inside page_test.

Done; makes very much sense, and I indeed wanted it to be like that :) Happy to
see that our design ideas converge in the end :)

Because this func was so generic (get the interaction and let it customize the
options), I added it to PageTest.