Support for running unit-tests under valgrind on Mac

Support for running unit-tests under valgrind on Mac, and some minor cleanups to valgrind_test.py

[Elliot Glaysher, 2009-02-24 17:45:14]

http://codereview.chromium.org/27079/diff/1/2
File tools/valgrind/valgrind_test.py (right):

http://codereview.chromium.org/27079/diff/1/2#newcode174
Line 174: class ValgrindMac(Valgrind):
I don't understand why you made a separate linux and mac valgrind class. Why do
you need to override Execute() and Run()?

[Nirnimesh-Gmail, 2009-02-24 18:47:59]

http://codereview.chromium.org/27079/diff/1/2
File tools/valgrind/valgrind_test.py (right):

http://codereview.chromium.org/27079/diff/1/2#newcode174
Line 174: class ValgrindMac(Valgrind):
I need a slightly different valgrind command for Mac, coz valgrind on Mac does
not support a few things (suppressions being one of them).

I separated out into different classes coz otherwise it would be a couple of
if-else at more than one place in the Valgrind class.

I've overridden Run() because I want to call Execute() only for Mac, ie no
analyze stuff, so I broke that out.

[Nirnimesh-Gmail, 2009-02-24 22:27:35]

Remove the unecessary --echo_to_stdout from valgrind/chrome_tests.py

[Elliot Glaysher, 2009-02-24 22:53:32]

(BTW, I don't see any differences between your two patch sets)

http://codereview.chromium.org/27079/diff/1/2
File tools/valgrind/valgrind_test.py (right):

http://codereview.chromium.org/27079/diff/1/2#newcode174
Line 174: class ValgrindMac(Valgrind):
On 2009/02/24 18:47:59, Nirnimesh wrote:
> I need a slightly different valgrind command for Mac, coz valgrind on Mac does
> not support a few things (suppressions being one of them).

That's reasonable. OK.

> I've overridden Run() because I want to call Execute() only for Mac, ie no
> analyze stuff, so I broke that out.

This seems really wrong to me. What's the purpose of running valgrind on the mac
if we aren't doing any analysis on the logs afterwards?

[Nirnimesh-Gmail, 2009-02-24 23:01:45]

http://codereview.chromium.org/27079/diff/1/2
File tools/valgrind/valgrind_test.py (right):

http://codereview.chromium.org/27079/diff/1/2#newcode174
Line 174: class ValgrindMac(Valgrind):
My current intention is to begin to run valgrind so that the leaks are at least
available for looking at.

I haven't really looked at the analysis part yet, which I'll do after I get
valgrind running at least. Valgrind on Mac is new and is not extremely
functional yet, so I was kinda focussing on that so far.

[Elliot Glaysher, 2009-02-24 23:28:29]

http://codereview.chromium.org/27079/diff/1/2
File tools/valgrind/valgrind_test.py (right):

http://codereview.chromium.org/27079/diff/1/2#newcode174
Line 174: class ValgrindMac(Valgrind):
On 2009/02/24 23:01:46, Nirnimesh wrote:
> My current intention is to begin to run valgrind so that the leaks are at
least
> available for looking at.
> 
> I haven't really looked at the analysis part yet, which I'll do after I get
> valgrind running at least. Valgrind on Mac is new and is not extremely
> functional yet, so I was kinda focussing on that so far.

Wouldn't it be better to restore all the flow control modified and renamed and
just override Analyze()? This will minimize your patch, and will give you a nice
place to put a TODO that you are explicitly not running analyze for a reason.

[Nirnimesh-Gmail, 2009-02-25 00:04:44]

http://codereview.chromium.org/27079/diff/1/2
File tools/valgrind/valgrind_test.py (right):

http://codereview.chromium.org/27079/diff/1/2#newcode174
Line 174: class ValgrindMac(Valgrind):
Makes sense. Thanks. Done.

[Elliot Glaysher, 2009-02-25 00:13:18]

LGTM

[Elliot Glaysher, 2009-03-02 17:52:09]

Hi,

Did you commit this? The issue is still open.

[Nirnimesh-Gmail, 2009-03-02 18:21:37]

Submitted by jrg.
http://src.chromium.org/viewvc/chrome?view=rev&revision=10552