PyAuto test for GPU memory consumption

First step towards adding PyAuto test for GPU memory consumption.  A few issues to resolve before considering checking in:
1. @mmocny, would you prefer that I keep the GpuMemoryManager functions that I added in, or would you prefer that I stub them out (since you'll be replacing them... whichever is easier for your workflow)
2. Where should the GpuProcessHostOnMemStatsCallback class be added?  It seems natural to leave it next to its caller in testing_automation_provider.cc, but there is only one other class in that file.
3. Will just adding the gpu_mem.py file for the new test activate the test, or is there a separate test list file (I'd prefer to activate the test separately)?

BUG=135525
TEST=gpu_mem.py

[mmocny, 2012-07-17 19:26:03]

Were you planning to also add tests for tracking tab visibility during tab
switching?  Does that come later?

If you are happy with my proposal for using the Stats interface in my CL, I can
work towards landing a dummy implementation asap so you can work off that
instead of stubbing yourself here.

http://codereview.chromium.org/10702200/diff/2001/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_manager.h (right):

http://codereview.chromium.org/10702200/diff/2001/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_manager.h:63: void
GetMemStats(content::GpuMemStats& stats);
In the CL I posted, I proposed GpuMemoryManager providing stats using data
structures that make the most sense for it, not for particular tests.

Since there may be a suite of tests for various purposes, I think it may be
better for content::GpuMemStats (which is really GpuMemStatsNeededForTestX) to
be filled in outside of GpuMemoryManager.

[jam, 2012-07-17 19:31:49]

please hold off on adding pyauto tests that are meant to be supported by other
devs. there's a larger discussion going on about whether pyauto is a supported
(i.e. all devs are responsible for tracking/debugging/fixing these tests).

in general, this could be done using browser_tests, and you wouldn't need to add
all the test harness in testing_automation_provider.cc. a browser_test directly
has access to all the data that you need without having to do IPCs to get to
another process.

[nduca, 2012-07-17 19:37:30]

Wait, what?

[nduca, 2012-07-17 19:41:08]

I thought browser_tests weren't going to work on Android and CrOS?

[ccameron, 2012-07-17 20:00:46]

http://codereview.chromium.org/10702200/diff/2001/content/common/gpu/gpu_memo...
File content/common/gpu/gpu_memory_manager.h (right):

http://codereview.chromium.org/10702200/diff/2001/content/common/gpu/gpu_memo...
content/common/gpu/gpu_memory_manager.h:63: void
GetMemStats(content::GpuMemStats& stats);
Yeah, I thought this would be the case -- I'll update the patch to put the logic
outside of GpuMemoryManager (I originally included this just to make sure that
calling in did indeed work).

[jam, 2012-07-17 20:00:55]

On Tue, Jul 17, 2012 at 12:41 PM, <nduca@chromium.org> wrote:

> I thought browser_tests weren't going to work on Android and CrOS?
>

For cros, afaik no one has looked into bugs that make it flaky there, but
there's nothing inherently preventing it from running.

As for Android, I'm not familiar with their testing story. I would be
extremely wary from having to support another test harness for chrome tests
just for Android. i.e. going by this logic, we convert all the
browser_tests that can run on Android to pyauto? that effectively means
supporting two test harnesses by all developers, with all the issues of
automation (i.e. all the harness code every time some new class in the
browser needs to be interacted with) that we didn't like with ui_tests and
made us add browser_tests.

but, like I said, there's a larger conversation about this, so stay tuned.
in the meantime, please don't commit this until the decision is made.


>
http://codereview.chromium.**org/10702200/<http://codereview.chromium.org/107...
>

[ccameron, 2012-07-17 20:09:22]

On 2012/07/17 19:26:03, mmocny wrote:
> Were you planning to also add tests for tracking tab visibility during tab
> switching?  Does that come later?

I hadn't planned to, but now that you've brought it up, validating that the
memory manager's tab visibility is consistent with the user should be helpful.

> If you are happy with my proposal for using the Stats interface in my CL, I
can
> work towards landing a dummy implementation asap so you can work off that
> instead of stubbing yourself here.

Yes, that interface will be exactly what I need to get this interface closer
(and then I can update GpuChildThread::OnGetMemStats to do something
meaningful).

[Nirnimesh, 2012-07-17 20:20:22]

On 2012/07/17 20:00:55, John Abd-El-Malek wrote:
> On Tue, Jul 17, 2012 at 12:41 PM, <mailto:nduca@chromium.org> wrote:
> 
> > I thought browser_tests weren't going to work on Android and CrOS?
> >
> 
> For cros, afaik no one has looked into bugs that make it flaky there, but
> there's nothing inherently preventing it from running.
> 
> As for Android, I'm not familiar with their testing story. I would be
> extremely wary from having to support another test harness for chrome tests
> just for Android. i.e. going by this logic, we convert all the
> browser_tests that can run on Android to pyauto? that effectively means
> supporting two test harnesses by all developers, with all the issues of
> automation (i.e. all the harness code every time some new class in the
> browser needs to be interacted with) that we didn't like with ui_tests and
> made us add browser_tests.
> 
> but, like I said, there's a larger conversation about this, so stay tuned.
> in the meantime, please don't commit this until the decision is made.
> 
> 
> >
>
http://codereview.chromium.**org/10702200/%3Chttp://codereview.chromium.org/1...>
> >

The meeting is set for next Monday, but to respond to your comments, let me
clarify that:
1. Not just android, you continue to ignore chromeos. No one is saying
browser_tests should not run. But there are things that require you to be out of
process from chrome (restart scenarios, interaction with other system services),
and browser_tests is definitely not a good fit then. And if it is so easy to
maintain browser_tests how come 2/3rd of it is broken on chromeos for several
months?
2. No decision was ever made to stop supporting pyauto. It's not just for
testers. It does not have many of the pitfalls ui_tests had. Most automation
commands reuse a single IPC. Automation hooks for the most part are fairly
routine, boilerplate code. Maintaining that is hard? I've been doing that on the
side for the last 2 years. Really, it's not as difficult as you're making it
seem.
3. I think the decision whether browser_tests or pyauto is a better fit should
lie with the developer / tester writing the test.

[jam, 2012-07-17 20:32:12]

On Tue, Jul 17, 2012 at 1:20 PM, <nirnimesh@chromium.org> wrote:

> On 2012/07/17 20:00:55, John Abd-El-Malek wrote:
>
>  On Tue, Jul 17, 2012 at 12:41 PM, <mailto:nduca@chromium.org> wrote:
>>
>
>  > I thought browser_tests weren't going to work on Android and CrOS?
>> >
>>
>
>  For cros, afaik no one has looked into bugs that make it flaky there, but
>> there's nothing inherently preventing it from running.
>>
>
>  As for Android, I'm not familiar with their testing story. I would be
>> extremely wary from having to support another test harness for chrome
>> tests
>> just for Android. i.e. going by this logic, we convert all the
>> browser_tests that can run on Android to pyauto? that effectively means
>> supporting two test harnesses by all developers, with all the issues of
>> automation (i.e. all the harness code every time some new class in the
>> browser needs to be interacted with) that we didn't like with ui_tests and
>> made us add browser_tests.
>>
>
>  but, like I said, there's a larger conversation about this, so stay tuned.
>> in the meantime, please don't commit this until the decision is made.
>>
>
>
>  >
>>
>
> http://codereview.chromium.****org/10702200/%3Chttp://coderev**
> iew.chromium.org/10702200/ <http://codereview.chromium.org/10702200/>>
>
>> >
>>
>
> The meeting is set for next Monday, but to respond to your comments, let me
> clarify that:
> 1. Not just android, you continue to ignore chromeos.


I'm not ignoring chromeos. I've seen the chromeos bug, and it seems that
someone just needs to put in the effort to figure out why it's flaky there.
That's a job for someone on the cros team.


> No one is saying
> browser_tests should not run. But there are things that require you to be
> out of
> process from chrome (restart scenarios, interaction with other system
> services),
> and browser_tests is definitely not a good fit then.


Yes, there are limitations of browser_tests. I think we need to come to an
agreement on what to do in those cases (i.e. use webdriver/pyauto or the
new automation framework that kkania is working on), and I leave that for
the meeting..


> And if it is so easy to
> maintain browser_tests how come 2/3rd of it is broken on chromeos for
> several
> months?
> 2. No decision was ever made to stop supporting pyauto.


To be very explicit, what I'm talking about is supporting pyauto by all
developers. This means that pyauto tests turn the tree red, and developers
are responsible for fixing those tests. Also other things like supporting
all the supporting code in the automation layer.


> It's not just for
> testers. It does not have many of the pitfalls ui_tests had. Most
> automation
> commands reuse a single IPC.


Whether the automation is implemented by a single json IPC, or through many
IPCs, is not the core of the issue. There is still a lot of code to respond
to the request:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/automation/tes...


> Automation hooks for the most part are fairly
> routine, boilerplate code. Maintaining that is hard? I've been doing that
> on the
> side for the last 2 years. Really, it's not as difficult as you're making
> it
> seem.
> 3. I think the decision whether browser_tests or pyauto is a better fit
> should
> lie with the developer / tester writing the test.
>

Right now, pyauto tests don't turn the tree red. So if a developer writes a
pyauto test, it doesn't catch regressions. As we're all aware, sometimes
pyauto have caught regressions but the test is ignored/just marked
flaky/disabled. The status quo is that developers are only responsible for
browser_tests integration tests. We should have the tests that developers
write be tree-breaking when they fail. And that's where we need to come to
an agreement about which test harnesses all developers have to support.


>
>
http://codereview.chromium.**org/10702200/<http://codereview.chromium.org/107...
>

[nduca, 2012-07-17 22:40:25]

http://codereview.chromium.org/10702200/diff/4003/chrome/test/data/gpu/mem_3d...
File chrome/test/data/gpu/mem_3dcss.html (right):

http://codereview.chromium.org/10702200/diff/4003/chrome/test/data/gpu/mem_3d...
chrome/test/data/gpu/mem_3dcss.html:2: <head>
Might put an explanation of what this is trying to do here. We typically put
text right in the test that says this. E.g. in body just before your
myPerspective div, say the rationale of the geometry etc

http://codereview.chromium.org/10702200/diff/4003/chrome/test/data/gpu/mem_3d...
chrome/test/data/gpu/mem_3dcss.html:4: .myPerspective
chrome css style is my-perspective for class names

http://codereview.chromium.org/10702200/diff/4003/chrome/test/data/gpu/mem_3d...
chrome/test/data/gpu/mem_3dcss.html:6: width:  800px;
alphebetize these... - comes first.

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
File chrome/test/functional/gpu_mem.py (right):

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
chrome/test/functional/gpu_mem.py:17: def testSanity(self):
In general, prefer test name that tell someone who'se in some random chrome team
(e.g. not gpu) to understand what broadly is broken in case it fails. That way
they can route the bug failure to someone easily, give it a decent bug title,
and give the person who gets the bug a good first guess at whats going wrong.

E.g. "testThatGpuMemoryAPIReturnsAValue"

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
chrome/test/functional/gpu_mem.py:21: print 'Using ', self._getGpuMemUsage(), '
bytes of GPU memory'
I think the intent of this is that it consumes a lot of memory, right? SO you
should have a part of the test that verifies that a single page consumed at
least some <smoke test amount> of memory. So, for example, if we suddenly
optimize the drawing of solid color tiles to use shaders instead of texture, we
get an error that points out that the test has stopped being relevant.

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
chrome/test/functional/gpu_mem.py:28: # TODO - compute this reliably -- this is
a wild guess
Hmmm so how are you going to compute this? Doesn't seem like we should land this
until you're computing this.

What if you add a command line option to chrome that boots up chrome with a
fixed gpu memory budget? E.g. memory manager uses a fake amount of memory.

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
chrome/test/functional/gpu_mem.py:33: # TODO - verify that we never exceeded the
GPU memory budget
How?

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
chrome/test/functional/gpu_mem.py:36: #        around some resources
Maybe using a fixed amount of memory is good here

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
chrome/test/functional/gpu_mem.py:37: print 'Using ', self._getGpuMemUsage(), '
bytes of GPU memory'
Should we have a test that verifies that we get a sane value for memory stats?
E.g. is the code that figures out the gpu memory stats working?

http://codereview.chromium.org/10702200/diff/4003/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/10702200/diff/4003/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:3833: def GetGpuMemStats(self, windex=0):
Is this where we put things like this? I would have expected this in the actual
test, maybe nirimesh can advise.

[mmocny, 2012-07-18 15:20:18]

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
File chrome/test/functional/gpu_mem.py (right):

http://codereview.chromium.org/10702200/diff/4003/chrome/test/functional/gpu_...
chrome/test/functional/gpu_mem.py:37: print 'Using ', self._getGpuMemUsage(), '
bytes of GPU memory'
GpuMemoryManager will have unittests here, but those use fake stubs that will
always provide "valid" numbers for memory allocation hints etc.

We certainly should have a test to make sure we are getting valid/correct
numbers with real pages.  This is basically the same case as with tab
visibility, so perhaps there should first be a simple, separate, sanity test for
that?  E.g. create a tab, make sure its visible and non-0 alloc, create a second
tab make sure first is non visible, create many tabs make sure some have 0
alloc.

On 2012/07/17 22:40:25, nduca wrote:
> Should we have a test that verifies that we get a sane value for memory stats?
> E.g. is the code that figures out the gpu memory stats working?