[Telemetry] Pass test_runner environment in local args instead of a global variable

[Telemetry] Pass test_runner environment in local args instead of a global variable

Also adds more fields to environment to narrow the scope of
benchmark and user story set discovery. This should avoid problems
with adding Python files to unrelated directories, and hides PageTests
from external Telemetry benchmark runners like run_gpu_tests.py and
chrome_proxy's run_benchmark.

R=dtu,nednguyen,sullivan,kbr@chromium.org,bolian
BUG=460181
TEST=tools/perf/run_benchmark; content/test/gpu/run_gpu_tests.py; tools/chrome_proxy/run_benchmark  # All return a full and correct test list.

Committed: https://crrev.com/1da5f7f70ea6dc7dd0667ea78637802c76305f5a
Cr-Commit-Position: refs/heads/master@{#318149}

Committed: https://crrev.com/e97726ddb2d44271cf6416ed7406ec17b97c7fad
Cr-Commit-Position: refs/heads/master@{#318531}

[eakuefner, 2015-02-19 23:44:25]

+kbr, bolian for OWNERS

This is a refresh of dtu's CL in crrev.com/267253002.

[sullivan, 2015-02-20 01:57:59]

lgtm, but would be great for Ned or Dave to take a second pass since they're
both more familiar with our unit tests.

[Ken Russell (switch to Gerrit), 2015-02-20 02:28:05]

gpu changes lgtm

[nednguyen, 2015-02-20 03:36:09]

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark_runner.py:28: user_story_set_dirs=None,
benchmark_aliases=None):
Can you add some documentation of what to these fields mean and their type
follows the style guide here:
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark_runner.py:29: self._top_level_dir =
top_level_dir
This field is the most unclear one to me.

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark_runner.py:58: return self._benchmark_aliases
I wonder whether we can kill this yet.

[eakuefner, 2015-02-20 03:50:16]

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark_runner.py:58: return self._benchmark_aliases
On 2015/02/20 03:36:09, nednguyen wrote:
> I wonder whether we can kill this yet.

I was wondering exactly the same thing -- I saw Manfred killed the aliases that
were in perf/run_benchmark a while ago, but I wasn't sure whether aliases had
stuck around as a concept.

[eakuefner, 2015-02-20 22:33:55]

New patchsets have been uploaded after l-g-t-m from
sullivan@chromium.org,kbr@chromium.org

[eakuefner, 2015-02-20 22:34:21]

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/942663002/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark_runner.py:28: user_story_set_dirs=None,
benchmark_aliases=None):
On 2015/02/20 03:36:09, nednguyen wrote:
> Can you add some documentation of what to these fields mean and their type
> follows the style guide here:
> https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

Done.

[nednguyen, 2015-02-23 16:31:00]

lgtm. We can land this and kill benchmark_aliases in a different patch so it's
easier to track down in case something breaks.

[eakuefner, 2015-02-23 16:46:45]

On 2015/02/23 16:31:00, nednguyen wrote:
> lgtm. We can land this and kill benchmark_aliases in a different patch so it's
> easier to track down in case something breaks.

sgtm

[eakuefner, 2015-02-23 16:59:07]

New patchsets have been uploaded after l-g-t-m from nednguyen@google.com

[dtu, 2015-02-23 18:25:16]

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark_runner.py:43: self._user_story_set_dirs =
user_story_set_dirs or []
I'm not sure we need page_test_dirs or user_story_set_dirs anymore. They were
used by run_measurement, which doesn't exist anymore. Benchmarks also now
reference their page test and user story by Python import instead of
discover.DiscoverClasses().

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark_runner.py:74: def AddCommandLineArgs(cls,
parser, environment):
Kind of think that instead of creating a band-aid class, the parent class should
have environment as an optional parameter. Does that work?

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/environment.py (left):

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/environment.py:7: def __init__(self, base_paths,
benchmark_aliases=None):
No, we don't need benchmark_aliases anymore. We decided on unambiguous,
canonical names, and have individual aliases within each Benchmark.

[eakuefner, 2015-02-23 18:42:49]

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark_runner.py:74: def AddCommandLineArgs(cls,
parser, environment):
On 2015/02/23 18:25:16, dtu wrote:
> Kind of think that instead of creating a band-aid class, the parent class
should
> have environment as an optional parameter. Does that work?

The parent class here is from a library, so I'm not sure what that would look
like. Also, to be clear, the reason I've introduced this class is to contain the
Pylint warnings to one block -- if the commands inherit directly from
command_line.OptparseCommand, we have to have a pylint disable for each command.

I'm willing to clean this up even more, but I'm not sure how.

[eakuefner, 2015-02-23 21:28:36]

On 2015/02/23 at 18:42:49, eakuefner wrote:
>
https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/benchmark_runner.py (right):
> 
>
https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/benchmark_runner.py:74: def AddCommandLineArgs(cls,
parser, environment):
> On 2015/02/23 18:25:16, dtu wrote:
> > Kind of think that instead of creating a band-aid class, the parent class
should
> > have environment as an optional parameter. Does that work?
> 
> The parent class here is from a library, so I'm not sure what that would look
like. Also, to be clear, the reason I've introduced this class is to contain the
Pylint warnings to one block -- if the commands inherit directly from
command_line.OptparseCommand, we have to have a pylint disable for each command.
> 
> I'm willing to clean this up even more, but I'm not sure how.

Whoops, my bad -- I thought the OptparseCommand was coming from the optparse
library, but it turns out we've rolled our own. I'll upload a fix that fixes
this in the way you describe.

[eakuefner, 2015-02-23 22:59:50]

dtu ptal

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark_runner.py:43: self._user_story_set_dirs =
user_story_set_dirs or []
On 2015/02/23 18:25:16, dtu wrote:
> I'm not sure we need page_test_dirs or user_story_set_dirs anymore. They were
> used by run_measurement, which doesn't exist anymore. Benchmarks also now
> reference their page test and user story by Python import instead of
> discover.DiscoverClasses().

Done.

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark_runner.py:74: def AddCommandLineArgs(cls,
parser, environment):
On 2015/02/23 18:42:48, eakuefner wrote:
> On 2015/02/23 18:25:16, dtu wrote:
> > Kind of think that instead of creating a band-aid class, the parent class
> should
> > have environment as an optional parameter. Does that work?
> 
> The parent class here is from a library, so I'm not sure what that would look
> like. Also, to be clear, the reason I've introduced this class is to contain
the
> Pylint warnings to one block -- if the commands inherit directly from
> command_line.OptparseCommand, we have to have a pylint disable for each
command.
> 
> I'm willing to clean this up even more, but I'm not sure how.

So, I investigated a number of potential solutions here. The problem is that
OptparseCommands are declared all over the place, and even making environment
optional in the OptparseCommand is not enough, because Pylint wants subclasses
of OptparseCommand to match the signature.

A potential compromise which you see in the most recent patch set is I renamed
TelemetryCommand to BenchmarkCommand, and moved it to command_line.py.

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/environment.py (left):

https://codereview.chromium.org/942663002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/environment.py:7: def __init__(self, base_paths,
benchmark_aliases=None):
On 2015/02/23 at 18:25:15, dtu wrote:
> No, we don't need benchmark_aliases anymore. We decided on unambiguous,
canonical names, and have individual aliases within each Benchmark.

Okay. I'll remove benchmark_aliases in the next CL.

[dtu, 2015-02-24 01:48:12]

lgtm

[dtu, 2015-02-24 01:50:41]

On 2015/02/24 01:48:12, dtu wrote:
> lgtm

OptparseCommand is used in 3 files, and 2 of them can make use of Environment.
But if you decide to keep BenchmarkCommand, I think it should be defined where
it's being used, as it's not a "blessed API"

[wfa511com, 2015-02-24 02:00:44]

بتاريخ Feb 20, 2015 2:44 AM، كتبها <eakuefner@chromium.org>:

> Reviewers: bolian, dtu, Ken Russell, nednguyen, sullivan,
>
> Message:
> +kbr, bolian for OWNERS
>
> This is a refresh of dtu's CL in crrev.com/267253002.
>
> Description:
> [Telemetry] Pass test_runner environment in local args instead of a global
> variable
>
> Also adds more fields to environment to narrow the scope of benchmark and
> user
> story set discovery. This should avoid problems with adding Python files to
> unrelated directories, and hides PageTests from external Telemetry
> benchmark
> runners like run_gpu_tests.py and chrome_proxy's run_benchmark.
>
> R=dtu,nednguyen,sullivan,kbr@chromium.org,bolian
> BUG=460181
> TEST=tools/perf/run_benchmark; content/test/gpu/run_gpu_tests.py;
> tools/chrome_proxy/run_benchmark  # All return a full and correct test
> list.
>
> Please review this at https://codereview.chromium.org/942663002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+97, -60 lines):
>   M content/test/gpu/run_gpu_test.py
>   M tools/chrome_proxy/run_benchmark
>   M tools/perf/run_benchmark
>   M tools/telemetry/telemetry/benchmark_runner.py
>   D tools/telemetry/telemetry/core/environment.py
>
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eakuefner, 2015-02-24 17:42:49]

Okay, the most recent patch set gets rid of BenchmarkCommand.

[eakuefner, 2015-02-24 17:42:54]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2015-02-24 17:42:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, kbr@chromium.org, sullivan@chromium.org, dtu@chromium.org
Link to the patchset: https://codereview.chromium.org/942663002/#ps80001 (title:
"Get rid of BenchmarkCommand")

[eakuefner, 2015-02-24 17:43:01]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2015-02-24 18:39:30]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2015-02-24 18:39:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
nednguyen@google.com, dtu@chromium.org, sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/942663002/#ps120001
(title: "even more rebase")

[commit-bot: I haz the power, 2015-02-24 18:40:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/942663002/120001

[eakuefner, 2015-02-24 18:49:34]

eakuefner@chromium.org changed reviewers:
- wfa511com@gmail.com

[commit-bot: I haz the power, 2015-02-24 20:29:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-24 20:29:36]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nednguyen, 2015-02-25 06:08:54]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-02-25 06:08:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, kbr@chromium.org, dtu@chromium.org, sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/942663002/#ps140001
(title: "Fix failing Telemetry unittests on waterfall")

[nednguyen, 2015-02-25 06:09:14]

The CQ bit was unchecked by nednguyen@google.com

[eakuefner, 2015-02-25 18:31:48]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2015-02-25 18:32:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/942663002/140001

[nednguyen, 2015-02-25 18:36:18]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2015-02-25 18:36:54]

On 2015/02/25 18:32:37, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/942663002/140001

I think we have some legit failures in #27 here:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...

[eakuefner, 2015-02-25 20:32:25]

On 2015/02/25 at 18:36:54, nednguyen wrote:
> On 2015/02/25 18:32:37, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/942663002/140001
> 
> I think we have some legit failures in #27 here:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...

Pretty odd... if you look at the callsite it's complaining about in my most
recent patch, you'll see I modified it to pass None in that position. I'll look
into it a bit more and then try again.

[eakuefner, 2015-02-25 20:47:15]

On 2015/02/25 at 20:32:25, eakuefner wrote:
> On 2015/02/25 at 18:36:54, nednguyen wrote:
> > On 2015/02/25 18:32:37, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/942663002/140001
> > 
> > I think we have some legit failures in #27 here:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
> 
> Pretty odd... if you look at the callsite it's complaining about in my most
recent patch, you'll see I modified it to pass None in that position. I'll look
into it a bit more and then try again.

I couldn't reproduce the failure locally at all, so I've uploaded a duplicate
patch set just to be safe and kicked off try jobs. My best guess is that some
wonky stuff with the pyc files is happening.

[eakuefner, 2015-02-25 22:43:14]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2015-02-25 22:43:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, kbr@chromium.org, dtu@chromium.org, sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/942663002/#ps160001
(title: "Try again")

[commit-bot: I haz the power, 2015-02-25 22:44:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/942663002/160001

[commit-bot: I haz the power, 2015-02-25 23:40:46]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-25 23:41:31]

Patchset 9 (id:??) landed as
https://crrev.com/1da5f7f70ea6dc7dd0667ea78637802c76305f5a
Cr-Commit-Position: refs/heads/master@{#318149}

[eakuefner, 2015-02-26 02:37:43]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/955183003/ by eakuefner@chromium.org.

The reason for reverting is: crbug.com/462063.

[eakuefner, 2015-02-26 22:45:56]

eakuefner@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2015-02-27 00:41:08]

On 2015/02/26 02:37:43, eakuefner wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/955183003/ by mailto:eakuefner@chromium.org.
> 
> The reason for reverting is: crbug.com/462063.

The output of find_dependencies seems to be the same. I've started a local build
of cros, which will take a bit of time. I'll let you know what happens. Thanks!

[eakuefner, 2015-02-27 00:50:00]

On 2015/02/27 at 00:41:08, achuith wrote:
> On 2015/02/26 02:37:43, eakuefner wrote:
> > A revert of this CL (patchset #9 id:160001) has been created in
> > https://codereview.chromium.org/955183003/ by mailto:eakuefner@chromium.org.
> > 
> > The reason for reverting is: crbug.com/462063.
> 
> The output of find_dependencies seems to be the same. I've started a local
build of cros, which will take a bit of time. I'll let you know what happens.
Thanks!

Thanks Achuith! I hope everything goes well.

[achuithb, 2015-02-27 21:15:17]

On 2015/02/27 00:50:00, eakuefner wrote:
> On 2015/02/27 at 00:41:08, achuith wrote:
> > On 2015/02/26 02:37:43, eakuefner wrote:
> > > A revert of this CL (patchset #9 id:160001) has been created in
> > > https://codereview.chromium.org/955183003/ by
mailto:eakuefner@chromium.org.
> > > 
> > > The reason for reverting is: crbug.com/462063.
> > 
> > The output of find_dependencies seems to be the same. I've started a local
> build of cros, which will take a bit of time. I'll let you know what happens.
> Thanks!
> 
> Thanks Achuith! I hope everything goes well.

Fyi the build completely successfully.

[achuithb, 2015-02-27 21:15:27]

On 2015/02/27 21:15:17, achuithb wrote:
> On 2015/02/27 00:50:00, eakuefner wrote:
> > On 2015/02/27 at 00:41:08, achuith wrote:
> > > On 2015/02/26 02:37:43, eakuefner wrote:
> > > > A revert of this CL (patchset #9 id:160001) has been created in
> > > > https://codereview.chromium.org/955183003/ by
> mailto:eakuefner@chromium.org.
> > > > 
> > > > The reason for reverting is: crbug.com/462063.
> > > 
> > > The output of find_dependencies seems to be the same. I've started a local
> > build of cros, which will take a bit of time. I'll let you know what
happens.
> > Thanks!
> > 
> > Thanks Achuith! I hope everything goes well.
> 
> Fyi the build completely successfully.

completed

[eakuefner, 2015-02-27 21:32:29]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2015-02-27 21:32:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, kbr@chromium.org, dtu@chromium.org, sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/942663002/#ps180001
(title: "Fix find_dependencies issue")

[commit-bot: I haz the power, 2015-02-27 21:33:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/942663002/180001

[commit-bot: I haz the power, 2015-02-27 22:33:18]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-27 22:33:58]

Patchset 10 (id:??) landed as
https://crrev.com/e97726ddb2d44271cf6416ed7406ec17b97c7fad
Cr-Commit-Position: refs/heads/master@{#318531}