Add option to command tester for running a test multiple times. Use that

Add option to command tester for running a test multiple times. Use that
option to run browser_startup time multiple times to smooth out measurements,
and hopefully avoid PERF_REGRESS flakes (seems to happen often on
the Mac bots).

BUG= none
TEST= trybots
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=7148

[Nick Bray, 2011-11-07 23:39:09]

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
File tests/browser_startup_time/nacl.scons (right):

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
tests/browser_startup_time/nacl.scons:40: num_runs='5',
This parameter should be an integer, at least until it is passed on the command
line.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py
File tools/command_tester.py (right):

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:110: 'num_runs': '1',
Make this an integer.  The option parser will do the right thing and you can
remove all the int() casts.

If you're feeling agressive, you can fix capture_output, too, although I'd
rather that was in a separate CL.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:144: return '[ RUN      ] nacl.%s%s' %
(GlobalSettings['name'], run_message_extra)
It might be clearer to produce the base string and then append to it?

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:275: elif type(GlobalSettings[option]) == int:
Check this out!

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:327: if stdout:
The case in which stdout is printed has changed.  Are we printing too much?  In
what cases will stdout be printed?

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:330: if stderr:
is not None?
!= ''?
It's hard to tell what your intent is.  If you're legitimately trying to check
for both, I think it's a smell something else wrong.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:528: def main(argv):
Capitalize

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:529: global GlobalPlatform
Do not declare globals unless you assign directly to the global variable.

... particularly because it's called "Global" - no confusion, there.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:550: os.putenv(key, val)
putenv considered harmful.  Assign to os.environ?

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined.py
File tools/process_perf_combined.py (right):

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined....
tools/process_perf_combined.py:13: ACCUMULATED_TIMES = dict()
{} preferred to dict(), but really - I'd rather see this global state disappear
entirely.

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined....
tools/process_perf_combined.py:17: def list_to_string(l):
CammelCase

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined....
tools/process_perf_combined.py:19: 
Two lines between functions

[jvoung - send to chromium..., 2011-11-09 00:50:54]

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
File tests/browser_startup_time/nacl.scons (right):

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
tests/browser_startup_time/nacl.scons:40: num_runs='5',
On 2011/11/07 23:39:09, Nick Bray wrote:
> This parameter should be an integer, at least until it is passed on the
command
> line.

I'm seeing some weird stuff:


run_browser_startup_time_test
(scons-out/nacl-x86-32/test_results/browser_startup_time.out) failed:
AttributeError `'int' object has no attribute 'abspath'' trying to evaluate
`${SOURCES[1].abspath}'

when I do that...

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py
File tools/command_tester.py (right):

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:110: 'num_runs': '1',
On 2011/11/07 23:39:09, Nick Bray wrote:
> Make this an integer.  The option parser will do the right thing and you can
> remove all the int() casts.
> 
> If you're feeling agressive, you can fix capture_output, too, although I'd
> rather that was in a separate CL.

Done. Leaving capture_output, track_cmdtime, alone for now.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:144: return '[ RUN      ] nacl.%s%s' %
(GlobalSettings['name'], run_message_extra)
On 2011/11/07 23:39:09, Nick Bray wrote:
> It might be clearer to produce the base string and then append to it?

Done.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:275: elif type(GlobalSettings[option]) == int:
On 2011/11/07 23:39:09, Nick Bray wrote:
> Check this out!

Done =P

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:327: if stdout:
On 2011/11/07 23:39:09, Nick Bray wrote:
> The case in which stdout is printed has changed.  Are we printing too much? 
In
> what cases will stdout be printed?

Leaving this the same as before for now, as noted below.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:330: if stderr:
On 2011/11/07 23:39:09, Nick Bray wrote:
> is not None?
> != ''?
> It's hard to tell what your intent is.  If you're legitimately trying to check
> for both, I think it's a smell something else wrong. 

Hmm... I don't remember why I did that change anymore. I'll leave the condition
the same. They should both be not None, or both be None.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:528: def main(argv):
On 2011/11/07 23:39:09, Nick Bray wrote:
> Capitalize

capitalize Main? ok.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:529: global GlobalPlatform
On 2011/11/07 23:39:09, Nick Bray wrote:
> Do not declare globals unless you assign directly to the global variable.
> 
> ... particularly because it's called "Global" - no confusion, there.

Done.

http://codereview.chromium.org/8439032/diff/3001/tools/command_tester.py#newc...
tools/command_tester.py:550: os.putenv(key, val)
On 2011/11/07 23:39:09, Nick Bray wrote:
> putenv considered harmful.  Assign to os.environ?

Don't know the specifics (does it sometimes not work?)... but okay.

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined.py
File tools/process_perf_combined.py (right):

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined....
tools/process_perf_combined.py:13: ACCUMULATED_TIMES = dict()
On 2011/11/07 23:39:09, Nick Bray wrote:
> {} preferred to dict(), but really - I'd rather see this global state
disappear
> entirely.
> 

oops, done.

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined....
tools/process_perf_combined.py:17: def list_to_string(l):
On 2011/11/07 23:39:09, Nick Bray wrote:
> CammelCase

Done.

http://codereview.chromium.org/8439032/diff/3001/tools/process_perf_combined....
tools/process_perf_combined.py:19: 
On 2011/11/07 23:39:09, Nick Bray wrote:
> Two lines between functions

Done.

[jvoung - send to chromium..., 2011-11-09 01:00:38]

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
File tests/browser_startup_time/nacl.scons (right):

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
tests/browser_startup_time/nacl.scons:40: num_runs='5',
On 2011/11/09 00:50:54, jvoung wrote:
> On 2011/11/07 23:39:09, Nick Bray wrote:
> > This parameter should be an integer, at least until it is passed on the
> command
> > line.
> 
> I'm seeing some weird stuff:
> 
> 
> run_browser_startup_time_test
> (scons-out/nacl-x86-32/test_results/browser_startup_time.out) failed:
> AttributeError `'int' object has no attribute 'abspath'' trying to evaluate
> `${SOURCES[1].abspath}'
> 
> when I do that...

Same thing happens if I go into SConstruct and change this code:

  # Other extra flags
  if not capture_output:
    script_flags.extend(['--capture_output', '0'])

from '0' to 0.

Maybe the command arguments are being analyzed as "sources".

So... can I leave this one as a string for now?

[jvoung - send to chromium..., 2011-11-09 17:42:53]

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
File tests/browser_startup_time/nacl.scons (right):

http://codereview.chromium.org/8439032/diff/3001/tests/browser_startup_time/n...
tests/browser_startup_time/nacl.scons:40: num_runs='5',
On 2011/11/09 01:00:38, jvoung wrote:
> On 2011/11/09 00:50:54, jvoung wrote:
> > On 2011/11/07 23:39:09, Nick Bray wrote:
> > > This parameter should be an integer, at least until it is passed on the
> > command
> > > line.
> > 
> > I'm seeing some weird stuff:
> > 
> > 
> > run_browser_startup_time_test
> > (scons-out/nacl-x86-32/test_results/browser_startup_time.out) failed:
> > AttributeError `'int' object has no attribute 'abspath'' trying to evaluate
> > `${SOURCES[1].abspath}'
> > 
> > when I do that...
> 
> Same thing happens if I go into SConstruct and change this code:
> 
>   # Other extra flags
>   if not capture_output:
>     script_flags.extend(['--capture_output', '0'])
> 
> from '0' to 0.
> 
> Maybe the command arguments are being analyzed as "sources".
> 
> So... can I leave this one as a string for now?

Okay, fixed. Is there a better way to detect non-strings and non-Scons objects
in python? Currently only converting ints to strings.

[Nick Bray, 2011-11-11 22:42:39]

LGTM w/ changes, most of which are nit-ish.  The gtest format and the isinstance
changes are the ones I care most about.

http://codereview.chromium.org/8439032/diff/16006/SConstruct
File SConstruct (right):

http://codereview.chromium.org/8439032/diff/16006/SConstruct#newcode2010
SConstruct:2010: if type(flag_value) == int:
isinstance(flag_value, int)

http://codereview.chromium.org/8439032/diff/16006/tools/command_tester.py
File tools/command_tester.py (right):

http://codereview.chromium.org/8439032/diff/16006/tools/command_tester.py#new...
tools/command_tester.py:114: 'process_output_single': '',
Would None be a better default?  (See other uses.)  You could then have very
clear "is not None" tests, but I won't requires that.

http://codereview.chromium.org/8439032/diff/16006/tools/command_tester.py#new...
tools/command_tester.py:142: msg += '  (run %d times)' % num_runs
Actually... you might want to put this info on its own line and move it out of
this function.

Bots expect a certain formatting for gtest, and this might trip them up?

http://codereview.chromium.org/8439032/diff/16006/tools/process_perf_combined.py
File tools/process_perf_combined.py (right):

http://codereview.chromium.org/8439032/diff/16006/tools/process_perf_combined...
tools/process_perf_combined.py:32: assert(unit == old_unit)
assert(unit == old_unit), (unit, old_unit) is more programmer-friendly because
it helps understand why the assert failed.

http://codereview.chromium.org/8439032/diff/16006/tools/process_perf_combined...
tools/process_perf_combined.py:33: if type(value) == list:
isinstance(value, list)

[jvoung - send to chromium..., 2011-11-11 23:28:26]

Thanks, dones.

http://codereview.chromium.org/8439032/diff/16006/SConstruct
File SConstruct (right):

http://codereview.chromium.org/8439032/diff/16006/SConstruct#newcode2010
SConstruct:2010: if type(flag_value) == int:
On 2011/11/11 22:42:39, Nick Bray wrote:
> isinstance(flag_value, int)

Done.

http://codereview.chromium.org/8439032/diff/16006/tools/command_tester.py
File tools/command_tester.py (right):

http://codereview.chromium.org/8439032/diff/16006/tools/command_tester.py#new...
tools/command_tester.py:114: 'process_output_single': '',
On 2011/11/11 22:42:39, Nick Bray wrote:
> Would None be a better default?  (See other uses.)  You could then have very
> clear "is not None" tests, but I won't requires that.

I'll Dones it.

http://codereview.chromium.org/8439032/diff/16006/tools/command_tester.py#new...
tools/command_tester.py:142: msg += '  (run %d times)' % num_runs
On 2011/11/11 22:42:39, Nick Bray wrote:
> Actually... you might want to put this info on its own line and move it out of
> this function.
> 
> Bots expect a certain formatting for gtest, and this might trip them up?

Done.

http://codereview.chromium.org/8439032/diff/16006/tools/process_perf_combined.py
File tools/process_perf_combined.py (right):

http://codereview.chromium.org/8439032/diff/16006/tools/process_perf_combined...
tools/process_perf_combined.py:32: assert(unit == old_unit)
On 2011/11/11 22:42:39, Nick Bray wrote:
> assert(unit == old_unit), (unit, old_unit) is more programmer-friendly because
> it helps understand why the assert failed.

Done

http://codereview.chromium.org/8439032/diff/16006/tools/process_perf_combined...
tools/process_perf_combined.py:33: if type(value) == list:
On 2011/11/11 22:42:39, Nick Bray wrote:
> isinstance(value, list)

Done.

[Nick Bray, 2011-11-11 23:46:33]

LGTM w/ nits.

http://codereview.chromium.org/8439032/diff/20001/tools/command_tester.py
File tools/command_tester.py (right):

http://codereview.chromium.org/8439032/diff/20001/tools/command_tester.py#new...
tools/command_tester.py:139: msg = '[ RUN      ] nacl.%s' %
GlobalSettings['name']
nit: revert it to the original version?  No point in touching a line of code
unless it matters.

http://codereview.chromium.org/8439032/diff/20001/tools/command_tester.py#new...
tools/command_tester.py:558: print " ".join(env_vars + command)
' '.join(...)

[jvoung - send to chromium..., 2011-11-12 19:46:51]

Thanks, 7148...

http://codereview.chromium.org/8439032/diff/20001/tools/command_tester.py
File tools/command_tester.py (right):

http://codereview.chromium.org/8439032/diff/20001/tools/command_tester.py#new...
tools/command_tester.py:139: msg = '[ RUN      ] nacl.%s' %
GlobalSettings['name']
On 2011/11/11 23:46:33, Nick Bray wrote:
> nit: revert it to the original version?  No point in touching a line of code
> unless it matters.

Done.

http://codereview.chromium.org/8439032/diff/20001/tools/command_tester.py#new...
tools/command_tester.py:558: print " ".join(env_vars + command)
On 2011/11/11 23:46:33, Nick Bray wrote:
> ' '.join(...)

Done.