Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2907643002:
Explicitly setting story names for the loading_mobile story set. (Closed)
|
Created:
3 years, 7 months ago by ashleymarie1 Modified:
3 years, 6 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExplicitly setting story names for the loading_mobile story set.
BUG=chromium:720002
BUG=chromium:728006
Review-Url: https://codereview.chromium.org/2907643002
Cr-Commit-Position: refs/heads/master@{#475998}
Committed: https://chromium.googlesource.com/chromium/src/+/aead6369dea3e206264040f71d66415af55d47b8
Patch Set 1 #Patch Set 2 : Explicitly setting story names for the loading_mobile story set. #
Total comments: 9
Patch Set 3 : Shortening lines #
Total comments: 2
Patch Set 4 : messing with indents #Patch Set 5 : Changing urls to titles #
Total comments: 6
Patch Set 6 : Explicitly setting story names for the loading_mobile story set. #Messages
Total messages: 30 (14 generated)
The CQ bit was checked by ashleymarie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Explicitly setting story names for the loading_mobile story set. BUG=chromium:720002 ========== to ========== Explicitly setting story names for the loading_mobile story set. BUG=chromium:720002 ==========
ashleymarie@chromium.org changed reviewers: + kouhei@chromium.org, ksakamoto@chromium.org, nednguyen@google.com
Hi Kouhei and Kunihiko, We're adding explicit names to stories so that we can easily reason about what name to use when disabling tests, sending test data to the dashboard, sending test data to the flakiness dashboard, and reading the output of the test in the logs. Could you please take a look at this cl which sets names for the loading_mobile benchmark? Once it is submitted, I'll kick off a migration as to not lose historical data. Thanks, Ashley
nednguyen@google.com changed reviewers: + rnephew@chromium.org
On 2017/05/25 19:11:26, ashleymarie1 wrote: > Hi Kouhei and Kunihiko, > We're adding explicit names to stories so that we can easily reason about what > name to use when disabling tests, sending test data to the dashboard, sending > test data to the flakiness dashboard, and reading the output of the test in the > logs. Could you please take a look at this cl which sets names for the > loading_mobile benchmark? Once it is submitted, I'll kick off a migration as to > not lose historical data. > Thanks, > Ashley lgtm as long as Randy & Tokyo folks are happy.
https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:72: ('https://www.flipkart.com/big-wing-casuals/p/itmemeageyfn6m9z?lid=LSTSHOEMEAGURG2PHPW18FTBN&pid=SHOEMEAGURG2PHPW', 'flipkart.com'), On these one with long lines, the name should probably be on the next line. I think the 'long line' exemption for urls does not cover anything past the url. https://google.github.io/styleguide/pyguide.html#Line_length https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:83: ('https://dev.opera.com/', 'dev.opera.com'), I know on loading_desktop for the most part we tried to give them names that weren't purely minified urls, but titles. Example: ('http://ynet.co.il/', 'Ynet'), ('http://farsnews.com/', 'FarsNews'), ('http://www.aljayyash.net/', 'Aljayyash'), ('http://haraj.com.sa', 'Haraj').... We may want to do that, but since this is purely a matter of opinions as long as loading-dev folks dont care, it good as is.
https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:72: ('https://www.flipkart.com/big-wing-casuals/p/itmemeageyfn6m9z?lid=LSTSHOEMEAGURG2PHPW18FTBN&pid=SHOEMEAGURG2PHPW', 'flipkart.com'), On 2017/05/25 19:22:58, rnephew (Reviews Here) wrote: > On these one with long lines, the name should probably be on the next line. I > think the 'long line' exemption for urls does not cover anything past the url. > > https://google.github.io/styleguide/pyguide.html#Line_length Makes sense, I'll go ahead and update those now https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:83: ('https://dev.opera.com/', 'dev.opera.com'), On 2017/05/25 19:22:58, rnephew (Reviews Here) wrote: > I know on loading_desktop for the most part we tried to give them names that > weren't purely minified urls, but titles. > Example: > ('http://ynet.co.il/', 'Ynet'), > ('http://farsnews.com/', 'FarsNews'), > ('http://www.aljayyash.net/', 'Aljayyash'), > ('http://haraj.com.sa', 'Haraj').... > > We may want to do that, but since this is purely a matter of opinions as long as > loading-dev folks dont care, it good as is. I'll wait for the loading-dev folks to chime in here before I change them all :)
https://codereview.chromium.org/2907643002/diff/40001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/40001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:33: 'google.com'), I think the indentation is off, I think it should be like this but I dont see an exact example of this in the style guide: self.AddStories(['global'], [ ('https://www.google.com/search?q=flower#q=flower+delivery', 'google.com'), Closest example I can find from style guide: x = ('This will build a very long long ' 'long long long long long long string')
https://codereview.chromium.org/2907643002/diff/40001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/40001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:33: 'google.com'), On 2017/05/25 20:10:33, rnephew (Reviews Here) wrote: > I think the indentation is off, I think it should be like this but I dont see > an exact example of this in the style guide: > self.AddStories(['global'], [ > ('https://www.google.com/search?q=flower#q=flower+delivery', > 'google.com'), > > Closest example I can find from style guide: > x = ('This will build a very long long ' > 'long long long long long long string') I thought it would be closer to if (width == 0 and height == 0 and color == 'red' and emphasis == 'strong'): since it's a continuation of a line, not a continuation of the same string. Although that would make it double indented instead of single so either way I messed it up haha
On 2017/05/25 20:19:17, ashleymarie1 wrote: > https://codereview.chromium.org/2907643002/diff/40001/tools/perf/page_sets/lo... > File tools/perf/page_sets/loading_mobile.py (right): > > https://codereview.chromium.org/2907643002/diff/40001/tools/perf/page_sets/lo... > tools/perf/page_sets/loading_mobile.py:33: 'google.com'), > On 2017/05/25 20:10:33, rnephew (Reviews Here) wrote: > > I think the indentation is off, I think it should be like this but I dont see > > an exact example of this in the style guide: > > self.AddStories(['global'], [ > > ('https://www.google.com/search?q=flower#q=flower+delivery', > > 'google.com'), > > > > Closest example I can find from style guide: > > x = ('This will build a very long long ' > > 'long long long long long long string') > > I thought it would be closer to > if (width == 0 and height == 0 and > color == 'red' and emphasis == 'strong'): > since it's a continuation of a line, not a continuation of the same string. > Although that would make it double indented instead of single so either way I > messed it up haha tho this basically confirms you're right: https://docs.python.org/3/reference/lexical_analysis.html#implicit-line-joining :) updated accordingly thanks for the learning opportunity! \o/
lgtm as long as KSakamoto@ and Kouhei@ are happy.
Thanks for doing this! https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:32: ('https://www.google.com/search?q=flower#q=flower+delivery', 'google.com'), This actually goes to google.co.jp SERP because I recorded this in Japan. (crbug.com/714814) Would you change the name to reflect this, something like 'google.com_redirected_to_google.co.jp'? https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:83: ('https://dev.opera.com/', 'dev.opera.com'), On 2017/05/25 19:43:42, ashleymarie1 wrote: > On 2017/05/25 19:22:58, rnephew (Reviews Here) wrote: > > I know on loading_desktop for the most part we tried to give them names that > > weren't purely minified urls, but titles. > > Example: > > ('http://ynet.co.il/', 'Ynet'), > > ('http://farsnews.com/', 'FarsNews'), > > ('http://www.aljayyash.net/', 'Aljayyash'), > > ('http://haraj.com.sa', 'Haraj').... > > > > We may want to do that, but since this is purely a matter of opinions as long > as > > loading-dev folks dont care, it good as is. > > I'll wait for the loading-dev folks to chime in here before I change them all :) Yeah, I think it's better to align with loading_desktop style. Could you change them to use titles, thanks!
Thanks! The plan sgtm. I'd like to delegate the detailed review to ksakamoto@
@ksakamoto, please take another look thanks! https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:32: ('https://www.google.com/search?q=flower#q=flower+delivery', 'google.com'), On 2017/05/26 01:41:59, Kunihiko Sakamoto wrote: > This actually goes to google.co.jp SERP because I recorded this in Japan. > (crbug.com/714814) > Would you change the name to reflect this, something like > 'google.com_redirected_to_google.co.jp'? Keeping with loading_desktop style, I changed it to GoogleRedirectToGoogleJapan Is there a better title for this page? https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:83: ('https://dev.opera.com/', 'dev.opera.com'), On 2017/05/26 01:41:59, Kunihiko Sakamoto wrote: > On 2017/05/25 19:43:42, ashleymarie1 wrote: > > On 2017/05/25 19:22:58, rnephew (Reviews Here) wrote: > > > I know on loading_desktop for the most part we tried to give them names that > > > weren't purely minified urls, but titles. > > > Example: > > > ('http://ynet.co.il/', 'Ynet'), > > > ('http://farsnews.com/', 'FarsNews'), > > > ('http://www.aljayyash.net/', 'Aljayyash'), > > > ('http://haraj.com.sa', 'Haraj').... > > > > > > We may want to do that, but since this is purely a matter of opinions as > long > > as > > > loading-dev folks dont care, it good as is. > > > > I'll wait for the loading-dev folks to chime in here before I change them all > :) > > Yeah, I think it's better to align with loading_desktop style. > Could you change them to use titles, thanks! Done! :)
Mostly looks good. https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/20001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:32: ('https://www.google.com/search?q=flower#q=flower+delivery', 'google.com'), On 2017/05/26 13:21:31, ashleymarie1 wrote: > On 2017/05/26 01:41:59, Kunihiko Sakamoto wrote: > > This actually goes to google.co.jp SERP because I recorded this in Japan. > > (crbug.com/714814) > > Would you change the name to reflect this, something like > > 'google.com_redirected_to_google.co.jp'? > > Keeping with loading_desktop style, I changed it to GoogleRedirectToGoogleJapan > Is there a better title for this page? sgtm, thanks! https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:44: ('https://www.google.co.id/#q=pengiriman+bunga', 'GoogleIndiaFlorist'), 'GoogleIndonesia'? https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:48: # ('G1'), URL is missing https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:68: ('http://xw.qq.com/news/20160803025029/NEW2016080302502901', 'xw.qq.com'), 'QQNews'?
Thanks for the catches! Please take another loook https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... File tools/perf/page_sets/loading_mobile.py (right): https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:44: ('https://www.google.co.id/#q=pengiriman+bunga', 'GoogleIndiaFlorist'), On 2017/05/30 04:25:11, Kunihiko Sakamoto wrote: > 'GoogleIndonesia'? Acknowledged. https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:48: # ('G1'), On 2017/05/30 04:25:11, Kunihiko Sakamoto wrote: > URL is missing Done. https://codereview.chromium.org/2907643002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/loading_mobile.py:68: ('http://xw.qq.com/news/20160803025029/NEW2016080302502901', 'xw.qq.com'), On 2017/05/30 04:25:11, Kunihiko Sakamoto wrote: > 'QQNews'? Acknowledged.
LGTM, thanks!
Description was changed from ========== Explicitly setting story names for the loading_mobile story set. BUG=chromium:720002 ========== to ========== Explicitly setting story names for the loading_mobile story set. BUG=chromium:720002 BUG=chromium:728006 ==========
The CQ bit was checked by ashleymarie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ashleymarie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, rnephew@chromium.org Link to the patchset: https://codereview.chromium.org/2907643002/#ps70001 (title: "Explicitly setting story names for the loading_mobile story set.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 70001, "attempt_start_ts": 1496259972616200,
"parent_rev": "7e7058a1cdf389f51184ce9f8a13c4afaf100eef", "commit_rev":
"aead6369dea3e206264040f71d66415af55d47b8"}
Message was sent while issue was closed.
Description was changed from ========== Explicitly setting story names for the loading_mobile story set. BUG=chromium:720002 BUG=chromium:728006 ========== to ========== Explicitly setting story names for the loading_mobile story set. BUG=chromium:720002 BUG=chromium:728006 Review-Url: https://codereview.chromium.org/2907643002 Cr-Commit-Position: refs/heads/master@{#475998} Committed: https://chromium.googlesource.com/chromium/src/+/aead6369dea3e206264040f71d66... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:70001) as https://chromium.googlesource.com/chromium/src/+/aead6369dea3e206264040f71d66... |
