Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1526923002:
AudioPlayer: Add UI tests. (Closed)
|
Created:
5 years ago by fukino Modified:
5 years ago Reviewers:
yawano CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAudioPlayer: Add UI tests.
Adding following tests:
- Audio player should start playing automatically on launch.
- Play button should toggle the play state and change aria-label accordingly.
- Volume button should mute/unmute the volume.
- Next button should activate the next track and start playing it.
- Playlist button should expand track list.
- Clicking a track on play list should change the active track, but shouldn't change play state.
- Clicking play icon on a track should change the active track and start playing it.
BUG=506007
TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.*
Committed: https://crrev.com/39e3c00516e2ed79089c9596754bfbab0c827b8c
Cr-Commit-Position: refs/heads/master@{#365709}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : Address review comments. #
Total comments: 6
Patch Set 4 : Make it sequntial. #
Messages
Total messages: 18 (7 generated)
Description was changed from ========== AudioPlayer: Add UI tests. BUG=none TEST=run browser_tests --gtest_filter= ========== to ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=none TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.ClickControlButtons ==========
fukino@chromium.org changed reviewers: + yawano@chromium.org
PTAL. Thanks!
Description was changed from ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=none TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.ClickControlButtons ========== to ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=506007 TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.ClickControlButtons ==========
https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player/click_control_buttons.js (right): https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:10: testcase.clickControlButtons = function() { This test case has more than 20 steps. It seems to be difficult to read what this test is doing. Is it possible to split this test case into a few smaller test cases? e.g. split into volumeControlTest, playListControlTest, playButtonControlTest. https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:23: appId, ['#play[aria-label=\'Pause\']']); nit: How about to use double quotes here? Our HTML style guide says we should use double quote for attribute value. - https://google.github.io/styleguide/htmlcssguide.xml?showone=HTML_Quotation_M... Also you don't need to use escape character if you use double quote. https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:109: nit: unnecessary blank line. https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player_test_manifest.json (right): https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player_test_manifest.json:14: "audio_player/click_control_buttons.js" nit: put file names in alphabetical order.
https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player/click_control_buttons.js (right): https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:10: testcase.clickControlButtons = function() { On 2015/12/16 02:54:14, yawano wrote: > This test case has more than 20 steps. It seems to be difficult to read what > this test is doing. Is it possible to split this test case into a few smaller > test cases? > > e.g. split into volumeControlTest, playListControlTest, playButtonControlTest. Thank you for the suggestion! Done. https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:23: appId, ['#play[aria-label=\'Pause\']']); On 2015/12/16 02:54:14, yawano wrote: > nit: How about to use double quotes here? Our HTML style guide says we should > use double quote for attribute value. - > https://google.github.io/styleguide/htmlcssguide.xml?showone=HTML_Quotation_M... > > Also you don't need to use escape character if you use double quote. Chromium coding style overrides the rule...: "Use single quotes instead of double quotes for all strings." https://www.chromium.org/developers/web-development-style-guide That said, as this is a test code and this is not CSS file, let me use double quotation for readability. https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:109: On 2015/12/16 02:54:14, yawano wrote: > nit: unnecessary blank line. Done. https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player_test_manifest.json (right): https://codereview.chromium.org/1526923002/diff/20001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player_test_manifest.json:14: "audio_player/click_control_buttons.js" On 2015/12/16 02:54:14, yawano wrote: > nit: put file names in alphabetical order. Done.
Thank you! lgtm with optional nits. https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player/click_control_buttons.js (right): https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:134: appId, '.track[index="0"][active]'); optional nit: how about to put this in the Promise.all same as #107. https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:145: appId, '.track[index="1"][active]'); optional nit: same with comment at #134.
Thank you! https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player/click_control_buttons.js (right): https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:134: appId, '.track[index="0"][active]'); On 2015/12/16 08:01:44, yawano wrote: > optional nit: how about to put this in the Promise.all same as #107. Both way should work, but I think sequential process is more natural to confirm that "Track #0 is paused". Otherwise, play states might be checked before the track is still #1. https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:145: appId, '.track[index="1"][active]'); On 2015/12/16 08:01:44, yawano wrote: > optional nit: same with comment at #134. ditto
https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player/click_control_buttons.js (right): https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:134: appId, '.track[index="0"][active]'); Yes, both will work. I added a comment just for consistency with #107. I think both ways are okay.
https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... File ui/file_manager/integration_tests/audio_player/click_control_buttons.js (right): https://codereview.chromium.org/1526923002/diff/40001/ui/file_manager/integra... ui/file_manager/integration_tests/audio_player/click_control_buttons.js:134: appId, '.track[index="0"][active]'); On 2015/12/16 08:53:39, yawano wrote: > Yes, both will work. I added a comment just for consistency with #107. I think > both ways are okay. Oops, I overlooked #107. I made it sequential. Thank you for pointing it out!
still lgtm. Thank you!
Description was changed from ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=506007 TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.ClickControlButtons ========== to ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=506007 TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.* ==========
The CQ bit was checked by fukino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1526923002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1526923002/60001
Message was sent while issue was closed.
Description was changed from ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=506007 TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.* ========== to ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=506007 TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.* ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=506007 TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.* ========== to ========== AudioPlayer: Add UI tests. Adding following tests: - Audio player should start playing automatically on launch. - Play button should toggle the play state and change aria-label accordingly. - Volume button should mute/unmute the volume. - Next button should activate the next track and start playing it. - Playlist button should expand track list. - Clicking a track on play list should change the active track, but shouldn't change play state. - Clicking play icon on a track should change the active track and start playing it. BUG=506007 TEST=run browser_tests --gtest_filter=AudioPlayerBrowserTest.* Committed: https://crrev.com/39e3c00516e2ed79089c9596754bfbab0c827b8c Cr-Commit-Position: refs/heads/master@{#365709} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/39e3c00516e2ed79089c9596754bfbab0c827b8c Cr-Commit-Position: refs/heads/master@{#365709} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
