Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2334233002:
[System Health] Create first System Health media user stories. (Closed)
|
Created:
4 years, 3 months ago by rnephew (Reviews Here) Modified:
4 years, 2 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[System Health] Create first System Health media user stories.
Audio cases for Pandora, Soundcloud, and Google Play Music.
BUG=646392
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
Committed: https://crrev.com/536ae480e73750dfaab944f54e0f9a19a78c712f
Cr-Commit-Position: refs/heads/master@{#421533}
Patch Set 1 #
Total comments: 35
Patch Set 2 : Petrs Review #Patch Set 3 : Add Pandora #
Total comments: 9
Patch Set 4 : charlie and neds reviews #
Total comments: 32
Patch Set 5 : [System Health] Create first System Health media user stories. #Patch Set 6 : [System Health] Create first System Health media user stories. #
Total comments: 18
Patch Set 7 : [System Health] Create first System Health media user stories. #
Total comments: 6
Patch Set 8 : [System Health] Create first System Health media user stories. #Patch Set 9 : disable on win #
Messages
Total messages: 64 (24 generated)
Description was changed from ========== [System Health] Create first System Health media user stories. Audio cases for soundcloud and google play music. BUG=646392 ========== to ========== [System Health] Create first System Health media user stories. Audio cases for soundcloud and google play music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
rnephew@chromium.org changed reviewers: + petrcermak@chromium.org
Looks good overall. I've left a couple of comments inline. Description nit: s/soundcloud and google play music/Soundcloud and Google Play Music/ My main concern is that we're now using "media" in multiple different components of the name. Example: browse:MEDIA:youtube vs. MEDIA:audio:soundcloud This is undesirable as it makes the story space more difficult to understand. Perhaps we should use "media" as the first component and change browse:MEDIA:* to browse:AUDIO:* and browse:VIDEO:*. On the other hand, as explained in https://docs.google.com/document/d/1pEeCnkbtrbsK3uuPA-ftbg4kzM4Bk7a2A9rhRYklm..., the first component should be a verb. Perhaps play:media:google_play_music and play:media:soundcloud would make more sense? Thanks, Petr https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:58: class BattOrSystemHealthMediaDesktop(_BattOrBenchmark): nit: can you add a docstring below here? https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:59: def CreateStorySet(self, options): nit: add blank line above this one https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:61: @classmethod nit: add blank line above this one https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:14: SEARCH_IDENTIFIER = NotImplemented nit: please group the IDENTIFIER fields together https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:15: SEARCH_QUERY = NotImplemented This is actually used only by one subclass. I suggest you remove it from here. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:21: self._SearchMedia(action_runner, self.SEARCH_IDENTIFIER, Can you please add a _SearchMedia method that throws a NotImplemented Error? https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:27: def _Login(self, action_runner): no need for this method (it's empty by default) https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:30: def _PlayMedia(self, action_runner, element_function): Both _PlayMedia and _StopMedia do exactly the same. I think they should be merged into _WaitForAndClickElement. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:39: class _MediaDesktopStory(_MediaStory): This seems to add an unnecessary level of indirection. I'd remove this class and set SUPPORTED_PLATFORMS in both subclasses. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:45: # Audio Stories nit: lowercase "s" in "stories" and add a period at the end https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:51: URL = 'https:/music.google.com' you're missing a slash in the url https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:54: 'document.getElementsByClassName("title fade-out tooltip ")[0]') remove unnecessary space from the end of the string (after "tooltip") https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:54: 'document.getElementsByClassName("title fade-out tooltip ")[0]') the following might be simpler: 'document.querySelector(".title.fade-out.tooltip")' (note that there's no need for '[0]') https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:66: action_runner.WaitForElement(element_function=element_function) you can reuse the WaitForAndClickElement I suggested above https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:69: navigate_element = ('document.getElementsByClassName("description tooltip ' this can be a class constant https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:80: SEARCH_QUERY = 'SSmooth Jazz' # First S for some reason gets ommited. Even if we wait longer? https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:89: action_runner.Wait(1) # Without wait it clicks too soon. Don't you mean "starts entering text" rather than "clicks"? Any chance we could use something like action_runner.WaitForJavaScriptCondition instead?
Description was changed from ========== [System Health] Create first System Health media user stories. Audio cases for soundcloud and google play music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [System Health] Create first System Health media user stories. Audio cases for Soundcloud and Google Play Music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:58: class BattOrSystemHealthMediaDesktop(_BattOrBenchmark): On 2016/09/13 16:49:03, petrcermak wrote: > nit: can you add a docstring below here? Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:59: def CreateStorySet(self, options): On 2016/09/13 16:49:03, petrcermak wrote: > nit: add blank line above this one Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:61: @classmethod On 2016/09/13 16:49:03, petrcermak wrote: > nit: add blank line above this one Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:14: SEARCH_IDENTIFIER = NotImplemented On 2016/09/13 16:49:03, petrcermak wrote: > nit: please group the IDENTIFIER fields together Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:15: SEARCH_QUERY = NotImplemented On 2016/09/13 16:49:03, petrcermak wrote: > This is actually used only by one subclass. I suggest you remove it from here. There will eventually be a MediaMobileStory as well. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:21: self._SearchMedia(action_runner, self.SEARCH_IDENTIFIER, On 2016/09/13 16:49:03, petrcermak wrote: > Can you please add a _SearchMedia method that throws a NotImplemented Error? Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:27: def _Login(self, action_runner): On 2016/09/13 16:49:03, petrcermak wrote: > no need for this method (it's empty by default) Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:30: def _PlayMedia(self, action_runner, element_function): On 2016/09/13 16:49:03, petrcermak wrote: > Both _PlayMedia and _StopMedia do exactly the same. I think they should be > merged into _WaitForAndClickElement. Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:39: class _MediaDesktopStory(_MediaStory): On 2016/09/13 16:49:03, petrcermak wrote: > This seems to add an unnecessary level of indirection. I'd remove this class and > set SUPPORTED_PLATFORMS in both subclasses. This is setting up for when I add MediaMobileStory once I get the mobile versions done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:45: # Audio Stories On 2016/09/13 16:49:03, petrcermak wrote: > nit: lowercase "s" in "stories" and add a period at the end Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:51: URL = 'https:/music.google.com' On 2016/09/13 16:49:03, petrcermak wrote: > you're missing a slash in the url Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:54: 'document.getElementsByClassName("title fade-out tooltip ")[0]') On 2016/09/13 16:49:03, petrcermak wrote: > the following might be simpler: > 'document.querySelector(".title.fade-out.tooltip")' (note that there's no need > for '[0]') Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:66: action_runner.WaitForElement(element_function=element_function) On 2016/09/13 16:49:03, petrcermak wrote: > you can reuse the WaitForAndClickElement I suggested above Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:69: navigate_element = ('document.getElementsByClassName("description tooltip ' On 2016/09/13 16:49:03, petrcermak wrote: > this can be a class constant Done. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:80: SEARCH_QUERY = 'SSmooth Jazz' # First S for some reason gets ommited. On 2016/09/13 16:49:03, petrcermak wrote: > Even if we wait longer? Yeah. I upped the wait time to 2 seconds and it still doesn't work as expected. https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:89: action_runner.Wait(1) # Without wait it clicks too soon. On 2016/09/13 16:49:03, petrcermak wrote: > Don't you mean "starts entering text" rather than "clicks"? Any chance we could > use something like action_runner.WaitForJavaScriptCondition instead? Yes, clicking is not the right word there. I'll work on switching it to WaitForJavaScriptCondition, but my js-fu is pretty weak.
Description was changed from ========== [System Health] Create first System Health media user stories. Audio cases for Soundcloud and Google Play Music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [System Health] Create first System Health media user stories. Audio cases for Pandora, Soundcloud, and Google Play Music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Added a Pandora user story as well. I will limit my patch to just these 3 (I had pandora ready except some changes to the credentials file that I just figured out with Neds help).
https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/pandora_login.py (right): https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/pandora_login.py:27: nit: probably okay to delete the second blank line https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:7: nit: extra blank line? https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:88: action_runner.Wait(1) # Without wait it enters text too soon. what is "it" in this context?
(overall, looks like great work. thanks randy!)
looks great to me overall. I will patch your CL & run it on my local machine https://codereview.chromium.org/2334233002/diff/60001/tools/perf/benchmarks/b... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2334233002/diff/60001/tools/perf/benchmarks/b... tools/perf/benchmarks/battor.py:58: class BattOrSystemHealthMediaDesktop(_BattOrBenchmark): I think we don't need this since it's covered by system_health.common_dekstop https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:88: action_runner.Wait(1) # Without wait it enters text too soon. On 2016/09/13 19:38:40, charliea wrote: > what is "it" in this context? Yeah. Probably just document that "add 1 seconds wait to make the the browsing section more realistic"
https://codereview.chromium.org/2334233002/diff/60001/tools/perf/benchmarks/b... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2334233002/diff/60001/tools/perf/benchmarks/b... tools/perf/benchmarks/battor.py:58: class BattOrSystemHealthMediaDesktop(_BattOrBenchmark): On 2016/09/13 19:41:17, nednguyen wrote: > I think we don't need this since it's covered by system_health.common_dekstop Done. https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/pandora_login.py (right): https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/pandora_login.py:27: On 2016/09/13 19:38:40, charliea wrote: > nit: probably okay to delete the second blank line Done. https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:7: On 2016/09/13 19:38:40, charliea wrote: > nit: extra blank line? Done. https://codereview.chromium.org/2334233002/diff/60001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:88: action_runner.Wait(1) # Without wait it enters text too soon. On 2016/09/13 19:41:17, nednguyen wrote: > On 2016/09/13 19:38:40, charliea wrote: > > what is "it" in this context? > > Yeah. Probably just document that "add 1 seconds wait to make the the browsing > section more realistic" Done.
It's starting to look pretty good to me ;-) https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:39: class _MediaDesktopStory(_MediaStory): On 2016/09/13 18:52:40, rnephew (Reviews Here) wrote: > On 2016/09/13 16:49:03, petrcermak wrote: > > This seems to add an unnecessary level of indirection. I'd remove this class > and > > set SUPPORTED_PLATFORMS in both subclasses. > > This is setting up for when I add MediaMobileStory once I get the mobile > versions done. This is already the case in other SH stories files, but they don't define the intermediate *Desktop* and *Mobile* abstract classes, e.g. https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows.... You can always re-introduce this class if you ever need to (because of some shared platform-specific code), but right now, it just adds complexity. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/pandora_login.py (right): https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/pandora_login.py:33: login_button_function = ('document.getElementsByClassName(' I would personally just inline this into the action_runner.ClickElement call (feel free to ignore this comment). https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/pandora_login.py:35: '__form__button--blue loginButton")[0]') nit: could you put the last "onboarding" on this line so that the Python line break happens at the space in the JS string? https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:19: SEARCH_QUERY = NotImplemented I don't think there's any value in having this abstract class constant. Since you just pass it to an abstract method, you might as well just remove the parameter from the abstract method. This just adds code complexity without reducing code duplication. The same applies to SEARCH_IDENTIFIER. You could just define a _SearchMedia(action_runner) abstract method and let the subclasses do the job. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:32: def _SearchMedia(self, action_runner, element_function, query): What does "element_function" refer to? The input box? The button to be pressed after typing the query? Please make the variable name more descriptive (e.g. input_element_function or button_element_function). https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:44: def _SearchMedia(self, action_runner, element_function, query): no need to re-define the method https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:49: # Audio stories nit: missing period (for consistency with other *_stories.py files) https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:70: # Clicks on Today's top hits. (SEARCH_IDENTIFIER) This is a proof that passing SEARCH_IDENTIFIER through an argument doesn't make sense (because you already know that it will be passed). https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:72: # Clicks on playlist. (NAVIGATE_IDENTIFIER) nit: no need for "(NAVIGATE_IDENTIFIER)" here. The next line tells the story ;-) (the same applies to line 71) https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:94: class PandoraDesktopStory(_MediaDesktopStory): Note that at this point, this class could just subclass directly from system_health_story.SystemHealthStory because it doesn't use any of the _MediaStory code. To address this, I suggest that you update _MediaStory.RunPageInteractions to do the following (same for STOP_IDENTIFIER): ... if self.PLAY_IDENFIER: self._WaitForAndClickElement(action_runner, self.PLAY_IDENTIFIER) ... If you do this, there will be no need to override RunPageInteractions at all (which will render my next comment irrelevant). https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:110: self._SearchMedia(action_runner, self.SEARCH_IDENTIFIER, Why do you do this? You're just calling an empty function.
lgtm Nice work Randy. I already found the page Pandora's carousel animation quite janky. It would be a nice addon to the system health stories family. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:20: PLAY_DURATION = 10 Can we change this to 20s?
https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:73: self._WaitForAndClickElement(action_runner, self.NAVIGATE_IDENTIFIER) The timing of google play is in "<span id="time-container-current" aria-label="Current track time: 19 seconds">0:19</span>" https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:112: action_runner.Wait(self.PLAY_DURATION) The elapse time of the player is reflected through "<div class="elapsedTime">0:07</div> ". Can you modify the wait condition to check for this to be at least PLAY_DURATION?
lgtm, but defer to Peter for the ultimate approval https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:17: PLAY_IDENTIFIER = NotImplemented I think "element" might be more accurate than "identifier" https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:19: SEARCH_QUERY = NotImplemented On 2016/09/14 at 16:42:57, petrcermak wrote: > I don't think there's any value in having this abstract class constant. Since you just pass it to an abstract method, you might as well just remove the parameter from the abstract method. This just adds code complexity without reducing code duplication. The same applies to SEARCH_IDENTIFIER. You could just define a _SearchMedia(action_runner) abstract method and let the subclasses do the job. +1 https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:32: def _SearchMedia(self, action_runner, element_function, query): I think _NavigateToMedia might be a more accurate name, given how it's used.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/lo... File tools/perf/page_sets/login_helpers/pandora_login.py (right): https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/pandora_login.py:33: login_button_function = ('document.getElementsByClassName(' On 2016/09/14 16:42:56, petrcermak wrote: > I would personally just inline this into the action_runner.ClickElement call > (feel free to ignore this comment). I like naming it. It makes it easier to understand what is going on than trying to parse meaning from the function itself. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/pandora_login.py:35: '__form__button--blue loginButton")[0]') On 2016/09/14 16:42:56, petrcermak wrote: > nit: could you put the last "onboarding" on this line so that the Python line > break happens at the space in the JS string? Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/lo... tools/perf/page_sets/login_helpers/pandora_login.py:35: '__form__button--blue loginButton")[0]') On 2016/09/14 16:42:56, petrcermak wrote: > nit: could you put the last "onboarding" on this line so that the Python line > break happens at the space in the JS string? Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:17: PLAY_IDENTIFIER = NotImplemented On 2016/09/14 23:55:03, charliea wrote: > I think "element" might be more accurate than "identifier" Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:19: SEARCH_QUERY = NotImplemented On 2016/09/14 16:42:57, petrcermak wrote: > I don't think there's any value in having this abstract class constant. Since > you just pass it to an abstract method, you might as well just remove the > parameter from the abstract method. This just adds code complexity without > reducing code duplication. The same applies to SEARCH_IDENTIFIER. You could just > define a _SearchMedia(action_runner) abstract method and let the subclasses do > the job. Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:20: PLAY_DURATION = 10 On 2016/09/14 16:57:41, nednguyen wrote: > Can we change this to 20s? Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:32: def _SearchMedia(self, action_runner, element_function, query): On 2016/09/14 23:55:03, charliea wrote: > I think _NavigateToMedia might be a more accurate name, given how it's used. Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:32: def _SearchMedia(self, action_runner, element_function, query): On 2016/09/14 16:42:57, petrcermak wrote: > What does "element_function" refer to? The input box? The button to be pressed > after typing the query? Please make the variable name more descriptive (e.g. > input_element_function or button_element_function). Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:44: def _SearchMedia(self, action_runner, element_function, query): On 2016/09/14 16:42:57, petrcermak wrote: > no need to re-define the method Pylint disagrees. It complains that an abstract method isn't being defined here. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:49: # Audio stories On 2016/09/14 16:42:57, petrcermak wrote: > nit: missing period (for consistency with other *_stories.py files) Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:70: # Clicks on Today's top hits. (SEARCH_IDENTIFIER) On 2016/09/14 16:42:57, petrcermak wrote: > This is a proof that passing SEARCH_IDENTIFIER through an argument doesn't make > sense (because you already know that it will be passed). Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:72: # Clicks on playlist. (NAVIGATE_IDENTIFIER) On 2016/09/14 16:42:57, petrcermak wrote: > nit: no need for "(NAVIGATE_IDENTIFIER)" here. The next line tells the story ;-) > > (the same applies to line 71) Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:73: self._WaitForAndClickElement(action_runner, self.NAVIGATE_IDENTIFIER) On 2016/09/14 17:02:03, nednguyen wrote: > The timing of google play is in "<span id="time-container-current" > aria-label="Current track time: 19 seconds">0:19</span>" Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:94: class PandoraDesktopStory(_MediaDesktopStory): On 2016/09/14 16:42:57, petrcermak wrote: > Note that at this point, this class could just subclass directly from > system_health_story.SystemHealthStory because it doesn't use any of the > _MediaStory code. To address this, I suggest that you update > _MediaStory.RunPageInteractions to do the following (same for STOP_IDENTIFIER): > > ... > if self.PLAY_IDENFIER: > self._WaitForAndClickElement(action_runner, self.PLAY_IDENTIFIER) > ... > > If you do this, there will be no need to override RunPageInteractions at all > (which will render my next comment irrelevant). Done. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:110: self._SearchMedia(action_runner, self.SEARCH_IDENTIFIER, On 2016/09/14 16:42:57, petrcermak wrote: > Why do you do this? You're just calling an empty function. Acknowledged. https://codereview.chromium.org/2334233002/diff/80001/tools/perf/page_sets/sy... tools/perf/page_sets/system_health/media_stories.py:112: action_runner.Wait(self.PLAY_DURATION) On 2016/09/14 17:02:03, nednguyen wrote: > The elapse time of the player is reflected through "<div > class="elapsedTime">0:07</div> ". Can you modify the wait condition to check for > this to be at least PLAY_DURATION? Done, but one concern here is that, at least on GPM, the timer is janky. It regularly will hang and update 5+ seconds at a time. Example: 1,2,7,9,10,15,21
https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... tools/perf/page_sets/system_health/media_stories.py:39: class _MediaDesktopStory(_MediaStory): On 2016/09/14 16:42:56, petrcermak wrote: > On 2016/09/13 18:52:40, rnephew (Reviews Here) wrote: > > On 2016/09/13 16:49:03, petrcermak wrote: > > > This seems to add an unnecessary level of indirection. I'd remove this class > > and > > > set SUPPORTED_PLATFORMS in both subclasses. > > > > This is setting up for when I add MediaMobileStory once I get the mobile > > versions done. > > This is already the case in other SH stories files, but they don't define the > intermediate *Desktop* and *Mobile* abstract classes, e.g. > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows.... > You can always re-introduce this class if you ever need to (because of some > shared platform-specific code), but right now, it just adds complexity. Woops. Missed this one. Done.
On 2016/09/20 22:19:30, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... > File tools/perf/page_sets/system_health/media_stories.py (right): > > https://codereview.chromium.org/2334233002/diff/1/tools/perf/page_sets/system... > tools/perf/page_sets/system_health/media_stories.py:39: class > _MediaDesktopStory(_MediaStory): > On 2016/09/14 16:42:56, petrcermak wrote: > > On 2016/09/13 18:52:40, rnephew (Reviews Here) wrote: > > > On 2016/09/13 16:49:03, petrcermak wrote: > > > > This seems to add an unnecessary level of indirection. I'd remove this > class > > > and > > > > set SUPPORTED_PLATFORMS in both subclasses. > > > > > > This is setting up for when I add MediaMobileStory once I get the mobile > > > versions done. > > > > This is already the case in other SH stories files, but they don't define the > > intermediate *Desktop* and *Mobile* abstract classes, e.g. > > > https://cs.chromium.org/chromium/src/tools/perf/page_sets/system_health/brows.... > > You can always re-introduce this class if you ever need to (because of some > > shared platform-specific code), but right now, it just adds complexity. > > Woops. Missed this one. Done. lgtm I patch your CL & run the story. Looks great!
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2334233002/#ps140001 (title: "[System Health] Create first System Health media user stories.")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
A few more comments. Sorry for being pedantic. Thanks, Petr https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:43: self.TIME_ELEMENT).split(':') you should define `TIME_ELEMENT = NotImplemented` at the top of the class. The same applies to PLAY_ELEMENT and STOP_ELEMENT https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:56: SEARCH_ELEMENT = 'document.querySelector(".title.fade-out.tooltip")' nit: please modify the order of the class constants here (and in the other story classes below) so that it's: NAME URL constants from _MediaStory story-specific constants for example, in this class, TIME_ELEMENT (_MediaStory constant) should be defined above NAVIGATE_IDENTIFIER (GooglePlayMusicDesktopStory constant). https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:58: 'document.getElementsByClassName("x-scope paper-fab-0")[0]') You use "document.getElementByClassName(SELECTOR)[0]" pretty much everywhere, which leads to unnecessary boilerplate. Could you please define SEARCH_SELECTOR, PLAY_SELECTOR, STOP_SELECTOR, ... instead and then use `"document.querySelector(%s)" % selc.SEARCH_SELECTOR` when appropriate? https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:61: NAVIGATE_IDENTIFIER = ( why is this called "IDENTIFIER" and not "ELEMENT" (inconsistent)? https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:63: TIME_ELEMENT = 'document.getElementById("time-container-current").innerHTML' why don't you use `textContent` instead of `innerHTML`? https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:63: TIME_ELEMENT = 'document.getElementById("time-container-current").innerHTML' All other ELEMENTs refer to the element, but this refer's to the element's HTML code (inconsistent). I suggest you use a selector instead (as I recommended in one of my previous comments) and then just do `'document.querySelector(%s).textContent' % self.TIME_SELECTOR` instead. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:76: def _GetTimeInSeconds(self, action_runner): No need to redefine the method again. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:102: # Format of returned value is: Instead of doing this in python, change your TIME_ELEMENT to: document.querySelector(".playbackTimeline__timePassed > span[aria-hidden=true]").textContent You can then reuse the existing implementation https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:115: SEARCH_QUERY = None no need to define SEARCH_QUERY
https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:43: self.TIME_ELEMENT).split(':') On 2016/09/22 09:50:56, petrcermak wrote: > you should define `TIME_ELEMENT = NotImplemented` at the top of the class. The > same applies to PLAY_ELEMENT and STOP_ELEMENT Done. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:56: SEARCH_ELEMENT = 'document.querySelector(".title.fade-out.tooltip")' On 2016/09/22 09:50:56, petrcermak wrote: > nit: please modify the order of the class constants here (and in the other story > classes below) so that it's: > > NAME > URL > constants from _MediaStory > story-specific constants > > for example, in this class, TIME_ELEMENT (_MediaStory constant) should be > defined above NAVIGATE_IDENTIFIER (GooglePlayMusicDesktopStory constant). Done. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:58: 'document.getElementsByClassName("x-scope paper-fab-0")[0]') On 2016/09/22 09:50:56, petrcermak wrote: > You use "document.getElementByClassName(SELECTOR)[0]" pretty much everywhere, > which leads to unnecessary boilerplate. Could you please define SEARCH_SELECTOR, > PLAY_SELECTOR, STOP_SELECTOR, ... instead and then use > `"document.querySelector(%s)" % selc.SEARCH_SELECTOR` when appropriate? Done. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:61: NAVIGATE_IDENTIFIER = ( On 2016/09/22 09:50:56, petrcermak wrote: > why is this called "IDENTIFIER" and not "ELEMENT" (inconsistent)? Missed when switching everything. Fixed. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:63: TIME_ELEMENT = 'document.getElementById("time-container-current").innerHTML' On 2016/09/22 09:50:56, petrcermak wrote: > why don't you use `textContent` instead of `innerHTML`? Didn't know about it, switched to it. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:63: TIME_ELEMENT = 'document.getElementById("time-container-current").innerHTML' On 2016/09/22 09:50:56, petrcermak wrote: > All other ELEMENTs refer to the element, but this refer's to the element's HTML > code (inconsistent). I suggest you use a selector instead (as I recommended in > one of my previous comments) and then just do > `'document.querySelector(%s).textContent' % self.TIME_SELECTOR` instead. Done. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:76: def _GetTimeInSeconds(self, action_runner): On 2016/09/22 09:50:56, petrcermak wrote: > No need to redefine the method again. Done. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:102: # Format of returned value is: On 2016/09/22 09:50:56, petrcermak wrote: > Instead of doing this in python, change your TIME_ELEMENT to: > > document.querySelector(".playbackTimeline__timePassed > > span[aria-hidden=true]").textContent > > You can then reuse the existing implementation Done. https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:115: SEARCH_QUERY = None On 2016/09/22 09:50:56, petrcermak wrote: > no need to define SEARCH_QUERY Done.
On 2016/09/22 16:40:52, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/media_stories.py (right): > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:43: > self.TIME_ELEMENT).split(':') > On 2016/09/22 09:50:56, petrcermak wrote: > > you should define `TIME_ELEMENT = NotImplemented` at the top of the class. The > > same applies to PLAY_ELEMENT and STOP_ELEMENT > > Done. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:56: SEARCH_ELEMENT = > 'document.querySelector(".title.fade-out.tooltip")' > On 2016/09/22 09:50:56, petrcermak wrote: > > nit: please modify the order of the class constants here (and in the other > story > > classes below) so that it's: > > > > NAME > > URL > > constants from _MediaStory > > story-specific constants > > > > for example, in this class, TIME_ELEMENT (_MediaStory constant) should be > > defined above NAVIGATE_IDENTIFIER (GooglePlayMusicDesktopStory constant). > > Done. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:58: > 'document.getElementsByClassName("x-scope paper-fab-0")[0]') > On 2016/09/22 09:50:56, petrcermak wrote: > > You use "document.getElementByClassName(SELECTOR)[0]" pretty much everywhere, > > which leads to unnecessary boilerplate. Could you please define > SEARCH_SELECTOR, > > PLAY_SELECTOR, STOP_SELECTOR, ... instead and then use > > `"document.querySelector(%s)" % selc.SEARCH_SELECTOR` when appropriate? > > Done. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:61: NAVIGATE_IDENTIFIER = ( > On 2016/09/22 09:50:56, petrcermak wrote: > > why is this called "IDENTIFIER" and not "ELEMENT" (inconsistent)? > > Missed when switching everything. Fixed. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:63: TIME_ELEMENT = > 'document.getElementById("time-container-current").innerHTML' > On 2016/09/22 09:50:56, petrcermak wrote: > > why don't you use `textContent` instead of `innerHTML`? > > Didn't know about it, switched to it. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:63: TIME_ELEMENT = > 'document.getElementById("time-container-current").innerHTML' > On 2016/09/22 09:50:56, petrcermak wrote: > > All other ELEMENTs refer to the element, but this refer's to the element's > HTML > > code (inconsistent). I suggest you use a selector instead (as I recommended in > > one of my previous comments) and then just do > > `'document.querySelector(%s).textContent' % self.TIME_SELECTOR` instead. > > Done. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:76: def > _GetTimeInSeconds(self, action_runner): > On 2016/09/22 09:50:56, petrcermak wrote: > > No need to redefine the method again. > > Done. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:102: # Format of returned > value is: > On 2016/09/22 09:50:56, petrcermak wrote: > > Instead of doing this in python, change your TIME_ELEMENT to: > > > > document.querySelector(".playbackTimeline__timePassed > > > span[aria-hidden=true]").textContent > > > > You can then reuse the existing implementation > > Done. > > https://codereview.chromium.org/2334233002/diff/140001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:115: SEARCH_QUERY = None > On 2016/09/22 09:50:56, petrcermak wrote: > > no need to define SEARCH_QUERY > > Done. It looks like the failures in the last attempt were chrome crashing and not related to my CL itself. Trying just those bots again then will commit after I see how they react.
Thanks a lot for implementing my previous comments, especially the selectors :-) LGTM with some final nits (feel free to send this patch to CQ after you address them). Petr https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:35: is_selector=True): As far as I can tell, you never pass is_selector=False, so please remove the argument. I suggest you then rename the argument to "selector" and do: element_function = 'document.querySelector("%s")' % selector https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:48: time_funct = ( supernit: "function" is typically abbreviated to "func" (see https://cs.chromium.org/search/?q=func%5Cb vs https://cs.chromium.org/search/?q=funct%5Cb). https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:75: action_runner.Wait(1) # Add 1 second wait to make the browsing realistic. nit: there should be 2 spaces before "#" (see http://google.github.io/styleguide/pyguide.html#Comments). This also applies to lines 78, 90, 94 and 112.
On 2016/09/22 17:10:49, petrcermak wrote: > Thanks a lot for implementing my previous comments, especially the selectors :-) > > LGTM with some final nits (feel free to send this patch to CQ after you address > them). > > Petr > > https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/media_stories.py (right): > > https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:35: is_selector=True): > As far as I can tell, you never pass is_selector=False, so please remove the > argument. I suggest you then rename the argument to "selector" and do: > > element_function = 'document.querySelector("%s")' % selector > > https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:48: time_funct = ( > supernit: "function" is typically abbreviated to "func" (see > https://cs.chromium.org/search/?q=func%5Cb vs > https://cs.chromium.org/search/?q=funct%5Cb). > > https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/media_stories.py:75: action_runner.Wait(1) # > Add 1 second wait to make the browsing realistic. > nit: there should be 2 spaces before "#" (see > http://google.github.io/styleguide/pyguide.html#Comments). This also applies to > lines 78, 90, 94 and 112. If the pandora smoke test still fails on win platform, feel free to disable it with the decorator with an open bug to track reenabling it.
> It looks like the failures in the last attempt were chrome crashing and not > related to my CL itself. Trying just those bots again then will commit after I > see how they react. https://chromium-swarm.appspot.com/user/task/316ba56ad8bf1210 Failed again this time. The run says: Either tab has crashed or browser does not support taking tab screenshot. Skip taking screenshot on failure. Which I assume means that it crashed. What is the usual thing for telemetry tests when it is failing due to the new benchmark exercising chrome in a new way instead of anything directly to do with my CL?
On 2016/09/22 17:12:33, rnephew (Reviews Here) wrote: > > It looks like the failures in the last attempt were chrome crashing and not > > related to my CL itself. Trying just those bots again then will commit after I > > see how they react. > > https://chromium-swarm.appspot.com/user/task/316ba56ad8bf1210 > Failed again this time. The run says: > Either tab has crashed or browser does not support taking tab screenshot. Skip > taking screenshot on failure. > > Which I assume means that it crashed. What is the usual thing for telemetry > tests when it is failing due to the new benchmark exercising chrome in a new way > instead of anything directly to do with my CL? Posted this before I saw your post ned, will disable it and retry.
https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/media_stories.py (right): https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:35: is_selector=True): On 2016/09/22 17:10:48, petrcermak wrote: > As far as I can tell, you never pass is_selector=False, so please remove the > argument. I suggest you then rename the argument to "selector" and do: > > element_function = 'document.querySelector("%s")' % selector Done. https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:48: time_funct = ( On 2016/09/22 17:10:48, petrcermak wrote: > supernit: "function" is typically abbreviated to "func" (see > https://cs.chromium.org/search/?q=func%5Cb vs > https://cs.chromium.org/search/?q=funct%5Cb). Done. https://codereview.chromium.org/2334233002/diff/160001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/media_stories.py:75: action_runner.Wait(1) # Add 1 second wait to make the browsing realistic. On 2016/09/22 17:10:48, petrcermak wrote: > nit: there should be 2 spaces before "#" (see > http://google.github.io/styleguide/pyguide.html#Comments). This also applies to > lines 78, 90, 94 and 112. Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2334233002/#ps180001 (title: "[System Health] Create first System Health media user stories.")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/09/22 19:38:45, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) Trying again, I dont think the failures are related to the CL.
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/22 19:42:14, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Actually, it looks like the soundcloud test is causing chrome to crash as well... [23/24] benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.play:media:soundcloud failed unexpectedly 4.7580s: [ RUN ] play:media:soundcloud Traceback (most recent call last): File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\telemetry\telemetry\internal\story_runner.py", line 79, in _RunStoryAndProcessErrorIfNeeded state.WillRunStory(story) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\common\py_trace_event\py_trace_event\trace_event_impl\decorators.py", line 52, in traced_function return func(*args, **kwargs) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\telemetry\telemetry\page\shared_page_state.py", line 224, in WillRunStory self._StartBrowser(page) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\common\py_trace_event\py_trace_event\trace_event_impl\decorators.py", line 52, in traced_function return func(*args, **kwargs) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\telemetry\telemetry\page\shared_page_state.py", line 184, in _StartBrowser self._browser = self._possible_browser.Create(self._finder_options) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\telemetry\telemetry\internal\backends\chrome\desktop_browser_finder.py", line 68, in Create browser_backend, self._platform_backend, self._credentials_path) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\telemetry\telemetry\internal\browser\browser.py", line 55, in __init__ self._browser_backend.Start() File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\common\py_trace_event\py_trace_event\trace_event_impl\decorators.py", line 52, in traced_function return func(*args, **kwargs) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\telemetry\telemetry\internal\backends\chrome\desktop_browser_backend.py", line 294, in Start self._WaitForBrowserToComeUp() File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\common\py_trace_event\py_trace_event\trace_event_impl\decorators.py", line 52, in traced_function return func(*args, **kwargs) File "e:\b\swarm_slave\w\irgsrzfb\third_party\catapult\telemetry\telemetry\internal\backends\chrome\chrome_browser_backend.py", line 160, in _WaitForBrowserToComeUp raise exceptions.BrowserGoneException(self.browser, e) BrowserGoneException: Return code: -1073741819 Found Minidump: False Stack Trace: ******************************************************************************** No crash dump found. ******************************************************************************** Standard output: ******************************************************************************** ******************************************************************************** [ FAILED ] play:media:soundcloud (2136 ms) [ PASSED ] 0 tests. [ FAILED ] 1 test, listed below: [ FAILED ] play:media:soundcloud
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/09/22 21:56:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) Going to try running it again now since its been awhile. Hopefully whatever was causing chrome to crash is fixed. If not I will investigate more and see if I can figure it out.
On 2016/09/26 17:58:28, rnephew (Reviews Here) wrote: > On 2016/09/22 21:56:51, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > Going to try running it again now since its been awhile. Hopefully whatever was > causing chrome to crash is fixed. If not I will investigate more and see if I > can figure it out. I did a local checkout of linux chromium and built with 'ninja -C out/Default chrome' When trying to run these stories on that using: './run_benchmark --browser=default system_health.common_desktop --also-run-disabled-tests --story-filter='play:media' I keep getting errors saying you need flash. Is this similar to how the bots will run it? Do I have to set something to ensure flash is available? This problem manifests on both soundcloud and google play music.
nednguyen@google.com changed reviewers: + dtu@chromium.org
+Dave: how does the perf waterfall build created? I remembered that we do have flash on those build
On 2016/09/26 21:57:51, nednguyen wrote: > +Dave: how does the perf waterfall build created? I remembered that we do have > flash on those build I disabled them on windows since that seems to be where the crashes are occurring. Since thats the only real change, I'll do a dry run and if no one has lg2m'd by them commit since there is no change in how the stories work.
The CQ bit was checked by rnephew@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 rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2334233002/#ps200001 (title: "disable on win")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [System Health] Create first System Health media user stories. Audio cases for Pandora, Soundcloud, and Google Play Music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [System Health] Create first System Health media user stories. Audio cases for Pandora, Soundcloud, and Google Play Music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [System Health] Create first System Health media user stories. Audio cases for Pandora, Soundcloud, and Google Play Music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [System Health] Create first System Health media user stories. Audio cases for Pandora, Soundcloud, and Google Play Music. BUG=646392 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq Committed: https://crrev.com/536ae480e73750dfaab944f54e0f9a19a78c712f Cr-Commit-Position: refs/heads/master@{#421533} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/536ae480e73750dfaab944f54e0f9a19a78c712f Cr-Commit-Position: refs/heads/master@{#421533}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:200001) has been created in https://codereview.chromium.org/2375003003/ by rnephew@chromium.org. The reason for reverting is: Causing failures: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/46833. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
