Remove telemetry unittests usage of cloud_storage

Only download from cloud_storage when a bucket parameter is used in the user_story. Use system_stub instead of going over the network in user_story_runner_unittest.

BUG=447462
Committed: https://crrev.com/681ceae056cd763436eb17aff3d584e8839ada65
Cr-Commit-Position: refs/heads/master@{#315152}

[aiolos (Not reviewing), 2015-02-06 20:52:41]

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

[nednguyen, 2015-02-06 22:07:06]

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_runner.py:217: if
user_story_set.bucket and not _UpdateAndCheckArchives(
Can you remind me why we should only update & check archives when user story set
has a bucket defined?

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right):

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:495:
wpr_stub.Restore()
You need some try final. Otherwise these will not restore if any of the
assertion fails.

[aiolos (Not reviewing), 2015-02-06 22:44:54]

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_runner.py:217: if
user_story_set.bucket and not _UpdateAndCheckArchives(
On 2015/02/06 22:07:05, nednguyen wrote:
> Can you remind me why we should only update & check archives when user story
set
> has a bucket defined?

Because the bucket specifies which cloud_storage bucket to download the
user_story_set's files from. A user_story_set should not have files stored in
cloud_storage without also having a bucket specified. So if there is no bucket,
we shouldn't be attempting to download files. I'm also in the process of making
GetIfChanged require the bucket parameter in a separate CL. (So calling
GetIfChanged with bucket=None will shortly throw an error instead of searching
all three telemetry buckets.)

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_runner_unittest.py (right):

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_runner_unittest.py:495:
wpr_stub.Restore()
On 2015/02/06 22:07:05, nednguyen wrote:
> You need some try final. Otherwise these will not restore if any of the
> assertion fails.

Done.

[nednguyen, 2015-02-06 22:53:06]

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_runner.py:217: if
user_story_set.bucket and not _UpdateAndCheckArchives(
On 2015/02/06 22:44:54, aiolos wrote:
> On 2015/02/06 22:07:05, nednguyen wrote:
> > Can you remind me why we should only update & check archives when user story
> set
> > has a bucket defined?
> 
> Because the bucket specifies which cloud_storage bucket to download the
> user_story_set's files from. A user_story_set should not have files stored in
> cloud_storage without also having a bucket specified. So if there is no
bucket,
> we shouldn't be attempting to download files. I'm also in the process of
making
> GetIfChanged require the bucket parameter in a separate CL. (So calling
> GetIfChanged with bucket=None will shortly throw an error instead of searching
> all three telemetry buckets.)

I see. I was surprised to see that user_story_set.bucket not None is a
requirement for this to work, but _UpdateAndCheckArchives(..) function does not
have any bucket param.

However, I think you are making a redundant check because the outer loop already
require user_story_set to have bucket defined.

Can we merge _UpdateUserStoryArchivesIfChanged & _UpdateAndCheckArchives ? How
are they different?

[aiolos (Not reviewing), 2015-02-06 23:15:22]

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/903083004/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_runner.py:217: if
user_story_set.bucket and not _UpdateAndCheckArchives(
On 2015/02/06 22:53:06, nednguyen wrote:
> On 2015/02/06 22:44:54, aiolos wrote:
> > On 2015/02/06 22:07:05, nednguyen wrote:
> > > Can you remind me why we should only update & check archives when user
story
> > set
> > > has a bucket defined?
> > 
> > Because the bucket specifies which cloud_storage bucket to download the
> > user_story_set's files from. A user_story_set should not have files stored
in
> > cloud_storage without also having a bucket specified. So if there is no
> bucket,
> > we shouldn't be attempting to download files. I'm also in the process of
> making
> > GetIfChanged require the bucket parameter in a separate CL. (So calling
> > GetIfChanged with bucket=None will shortly throw an error instead of
searching
> > all three telemetry buckets.)
> 
> I see. I was surprised to see that user_story_set.bucket not None is a
> requirement for this to work, but _UpdateAndCheckArchives(..) function does
not
> have any bucket param.

That will be changing in the CL that makes the bucket parameter required for
GetIfChanged.
> 
> However, I think you are making a redundant check because the outer loop
already
> require user_story_set to have bucket defined.

Done.
> 
> Can we merge _UpdateUserStoryArchivesIfChanged & _UpdateAndCheckArchives ? How
> are they different?

The second is misnamed, and doesn't do anything with archives. It's pulling
things for the serving_dirs. See this review for the work being done on it:
https://codereview.chromium.org/838253005/

I haven't fixed the unittests for that CL yet. It's on my list. But getting the
unittests to stop hitting the network was a higher priority since it causes
anyone trying to run the telemetry unittests (such as perf sheriffs trying to
track down issues) to have to configure gsutil when it should never have been a
requirement.

[nednguyen, 2015-02-06 23:38:48]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-02-06 23:38:49]

lgtm

[commit-bot: I haz the power, 2015-02-06 23:39:22]

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

[commit-bot: I haz the power, 2015-02-07 00:52:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-07 00:53:08]

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