Modify list_system_health_stories to generate_system_health_csv

Modify list_system_health_stories to generate_system_health_csv

This CL also check in system_health_stories.csv file, which will be used to later
replace https://bit.ly/csh-stories

BUG=723636
Review-Url: https://codereview.chromium.org/2888133002
Cr-Commit-Position: refs/heads/master@{#473213}
Committed: https://chromium.googlesource.com/chromium/src/+/b087b7731c2efa0c446d4b52ea0839487cce4e1d

[nednguyen, 2017-05-17 13:27:14]

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

[nednguyen, 2017-05-17 13:32:42]

Description was changed from

==========
Modify list_system_health_stories to generate_system_health_csv

This CL also check in system_health_stories.csv file, which will be used to
later
replace https://bit.ly/csh-stories
==========

to

==========
Modify list_system_health_stories to generate_system_health_csv

This CL also check in system_health_stories.csv file, which will be used to
later
replace https://bit.ly/csh-stories

BUG=723636
==========

[nednguyen(REVIEW IN OTHER ACC), 2017-05-18 09:30:36]

On 2017/05/17 13:27:15, nednguyen wrote:

Ping

[perezju, 2017-05-18 09:52:35]

https://codereview.chromium.org/2888133002/diff/1/tools/perf/generate_system_...
File tools/perf/generate_system_health_csv (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/generate_system_...
tools/perf/generate_system_health_csv:24: 'system_health_stories.csv')
Do we really want to check in the output file to the repo? What is the value we
would get from that, as opposed to just generating the file (maybe
programmatically) as needed?

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
File tools/perf/page_sets/system_health/loading_stories.py (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/loading_stories.py:17: """
nit: no need to push quotes down if title fits in one line.

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/system_health_story.py:92: return cls.__doc__
maybe just return the first line (title) of the doc string?

https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
File tools/perf/system_health_stories.csv (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
tools/perf/system_health_stories.csv:20: browse:media:facebook_photos,mobile,N/A
We should also provide some default description for browse stories, like: "Load
{url} and navigate to some items/articles."

https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
tools/perf/system_health_stories.csv:91: load:games:bubbles,all,Load
https://games.cdn.famobi.com/html5games/s/smarty-bubbles/v010/?fg_domain=play...
These don't look particularly helpful.

We should either, provide some better default description, e.g.:

  "Load from {url_domain} a representative
page.".format(url_domain='games.cdn.famobi.com')

Or provide some manual descriptions, e.g.

  """Load "smarty bubbles" game on famobi.com."""

[nednguyen, 2017-05-18 14:02:58]

https://codereview.chromium.org/2888133002/diff/1/tools/perf/generate_system_...
File tools/perf/generate_system_health_csv (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/generate_system_...
tools/perf/generate_system_health_csv:24: 'system_health_stories.csv')
On 2017/05/18 09:52:35, perezju wrote:
> Do we really want to check in the output file to the repo? What is the value
we
> would get from that, as opposed to just generating the file (maybe
> programmatically) as needed?

We can then do IMPORTDATA("https://cs.chromium.org/...") in google spreadsheet
to have this loaded in the sheet.

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
File tools/perf/page_sets/system_health/loading_stories.py (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/loading_stories.py:17: """
On 2017/05/18 09:52:35, perezju wrote:
> nit: no need to push quotes down if title fits in one line.

Done.

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/system_health_story.py:92: return cls.__doc__
On 2017/05/18 09:52:35, perezju wrote:
> maybe just return the first line (title) of the doc string?

For cases like
https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc...,
I think it's valuable for people to know exactly what the story does.

https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
File tools/perf/system_health_stories.csv (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
tools/perf/system_health_stories.csv:20: browse:media:facebook_photos,mobile,N/A
On 2017/05/18 09:52:35, perezju wrote:
> We should also provide some default description for browse stories, like:
"Load
> {url} and navigate to some items/articles."

Done.

https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
tools/perf/system_health_stories.csv:91: load:games:bubbles,all,Load
https://games.cdn.famobi.com/html5games/s/smarty-bubbles/v010/?fg_domain=play...
On 2017/05/18 09:52:35, perezju wrote:
> These don't look particularly helpful.
> 
> We should either, provide some better default description, e.g.:
> 
>   "Load from {url_domain} a representative
> page.".format(url_domain='games.cdn.famobi.com')
> 
> Or provide some manual descriptions, e.g.
> 
>   """Load "smarty bubbles" game on famobi.com."""

I do the the manual description.

[nednguyen, 2017-05-18 14:03:03]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 14:03:32]

Dry run: CQ is trying da patch.

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

[nednguyen, 2017-05-18 14:03:55]

On 2017/05/18 14:02:58, nednguyen wrote:
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/generate_system_...
> File tools/perf/generate_system_health_csv (right):
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/generate_system_...
> tools/perf/generate_system_health_csv:24: 'system_health_stories.csv')
> On 2017/05/18 09:52:35, perezju wrote:
> > Do we really want to check in the output file to the repo? What is the value
> we
> > would get from that, as opposed to just generating the file (maybe
> > programmatically) as needed?
> 
> We can then do IMPORTDATA("https://cs.chromium.org/...") in google spreadsheet
> to have this loaded in the sheet.
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
> File tools/perf/page_sets/system_health/loading_stories.py (right):
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
> tools/perf/page_sets/system_health/loading_stories.py:17: """
> On 2017/05/18 09:52:35, perezju wrote:
> > nit: no need to push quotes down if title fits in one line.
> 
> Done.
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
> File tools/perf/page_sets/system_health/system_health_story.py (right):
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
> tools/perf/page_sets/system_health/system_health_story.py:92: return
cls.__doc__
> On 2017/05/18 09:52:35, perezju wrote:
> > maybe just return the first line (title) of the doc string?
> 
> For cases like
>
https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc...,
> I think it's valuable for people to know exactly what the story does.
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
> File tools/perf/system_health_stories.csv (right):
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
> tools/perf/system_health_stories.csv:20:
browse:media:facebook_photos,mobile,N/A
> On 2017/05/18 09:52:35, perezju wrote:
> > We should also provide some default description for browse stories, like:
> "Load
> > {url} and navigate to some items/articles."
> 
> Done.
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/system_health_st...
> tools/perf/system_health_stories.csv:91: load:games:bubbles,all,Load
>
https://games.cdn.famobi.com/html5games/s/smarty-bubbles/v010/?fg_domain=play...
> On 2017/05/18 09:52:35, perezju wrote:
> > These don't look particularly helpful.
> > 
> > We should either, provide some better default description, e.g.:
> > 
> >   "Load from {url_domain} a representative
> > page.".format(url_domain='games.cdn.famobi.com')
> > 
> > Or provide some manual descriptions, e.g.
> > 
> >   """Load "smarty bubbles" game on famobi.com."""
> 
> I do the the manual description.

Now all the system health stories have description. I would want to add check to
make sure that, but that can be done in a subsequent CL since this is already a
bit big.

[nednguyen, 2017-05-18 14:05:05]

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/generate_sys...
File tools/perf/generate_system_health_csv (right):

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/generate_sys...
tools/perf/generate_system_health_csv:40: csv_writer.writerow([s.name, p,
s.GetStoryDescription()])
Will add: whether the story is disable (& which platform) + tag in the
subsequent CL

[nednguyen, 2017-05-18 14:26:53]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 14:27:04]

Dry run: 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-18 15:40:04]

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

[commit-bot: I haz the power, 2017-05-18 15:40:05]

Dry run: This issue passed the CQ dry run.

[nednguyen, 2017-05-19 11:50:01]

On 2017/05/18 14:05:05, nednguyen wrote:
>
https://codereview.chromium.org/2888133002/diff/20001/tools/perf/generate_sys...
> File tools/perf/generate_system_health_csv (right):
> 
>
https://codereview.chromium.org/2888133002/diff/20001/tools/perf/generate_sys...
> tools/perf/generate_system_health_csv:40: csv_writer.writerow([s.name, p,
> s.GetStoryDescription()])
> Will add: whether the story is disable (& which platform) + tag in the
> subsequent CL

Ping

[perezju, 2017-05-19 13:48:29]

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
File tools/perf/page_sets/system_health/system_health_story.py (right):

https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/system_health_story.py:92: return cls.__doc__
On 2017/05/18 14:02:58, nednguyen wrote:
> On 2017/05/18 09:52:35, perezju wrote:
> > maybe just return the first line (title) of the doc string?
> 
> For cases like
>
https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc...,
> I think it's valuable for people to know exactly what the story does.

I think it's valuable to have a long full explanation next to the story
definition when doing a code search. But for the csv, I still think a short one
liner should be better.

This also forces us to come up with good short descriptions
for the stories, which could be surfaced in other places (e.g notes for leads?)

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/background_stories.py (right):

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/background_stories.py:29: return 'Load %s,
then put the Chrome browser into the background.' % cls.URL
nit: "put the Chrome browser" --> "put the browser"

(the browser could be a WebView shell)

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/searching_stories.py (right):

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/searching_stories.py:84: For each of the
search queries in in the list below, this story does:
nit: "in in" -> "in"

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/searching_stories.py:94: 'how to tie a tie'
I'm not very happy about duplicating this list here. Could be easy to forget
updating if at some point we change the other list below.

[perezju, 2017-05-19 14:51:42]

On 2017/05/19 13:48:29, perezju wrote:
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
> File tools/perf/page_sets/system_health/system_health_story.py (right):
> 
>
https://codereview.chromium.org/2888133002/diff/1/tools/perf/page_sets/system...
> tools/perf/page_sets/system_health/system_health_story.py:92: return
cls.__doc__
> On 2017/05/18 14:02:58, nednguyen wrote:
> > On 2017/05/18 09:52:35, perezju wrote:
> > > maybe just return the first line (title) of the doc string?
> > 
> > For cases like
> >
>
https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/searc...,
> > I think it's valuable for people to know exactly what the story does.
> 
> I think it's valuable to have a long full explanation next to the story
> definition when doing a code search. But for the csv, I still think a short
one
> liner should be better.
> 
> This also forces us to come up with good short descriptions
> for the stories, which could be surfaced in other places (e.g notes for
leads?)
> 
>
https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
> File tools/perf/page_sets/system_health/background_stories.py (right):
> 
>
https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
> tools/perf/page_sets/system_health/background_stories.py:29: return 'Load %s,
> then put the Chrome browser into the background.' % cls.URL
> nit: "put the Chrome browser" --> "put the browser"
> 
> (the browser could be a WebView shell)
> 
>
https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
> File tools/perf/page_sets/system_health/searching_stories.py (right):
> 
>
https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
> tools/perf/page_sets/system_health/searching_stories.py:84: For each of the
> search queries in in the list below, this story does:
> nit: "in in" -> "in"
> 
>
https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
> tools/perf/page_sets/system_health/searching_stories.py:94: 'how to tie a tie'
> I'm not very happy about duplicating this list here. Could be easy to forget
> updating if at some point we change the other list below.

lgtm

Talked to Ned online. We can land this to unblock the csv generation, and try to
improve story descriptions on a follow up.

[nednguyen, 2017-05-19 15:07:50]

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/background_stories.py (right):

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/background_stories.py:29: return 'Load %s,
then put the Chrome browser into the background.' % cls.URL
On 2017/05/19 13:48:29, perezju wrote:
> nit: "put the Chrome browser" --> "put the browser"
> 
> (the browser could be a WebView shell)

Done.

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/searching_stories.py (right):

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/searching_stories.py:84: For each of the
search queries in in the list below, this story does:
On 2017/05/19 13:48:29, perezju wrote:
> nit: "in in" -> "in"

Done.

https://codereview.chromium.org/2888133002/diff/20001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/searching_stories.py:94: 'how to tie a tie'
On 2017/05/19 13:48:29, perezju wrote:
> I'm not very happy about duplicating this list here. Could be easy to forget
> updating if at some point we change the other list below.

I removed the duplicating list for now.

[nednguyen, 2017-05-19 15:07:53]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2017-05-19 15:07:53]

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

[commit-bot: I haz the power, 2017-05-19 15:08:14]

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-19 16:52:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495206473627440,
"parent_rev": "e309521b316083148b2b2073fb589a086e010bc7", "commit_rev":
"b087b7731c2efa0c446d4b52ea0839487cce4e1d"}

[commit-bot: I haz the power, 2017-05-19 16:52:43]

Description was changed from

==========
Modify list_system_health_stories to generate_system_health_csv

This CL also check in system_health_stories.csv file, which will be used to
later
replace https://bit.ly/csh-stories

BUG=723636
==========

to

==========
Modify list_system_health_stories to generate_system_health_csv

This CL also check in system_health_stories.csv file, which will be used to
later
replace https://bit.ly/csh-stories

BUG=723636

Review-Url: https://codereview.chromium.org/2888133002
Cr-Commit-Position: refs/heads/master@{#473213}
Committed:
https://chromium.googlesource.com/chromium/src/+/b087b7731c2efa0c446d4b52ea08...
==========

[commit-bot: I haz the power, 2017-05-19 16:52:46]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b087b7731c2efa0c446d4b52ea08...