[Telemetry] Add test to prevents adding new dependencies to telemetry.

[Telemetry] Add test to prevents adding new dependencies to telemetry.

BUG=472672
Committed: https://crrev.com/e2ff83b4c95cbd9c0df8fd338c9afa39b861cb14
Cr-Commit-Position: refs/heads/master@{#325490}

[nednguyen, 2015-04-14 17:38:30]

nednguyen@google.com changed reviewers:
+ aiolos@chromium.org, dtu@chromium.org, sullivan@chromium.org

[nednguyen, 2015-04-14 17:50:38]

On 2015/04/14 17:38:30, nednguyen wrote:

Actually this may not work as I expect. I add a simple third_party dependency
but find_dependencies script fails to list that one.

[nednguyen, 2015-04-14 18:01:44]

On 2015/04/14 17:50:38, nednguyen wrote:
> On 2015/04/14 17:38:30, nednguyen wrote:
> 
> Actually this may not work as I expect. I add a simple third_party dependency
> but find_dependencies script fails to list that one.

Ok, this is not the fault of find_dependencies script but my fault for not
setting target_paths to the cover more path. I will update the patch shortly.

[nednguyen, 2015-04-14 18:37:24]

On 2015/04/14 18:01:44, nednguyen wrote:
> On 2015/04/14 17:50:38, nednguyen wrote:
> > On 2015/04/14 17:38:30, nednguyen wrote:
> > 
> > Actually this may not work as I expect. I add a simple third_party
dependency
> > but find_dependencies script fails to list that one.
> 
> Ok, this is not the fault of find_dependencies script but my fault for not
> setting target_paths to the cover more path. I will update the patch shortly.

Adding 'benchmark_runner.py' helps discover
"third_party/webdriver/pylib/selenium/".

Chat offline with Dave, 'benchmark_runner.py's dependencies probably should be
enough to cover all dependencies in telemetry. *shrug*

[sullivan, 2015-04-14 19:20:34]

lgtm

https://codereview.chromium.org/1057553003/diff/20001/tools/telemetry/PRESUBM...
File tools/telemetry/PRESUBMIT.py (right):

https://codereview.chromium.org/1057553003/diff/20001/tools/telemetry/PRESUBM...
tools/telemetry/PRESUBMIT.py:60: ' dtu@, or nednguyen@ on how to proceeds with
this change.'))
nit: s/proceeds/proceed/

[nednguyen, 2015-04-14 19:46:54]

https://codereview.chromium.org/1057553003/diff/20001/tools/telemetry/PRESUBM...
File tools/telemetry/PRESUBMIT.py (right):

https://codereview.chromium.org/1057553003/diff/20001/tools/telemetry/PRESUBM...
tools/telemetry/PRESUBMIT.py:60: ' dtu@, or nednguyen@ on how to proceeds with
this change.'))
On 2015/04/14 19:20:34, sullivan wrote:
> nit: s/proceeds/proceed/

Done.

[nednguyen, 2015-04-14 20:57:20]

Dave, Kari: do you want to take a look at this?

[aiolos (Not reviewing), 2015-04-14 22:41:39]

On 2015/04/14 20:57:20, nednguyen wrote:
> Dave, Kari: do you want to take a look at this?

lgtm

[nednguyen, 2015-04-14 22:41:51]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-04-14 22:41:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/1057553003/#ps40001
(title: "Address Annie's nit")

[commit-bot: I haz the power, 2015-04-14 22:41:58]

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

[commit-bot: I haz the power, 2015-04-14 22:54:54]

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

[commit-bot: I haz the power, 2015-04-14 22:54:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nednguyen, 2015-04-15 16:43:22]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2015-04-15 16:43:56]

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

[commit-bot: I haz the power, 2015-04-15 16:52:21]

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

[commit-bot: I haz the power, 2015-04-15 16:52:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[iannucci, 2015-04-15 21:53:12]

On 2015/04/15 16:52:22, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Hm... I don't think you're allowed to import anything in a PRESUBMIT.py. Path
and stuff are provided by input_api, for example. I believe json is as well.

Is is possible that telemetry.util ends up importing something else transitively
which tries to load libdc1394?

R

[nednguyen, 2015-04-15 21:55:13]

On 2015/04/15 21:53:12, iannucci wrote:
> On 2015/04/15 16:52:22, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Hm... I don't think you're allowed to import anything in a PRESUBMIT.py. Path
> and stuff are provided by input_api, for example. I believe json is as well.
> 
> Is is possible that telemetry.util ends up importing something else
transitively
> which tries to load libdc1394?
> 
> R

Thanks Robbie. I will leave the PRESUBMIT script alone and rewrite this check as
a "unittest" in telemetry.

[dtu, 2015-04-15 22:25:01]

https://chromiumcodereview.appspot.com/1057553003/diff/40001/tools/telemetry/...
File tools/telemetry/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/1057553003/diff/40001/tools/telemetry/...
tools/telemetry/PRESUBMIT.py:26: telemetry_deps = {}
nit: Not needed.

https://chromiumcodereview.appspot.com/1057553003/diff/40001/tools/telemetry/...
tools/telemetry/PRESUBMIT.py:52: any(dep_path.startswith(d) for d in
telemetry_deps['directory_deps']))
Not quite right. Use path.IsSubpath()

[nednguyen, 2015-04-15 22:50:00]

PTAL again since I move the check to telemetry_dependencies_unittest

https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
File tools/telemetry/PRESUBMIT.py (right):

https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
tools/telemetry/PRESUBMIT.py:26: telemetry_deps = {}
On 2015/04/15 22:25:00, dtu wrote:
> nit: Not needed.

Done.

https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
tools/telemetry/PRESUBMIT.py:52: any(dep_path.startswith(d) for d in
telemetry_deps['directory_deps']))
On 2015/04/15 22:25:00, dtu wrote:
> Not quite right. Use path.IsSubpath()

Done.

[nednguyen, 2015-04-16 16:51:48]

On 2015/04/15 22:50:00, nednguyen wrote:
> PTAL again since I move the check to telemetry_dependencies_unittest
> 
>
https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
> File tools/telemetry/PRESUBMIT.py (right):
> 
>
https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
> tools/telemetry/PRESUBMIT.py:26: telemetry_deps = {}
> On 2015/04/15 22:25:00, dtu wrote:
> > nit: Not needed.
> 
> Done.
> 
>
https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
> tools/telemetry/PRESUBMIT.py:52: any(dep_path.startswith(d) for d in
> telemetry_deps['directory_deps']))
> On 2015/04/15 22:25:00, dtu wrote:
> > Not quite right. Use path.IsSubpath()
> 
> Done.

For some reason, a new dependency file
chromium/src/third_party/pexpect/pexpect.py is added since the start of this
patch. I think this is probably because some dependency of telemetry adds a new
dependency. I will need to update the test so that it ignore dependencies of
telemetry's direct dependencies.

[nednguyen, 2015-04-16 17:41:15]

On 2015/04/16 16:51:48, nednguyen wrote:
> On 2015/04/15 22:50:00, nednguyen wrote:
> > PTAL again since I move the check to telemetry_dependencies_unittest
> > 
> >
>
https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
> > File tools/telemetry/PRESUBMIT.py (right):
> > 
> >
>
https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
> > tools/telemetry/PRESUBMIT.py:26: telemetry_deps = {}
> > On 2015/04/15 22:25:00, dtu wrote:
> > > nit: Not needed.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1057553003/diff/40001/tools/telemetry/PRESUBM...
> > tools/telemetry/PRESUBMIT.py:52: any(dep_path.startswith(d) for d in
> > telemetry_deps['directory_deps']))
> > On 2015/04/15 22:25:00, dtu wrote:
> > > Not quite right. Use path.IsSubpath()
> > 
> > Done.
> 
> For some reason, a new dependency file
> chromium/src/third_party/pexpect/pexpect.py is added since the start of this
> patch. I think this is probably because some dependency of telemetry adds a
new
> dependency. I will need to update the test so that it ignore dependencies of
> telemetry's direct dependencies.

It's not simple to ignore the dependencies of telemetry's direct dependencies,
and I think we should also be worried about those cases as well. If too many
people complain to us that telemetry blocking them from landing patches, we can
modify the "directory_deps" in TELEMETRY_DEPS to "third_party/", which I suspect
will be the majority of cases.

[sullivan, 2015-04-16 18:46:11]

lgtm.

One nit before landing: this is now a unit test and not a presubmit, correct? If
so, please update the CL description before landing.

[nednguyen, 2015-04-16 18:59:10]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-04-16 18:59:11]

The patchset sent to the CQ was uploaded after l-g-t-m from aiolos@chromium.org
Link to the patchset: https://codereview.chromium.org/1057553003/#ps100001
(title: "Update comment in TELEMETRY_DEPS")

[commit-bot: I haz the power, 2015-04-16 19:00:10]

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

[commit-bot: I haz the power, 2015-04-16 19:10:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-16 19:11:38]

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