Change for preparation for reference-build on perfbot (media)

Change for preparation for reference-build for PerfBot. PerfBot will be downloading binaries (pyauto.py and its library) for particular build and compare it with current build. This change is preparation for that. It enables media_perf tests to use different binary if reference build option (-a) is used. When the option is set, media_perf_runner runs the test with current build and then reference build (specified by REFERENCE_BUILD_DIR in pyauto_media.py), which I set bogus directory for now.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86361

[imasaki1, 2011-05-18 00:43:33]

Thanks.

[dennis_jeffrey, 2011-05-19 01:20:13]

Several comments.  Sorry about the delay in looking at this.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_base.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_base.py:196: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
(optional) I recommend adding a comment here that this environment variable is
assumed to have value "True" or "False" (until I realized that, I wasn't sure
why you were calling eval() on it).

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_env_names.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_env_names.py:59: # Define this is run on
reference build or not.
I recommend making the comment a little more specific:

# 'True' to run on a reference build, or 'False' otherwise.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_runner.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:116: help='Include reference
build run', default=False)
add the following parameter:

action='store_true'

That way, it's clear that the default value for this option will be False, but
if this flag is specified, the value will change to True.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:149: # Determine if we need to
loop. The defsault is False
I recommend this comment, if it's accurate:

# Determine whether we need to repeat a test using a reference build.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:153:
reference_build_list.append(True)
(optional) I recommend replacing lines 150-153 with this, since it might be a
little clearer.

if options.reference_build:
  reference_build_list = [False, True]
else:
  reference_build_list = [False]

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/pyauto_media.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:30: # Define the location of the
binary used for reference build
Add period at the end of this sentence.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:43: # Setting PYTHONPATH for
reference build
Add period at the end of this sentence.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:44: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
(optional) I recommend adding a comment here that this environment variable is
assumed to have value "True" or "False".

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/ui_perf_test_utils.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/ui_perf_test_utils.py:64: trace: trace string used
for PerfBot graph name (such as 't' or 't_ref')
Add period at end of sentence.

[Nirnimesh, 2011-05-19 06:40:19]

Please update the title & description to make it clear that this change is for
'media' perfbot, to distinguish it from the other perf bots.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_base.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_base.py:196: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
Why do you need to eval it anyway?
Why not assume False if the env is missing, and True otherwise.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_runner.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:149: # Determine if we need to
loop. The defsault is False
typo: defsault

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:151: # reference_build_list =
[True]
remove commented code

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/pyauto_media.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:44: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
Don't eval.

Just:

if os.getenv(...):
  sys.path.insert(0, os.getenv(...))

[imasaki1, 2011-05-20 05:12:01]

Thank you. I also added some other changes the relating to your comments.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_base.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_base.py:196: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> (optional) I recommend adding a comment here that this environment variable is
> assumed to have value "True" or "False" (until I realized that, I wasn't sure
> why you were calling eval() on it).

Changed based on Nirnimesh comments.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_base.py:196: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
On 2011/05/19 06:40:20, Nirnimesh wrote:
> Why do you need to eval it anyway?
> Why not assume False if the env is missing, and True otherwise.

Done.

Also change other boolean environment valuables as well.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_env_names.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_env_names.py:59: # Define this is run on
reference build or not.
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> I recommend making the comment a little more specific:
> 
> # 'True' to run on a reference build, or 'False' otherwise.

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/media_test_runner.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:116: help='Include reference
build run', default=False)
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> add the following parameter:
> 
> action='store_true'
> 
> That way, it's clear that the default value for this option will be False, but
> if this flag is specified, the value will change to True.

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:149: # Determine if we need to
loop. The defsault is False
On 2011/05/19 06:40:20, Nirnimesh wrote:
> typo: defsault

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:149: # Determine if we need to
loop. The defsault is False
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> I recommend this comment, if it's accurate:
> 
> # Determine whether we need to repeat a test using a reference build.

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:151: # reference_build_list =
[True]
On 2011/05/19 06:40:20, Nirnimesh wrote:
> remove commented code

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/media_test_runner.py:153:
reference_build_list.append(True)
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> (optional) I recommend replacing lines 150-153 with this, since it might be a
> little clearer.
> 
> if options.reference_build:
>   reference_build_list = [False, True]
> else:
>   reference_build_list = [False]

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/pyauto_media.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:30: # Define the location of the
binary used for reference build
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> Add period at the end of this sentence.

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:43: # Setting PYTHONPATH for
reference build
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> Add period at the end of this sentence.

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:44: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
On 2011/05/19 06:40:20, Nirnimesh wrote:
> Don't eval.
> 
> Just:
> 
> if os.getenv(...):
>   sys.path.insert(0, os.getenv(...))

Done.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/pyauto_media.py:44: if
eval(os.getenv(MediaTestEnvNames.REFERENCE_BUILD_ENV_NAME)):
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> (optional) I recommend adding a comment here that this environment variable is
> assumed to have value "True" or "False".

Changed based on Nirnimesh's comments.

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
File chrome/test/functional/media/ui_perf_test_utils.py (right):

http://codereview.chromium.org/7031020/diff/2001/chrome/test/functional/media...
chrome/test/functional/media/ui_perf_test_utils.py:64: trace: trace string used
for PerfBot graph name (such as 't' or 't_ref')
On 2011/05/19 01:20:13, dennis_jeffrey wrote:
> Add period at end of sentence.

Done.

[dennis_jeffrey, 2011-05-20 18:10:46]

Just a few more comments.

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
File chrome/test/functional/media/media_test_env_names.py (right):

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:59: # 'True' to run on a
reference build, or 'False' otherwise.
Based on your latest changes, this environment variable is defined if we want a
reference build, and not defined otherwise.  So we should update this comment to
reflect that.

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:62: # Define the location
of the binary used for reference build.
May want to clarify in this comment whether the location should be the path to
the binary itself, or the path to the folder containing the binary.  The name
"REFERENCE_BUILD_DIR" implies that it is the path to the folder containing the
binary.  Is that correct?

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
File chrome/test/functional/media/media_test_runner.py (right):

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_runner.py:118: help='reference build dir
location')
reference -->
Reference

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_runner.py:118: help='reference build dir
location')
Maybe this would be more clear?

dir location -->
directory

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_runner.py:119: parser.add_option('-v',
'--verbose', dest='verbose', help='verbose mode',
verbose mode -->
Verbose mode

[imasaki1, 2011-05-20 20:50:13]

Thanks.

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
File chrome/test/functional/media/media_test_env_names.py (right):

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:59: # 'True' to run on a
reference build, or 'False' otherwise.
On 2011/05/20 18:10:46, dennis_jeffrey wrote:
> Based on your latest changes, this environment variable is defined if we want
a
> reference build, and not defined otherwise.  So we should update this comment
to
> reflect that.

Done.

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:62: # Define the location
of the binary used for reference build.
On 2011/05/20 18:10:46, dennis_jeffrey wrote:
> May want to clarify in this comment whether the location should be the path to
> the binary itself, or the path to the folder containing the binary.  The name
> "REFERENCE_BUILD_DIR" implies that it is the path to the folder containing the
> binary.  Is that correct?

Done.

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
File chrome/test/functional/media/media_test_runner.py (right):

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_runner.py:118: help='reference build dir
location')
On 2011/05/20 18:10:46, dennis_jeffrey wrote:
> Maybe this would be more clear?
> 
> dir location -->
> directory

Done.

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_runner.py:118: help='reference build dir
location')
On 2011/05/20 18:10:46, dennis_jeffrey wrote:
> reference -->
> Reference

Rewrite to match in the comments in the code below.

http://codereview.chromium.org/7031020/diff/13007/chrome/test/functional/medi...
chrome/test/functional/media/media_test_runner.py:119: parser.add_option('-v',
'--verbose', dest='verbose', help='verbose mode',
On 2011/05/20 18:10:46, dennis_jeffrey wrote:
> verbose mode -->
> Verbose mode

Done.

[dennis_jeffrey, 2011-05-20 21:31:43]

LGTM

Just two minor comments.  Also, please wait to hear from Nirnimesh before you
commit.  Thank you!

http://codereview.chromium.org/7031020/diff/16002/chrome/test/functional/medi...
File chrome/test/functional/media/media_test_env_names.py (right):

http://codereview.chromium.org/7031020/diff/16002/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:28: # Define this
environment varialbe when you do not want to report
varialbe -->
variable

http://codereview.chromium.org/7031020/diff/16002/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:61: # reference build, and
not defined otherwise.
and not defined otherwise -->
otherwise do not define this variable

[imasaki1, 2011-05-20 22:07:16]

Thanks. Nirmimesh, can you look at it?

http://codereview.chromium.org/7031020/diff/16002/chrome/test/functional/medi...
File chrome/test/functional/media/media_test_env_names.py (right):

http://codereview.chromium.org/7031020/diff/16002/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:28: # Define this
environment varialbe when you do not want to report
On 2011/05/20 21:31:43, dennis_jeffrey wrote:
> varialbe -->
> variable

Done.

http://codereview.chromium.org/7031020/diff/16002/chrome/test/functional/medi...
chrome/test/functional/media/media_test_env_names.py:61: # reference build, and
not defined otherwise.
On 2011/05/20 21:31:43, dennis_jeffrey wrote:
> and not defined otherwise -->
> otherwise do not define this variable

Done.

[imasaki1, 2011-05-20 22:18:09]

Thanks. Nirmimesh, can you look at it?

[Nirnimesh, 2011-05-23 17:52:44]

LGTM provided you address the one comment.

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
File chrome/test/functional/media/pyauto_media.py (right):

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
chrome/test/functional/media/pyauto_media.py:46:
os.path.join(tempfile.gettempdir(), 'chrome-linux'))
why hardcode to chrome-linux?

[imasaki1, 2011-05-23 18:58:47]

Thanks.

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
File chrome/test/functional/media/pyauto_media.py (right):

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
chrome/test/functional/media/pyauto_media.py:46:
os.path.join(tempfile.gettempdir(), 'chrome-linux'))
On 2011/05/23 17:52:44, Nirnimesh wrote:
> why hardcode to chrome-linux?

This is just test directory, it will be change later. I am adding TODO.

[Nirnimesh, 2011-05-23 20:13:11]

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
File chrome/test/functional/media/pyauto_media.py (right):

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
chrome/test/functional/media/pyauto_media.py:46:
os.path.join(tempfile.gettempdir(), 'chrome-linux'))
Even if it's experimental or temporary, it seems like a bad idea to hardcode the
platform. How about chrome-media-test?

[imasaki1, 2011-05-23 20:26:23]

Thanks.

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
File chrome/test/functional/media/pyauto_media.py (right):

http://codereview.chromium.org/7031020/diff/13011/chrome/test/functional/medi...
chrome/test/functional/media/pyauto_media.py:46:
os.path.join(tempfile.gettempdir(), 'chrome-linux'))
On 2011/05/23 20:13:11, Nirnimesh wrote:
> Even if it's experimental or temporary, it seems like a bad idea to hardcode
the
> platform. How about chrome-media-test?

Done.

[commit-bot: I haz the power, 2011-05-23 23:48:01]

Change committed as 86361