[Telemetry] Do not have benchmarks validate story name in expectations during runs.

[Telemetry] Do not have benchmarks validate story name in expectations during runs.

There is a CL out adding a presubmit test for checking names:
https://chromium-review.googlesource.com/c/513162/

This should be the approach we use for making sure names exist. The telemetry
smoke tests do some optimizations that make checking mid run not viable.
https://cs.chromium.org/chromium/src/tools/perf/benchmarks/benchmark_smoke_unittest.py?type=cs&q=SinglePageBenchmark&l=53

The fact that you can add layers that change what CreateStorySet() returns makes
it hard to determine mid run if a story name is valid. Checking during presubmit
allows us to sidestep the smoke tests being clever but still gives us assurance
that the story names in expectations exist.

BUG=chromium:713222
Review-Url: https://codereview.chromium.org/2903023002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/518df53454b5c951cc4804786f957ffb0639c8ec

[rnephew (Reviews Here), 2017-05-24 17:23:27]

rnephew@chromium.org changed reviewers:
+ charliea@chromium.org, nednguyen@google.com

[nednguyen, 2017-05-24 18:23:00]

On 2017/05/24 17:23:28, rnephew (Reviews Here) wrote:

Maybe just change this into a logging.error instead. Essentially, Telemetry is a
library can be used on it own separated from chromium's tools/perf.

[nednguyen, 2017-05-24 18:23:30]

lgtm so you can land after change it to log error

[rnephew (Reviews Here), 2017-05-24 18:44:15]

Patchset #2 (id:20001) has been deleted

[rnephew (Reviews Here), 2017-05-24 18:45:26]

The CQ bit was checked by rnephew@chromium.org

[rnephew (Reviews Here), 2017-05-24 18:45:27]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2903023002/#ps40001
(title: "[Telemetry] Do not have benchmarks validate story name in expectations
during runs.")

[commit-bot: I haz the power, 2017-05-24 18:45:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[charliea (OOO until 10-5), 2017-05-24 19:07:35]

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner.py:389:
benchmark.ValidateExpectations(stories, log_only=True)
The comment above isn't correct, is it?

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/story_runner_unittest.py (right):

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner_unittest.py:1229: self.assertEqual(rc,
0)
The comment above directly contradicts what we're asserting, doesn't it?

[charliea (OOO until 10-5), 2017-05-24 19:08:12]

The CQ bit was unchecked by charliea@chromium.org

[charliea (OOO until 10-5), 2017-05-24 19:08:43]

not lgtm right now

I don't think the comments match what the code is actually doing

[charliea (OOO until 10-5), 2017-05-24 19:11:31]

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/sto...
File telemetry/telemetry/story/expectations.py (right):

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:37: return all_pass
Why does this function return a value? It doesn't seem like the value is used
anywhere other than the unit test that asserts the interface

[rnephew (Reviews Here), 2017-05-24 19:59:24]

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner.py:389:
benchmark.ValidateExpectations(stories, log_only=True)
On 2017/05/24 19:07:35, charliea wrote:
> The comment above isn't correct, is it?

Done.

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/story_runner_unittest.py (right):

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/story_runner_unittest.py:1229: self.assertEqual(rc,
0)
On 2017/05/24 19:07:35, charliea wrote:
> The comment above directly contradicts what we're asserting, doesn't it?

Done.

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/sto...
File telemetry/telemetry/story/expectations.py (right):

https://codereview.chromium.org/2903023002/diff/40001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:37: return all_pass
On 2017/05/24 19:11:31, charliea wrote:
> Why does this function return a value? It doesn't seem like the value is used
> anywhere other than the unit test that asserts the interface

I refactored this to only log error messages. We can let the end user of this
decide what they want to do with the knowledge that there is bad story names.

[rnephew (Reviews Here), 2017-05-24 20:09:55]

Patchset #3 (id:60001) has been deleted

[charliea (OOO until 10-5), 2017-05-24 21:55:44]

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark.py:270: def ValidateExpectations(self, story_set):
I think the whole thing is a little hard to trace through because of the current
naming of these methods:

I'd expect "ValidateExpectations()" to be an imperative method that might cause
something to go boom when it's called (which makes sense, because that's exactly
what it did when it was written)

If the method is more functional (which it is, plus a little logging) and
operates via a return value, I'd expect the name of the method to be something
like "GetBrokenExpectations". As much as possible, we want to make sure to
clearly convey the interface of the method in the name of the method

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
File telemetry/telemetry/story/expectations.py (right):

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:27: def ValidateAgainstStorySet(self,
story_set):
Same with above: I think "Validate..." carries the connotation that something's
an imperative method, whereas this one is functional

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:28: stories = [s.display_name for s in
story_set.stories]
I might recommend calling this "story_names" rather than "stories": "stories"
sounds like it might be some group of objects, whereas story_names makes it
clear that we're just talking about a bunch of strings here

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:29: failed_stories = []
I still don't think this is super clear.

"failed_stories" is confusing, because when I think of a "failed story", I think
of a story that tried to run but wasn't successful. Instead, what this actually
is is a story that we haven't run yet, but has an invalid name. Similar to
above, I might recommend renaming this to be something like
"invalid_story_names" rather than "failed_stories" to make it clear that 1) it's
a list of strings and 2) these stories aren't failed runs, but rather names that
are invalid.

I'm also super confused by the line "expectation not in stories": wut? It looks
like self._expectations is keyed by story name, not expectation. Given that,
should this be "for story_name in self._expectations:"?

[charliea (OOO until 10-5), 2017-05-24 21:57:33]

lgtm

If you feel comfortable submitting this CL without me taking a look at it again,
I'm okay with you doing so

[rnephew (Reviews Here), 2017-05-24 22:11:56]

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/ben...
File telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/ben...
telemetry/telemetry/benchmark.py:270: def ValidateExpectations(self, story_set):
On 2017/05/24 21:55:44, charliea wrote:
> I think the whole thing is a little hard to trace through because of the
current
> naming of these methods:
> 
> I'd expect "ValidateExpectations()" to be an imperative method that might
cause
> something to go boom when it's called (which makes sense, because that's
exactly
> what it did when it was written)
> 
> If the method is more functional (which it is, plus a little logging) and
> operates via a return value, I'd expect the name of the method to be something
> like "GetBrokenExpectations". As much as possible, we want to make sure to
> clearly convey the interface of the method in the name of the method

Done.

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
File telemetry/telemetry/story/expectations.py (right):

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:27: def ValidateAgainstStorySet(self,
story_set):
On 2017/05/24 21:55:44, charliea wrote:
> Same with above: I think "Validate..." carries the connotation that
something's
> an imperative method, whereas this one is functional

Done.

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:28: stories = [s.display_name for s in
story_set.stories]
On 2017/05/24 21:55:44, charliea wrote:
> I might recommend calling this "story_names" rather than "stories": "stories"
> sounds like it might be some group of objects, whereas story_names makes it
> clear that we're just talking about a bunch of strings here

Done.

https://codereview.chromium.org/2903023002/diff/80001/telemetry/telemetry/sto...
telemetry/telemetry/story/expectations.py:29: failed_stories = []
On 2017/05/24 21:55:44, charliea wrote:
> I still don't think this is super clear.
> 
> "failed_stories" is confusing, because when I think of a "failed story", I
think
> of a story that tried to run but wasn't successful. Instead, what this
actually
> is is a story that we haven't run yet, but has an invalid name. Similar to
> above, I might recommend renaming this to be something like
> "invalid_story_names" rather than "failed_stories" to make it clear that 1)
it's
> a list of strings and 2) these stories aren't failed runs, but rather names
that
> are invalid.
> 
> I'm also super confused by the line "expectation not in stories": wut? It
looks
> like self._expectations is keyed by story name, not expectation. Given that,
> should this be "for story_name in self._expectations:"?
> 
Done-ish.
Your suggestion as is we would have the problem of story_name being too similar
to story_names. I switched it to story_set_story_names and story_name to help
mitigate that.

[rnephew (Reviews Here), 2017-05-24 22:13:27]

The CQ bit was checked by rnephew@chromium.org

[rnephew (Reviews Here), 2017-05-24 22:13:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, charliea@chromium.org
Link to the patchset: https://codereview.chromium.org/2903023002/#ps100001
(title: "[Telemetry] Do not have benchmarks validate story name in expectations
during runs.")

[commit-bot: I haz the power, 2017-05-24 22:13:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 22:37:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495664007165640,
"parent_rev": "7114b8194168729394c8439858212be5fd996705", "commit_rev":
"518df53454b5c951cc4804786f957ffb0639c8ec"}

[commit-bot: I haz the power, 2017-05-24 22:37:30]

Description was changed from

==========
[Telemetry] Do not have benchmarks validate story name in expectations during
runs.

There is a CL out adding a presubmit test for checking names:
https://chromium-review.googlesource.com/c/513162/

This should be the approach we use for making sure names exist. The telemetry
smoke tests do some optimizations that make checking mid run not viable.
https://cs.chromium.org/chromium/src/tools/perf/benchmarks/benchmark_smoke_un...

The fact that you can add layers that change what CreateStorySet() returns makes
it hard to determine mid run if a story name is valid. Checking during presubmit
allows us to sidestep the smoke tests being clever but still gives us assurance
that the story names in expectations exist.

BUG=chromium:713222
==========

to

==========
[Telemetry] Do not have benchmarks validate story name in expectations during
runs.

There is a CL out adding a presubmit test for checking names:
https://chromium-review.googlesource.com/c/513162/

This should be the approach we use for making sure names exist. The telemetry
smoke tests do some optimizations that make checking mid run not viable.
https://cs.chromium.org/chromium/src/tools/perf/benchmarks/benchmark_smoke_un...

The fact that you can add layers that change what CreateStorySet() returns makes
it hard to determine mid run if a story name is valid. Checking during presubmit
allows us to sidestep the smoke tests being clever but still gives us assurance
that the story names in expectations exist.

BUG=chromium:713222

Review-Url: https://codereview.chromium.org/2903023002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-05-24 22:37:31]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[nednguyen, 2017-06-02 11:39:18]

https://codereview.chromium.org/2903023002/diff/100001/telemetry/telemetry/in...
File telemetry/telemetry/internal/story_runner.py (right):

https://codereview.chromium.org/2903023002/diff/100001/telemetry/telemetry/in...
telemetry/telemetry/internal/story_runner.py:387:
benchmark.GetBrokenExpectations(stories)
Sorry but I stumble on this code again. This is quite awkward because we rely on
a getter to do logging. I suggest instead remove the logging, and change this
to:

invalid_stories_name = benchmark.GetBrokenExpectations(stories)
logging.error('These story names are invalid: %s, '
,'.join(invalid_stories_name))