Do not actually remove trees when test bisect in dry-run mode

Do not actually remove trees when testing bisect in dry-run mode

R=qyearsley@chromium.org, ojan@chromium.org

Committed: https://crrev.com/188904367305bc761d7b52e1a6df348b7d723c7d
Cr-Commit-Position: refs/heads/master@{#296994}

[Sergiy Byelozyorov, 2014-09-19 12:35:16]

sergiyb@chromium.org changed reviewers:
+ ojan@chromium.org, qyearsley@chromium.org

[Sergiy Byelozyorov, 2014-09-19 12:35:59]

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:59: from telemetry.util import
cloud_storage # pylint: disable=F0401
This was necessary for PRESUBMIT to pass

[ojan, 2014-09-19 17:42:51]

ojan@chromium.org changed reviewers:
+ maruel@chromium.org

[ojan, 2014-09-19 17:42:52]

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:59: from telemetry.util import
cloud_storage # pylint: disable=F0401
On 2014/09/19 at 12:35:59, Sergiy Byelozyorov wrote:
> This was necessary for PRESUBMIT to pass

I don't think this is the best fix for this. It looks like
telemetry/telemetry/util is what this is importing. So the other script that
runs this file must have telemetry on it's python path or be run from the
telemetry directory. Ideally we'd fix all the things to do imports from the
root, but that's a big change. So, instead, I think you should change pylint to
put the root telemetry directory in sys.path.

I *think* you need to modify
https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py to make
that happen. I don't know for sure though. maruel, do you know this stuff?

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:252: shutil.rmtree = lambda
path, onerror: None
I typically do this in a try/finally so you can be sure shutil.rmtree gets reset
in case the test throws an error. That way this test won't affect the running of
other tests.

old_rmtree = shutil.rmtree
try:
  shutil.rmtree = lambda path, onerror: None
  # THE REST OF THE TEST CODE HERE
finally:
  shutil.rmtree = old_rmtree

[M-A Ruel, 2014-09-20 00:47:44]

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:59: from telemetry.util import
cloud_storage # pylint: disable=F0401
On 2014/09/19 17:42:52, ojan-only-code-yellow-reviews wrote:
> On 2014/09/19 at 12:35:59, Sergiy Byelozyorov wrote:
> > This was necessary for PRESUBMIT to pass
> 
> I don't think this is the best fix for this. It looks like
> telemetry/telemetry/util is what this is importing. So the other script that
> runs this file must have telemetry on it's python path or be run from the
> telemetry directory. Ideally we'd fix all the things to do imports from the
> root, but that's a big change. So, instead, I think you should change pylint
to
> put the root telemetry directory in sys.path.
> 
> I *think* you need to modify
> https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py to
make
> that happen. I don't know for sure though. maruel, do you know this stuff?

pylint is not run from src/PRESUBMIT.py because there's no project level
consistent sys.path setup. It's the one in tools/auto_bisect. There's a way to
pass additional paths to the check.

[Sergiy Byelozyorov, 2014-09-22 20:04:15]

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression.py:59: from telemetry.util import
cloud_storage # pylint: disable=F0401
On 2014/09/20 00:47:44, M-A Ruel wrote:
> On 2014/09/19 17:42:52, ojan-only-code-yellow-reviews wrote:
> > On 2014/09/19 at 12:35:59, Sergiy Byelozyorov wrote:
> > > This was necessary for PRESUBMIT to pass
> > 
> > I don't think this is the best fix for this. It looks like
> > telemetry/telemetry/util is what this is importing. So the other script that
> > runs this file must have telemetry on it's python path or be run from the
> > telemetry directory. Ideally we'd fix all the things to do imports from the
> > root, but that's a big change. So, instead, I think you should change pylint
> to
> > put the root telemetry directory in sys.path.
> > 
> > I *think* you need to modify
> > https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py to
> make
> > that happen. I don't know for sure though. maruel, do you know this stuff?
> 
> pylint is not run from src/PRESUBMIT.py because there's no project level
> consistent sys.path setup. It's the one in tools/auto_bisect. There's a way to
> pass additional paths to the check.

Done.

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/584853002/diff/1/tools/auto_bisect/bisect_per...
tools/auto_bisect/bisect_perf_regression_test.py:252: shutil.rmtree = lambda
path, onerror: None
On 2014/09/19 17:42:52, ojan-only-code-yellow-reviews wrote:
> I typically do this in a try/finally so you can be sure shutil.rmtree gets
reset
> in case the test throws an error. That way this test won't affect the running
of
> other tests.
> 
> old_rmtree = shutil.rmtree
> try:
>   shutil.rmtree = lambda path, onerror: None
>   # THE REST OF THE TEST CODE HERE
> finally:
>   shutil.rmtree = old_rmtree

Done.

[M-A Ruel, 2014-09-22 21:14:03]

lgtm

[qyearsley, 2014-09-22 22:11:08]

On 2014/09/22 21:14:03, M-A Ruel wrote:
> lgtm

lgtm as well.

[Sergiy Byelozyorov, 2014-09-23 15:19:46]

The CQ bit was checked by sergiyb@chromium.org

[Sergiy Byelozyorov, 2014-09-23 15:20:05]

The CQ bit was unchecked by sergiyb@chromium.org

[Sergiy Byelozyorov, 2014-09-23 18:37:09]

ojan, please lgtm if this looks ok now

[ojan, 2014-09-26 17:34:59]

lgtm

[Sergiy Byelozyorov, 2014-09-26 18:28:08]

The CQ bit was checked by sergiyb@chromium.org

[commit-bot: I haz the power, 2014-09-26 18:28:34]

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

[commit-bot: I haz the power, 2014-09-26 19:02:21]

Committed patchset #3 (id:40001) as 5e029f0162edae65040ee90b6826b267aff148aa

[commit-bot: I haz the power, 2014-09-26 19:02:53]

Patchset 3 (id:??) landed as
https://crrev.com/188904367305bc761d7b52e1a6df348b7d723c7d
Cr-Commit-Position: refs/heads/master@{#296994}