Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
983983002:
Some browser_tests for Drive search. (Closed)
|
Created:
5 years, 9 months ago by dhaddock1 Modified:
5 years, 9 months ago CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSome browser_tests for Drive search.
First attempt at browsers tests. I've added two tests to cover a regression
issue that came up recently where clicking the search result in the
autocomplete box didn't show all of the results. Pressing enter didn't do
anything either.
I have refactored drive_specific/autocomplete so I could make use of it.
BUG=chromium:440251, chromium:454907
TEST=browser_tests --gtest_filter="*DriveSpecific/FileManagerBrowserTest.Test/{5/6}"
Committed: https://crrev.com/15064e4a591127eee81c56407558c275160f3b94
Cr-Commit-Position: refs/heads/master@{#319830}
Patch Set 1 #Patch Set 2 : Removal some dummy code I left in #
Total comments: 19
Patch Set 3 : Fixes from your comments #Patch Set 4 : Update commit message with another tracking issue #Patch Set 5 : change to mouseDown #
Total comments: 10
Patch Set 6 : Fixed nits #
Messages
Total messages: 17 (4 generated)
dhaddock@chromium.org changed reviewers: + fukino@chromium.org, mtomasz@chromium.org
dhaddock@chromium.org changed reviewers: + kathrelkeld@chromium.org
Nice! Looks pretty good. First quick round of comments. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:8: * The expected autocomplete results nit: We can add jsdoc here similarly to the one below. @type {Array<string>} @const https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:12: 'hello.txt', nit: No trailing ,. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:18: * @type {Array.<TestEntryInfo>} nit: We recently started using Array< without the dot. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:29: function openSearchResultsAutoComplete() { nit: Rename to getStepsForXXX? https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:47: console.log(appId); nit: Please remove debug logs before committing. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:265: function(result) nit: { should be in the previous line. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:270: function(actualFilesAfter) nit: ditto
Awesome! Thank you for making test to prevent the regression again. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:224: ['#autocomplete-list'], Shouldn't be ['#autocomplete-list li[selected]']?
https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:8: * The expected autocomplete results On 2015/03/06 05:33:11, mtomasz wrote: > nit: We can add jsdoc here similarly to the one below. > @type {Array<string>} > @const Done. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:12: 'hello.txt', On 2015/03/06 05:33:11, mtomasz wrote: > nit: No trailing ,. Done. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:18: * @type {Array.<TestEntryInfo>} On 2015/03/06 05:33:11, mtomasz wrote: > nit: We recently started using Array< without the dot. Done. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:29: function openSearchResultsAutoComplete() { On 2015/03/06 05:33:11, mtomasz wrote: > nit: Rename to getStepsForXXX? Done. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:29: function openSearchResultsAutoComplete() { On 2015/03/06 05:33:11, mtomasz wrote: > nit: Rename to getStepsForXXX? Done. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:47: console.log(appId); On 2015/03/06 05:33:11, mtomasz wrote: > nit: Please remove debug logs before committing. Done. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:224: ['#autocomplete-list'], On 2015/03/06 06:15:08, fukino wrote: > Shouldn't be ['#autocomplete-list li[selected]']? I tried fakeMouseClick on ['#autocomplete-list li[selected]'] first but it returned false for last two mouse events. I tested it though. The li that is selected gets clicked with you click #autocomplete-list. So if you select the first option and click #autocomplete-list it does a search. If you select the second option and click #autocomplete-list it opens the hello.txt. Let me know if there is a better way. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:265: function(result) On 2015/03/06 05:33:11, mtomasz wrote: > nit: { should be in the previous line. Done. https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:270: function(actualFilesAfter) On 2015/03/06 05:33:11, mtomasz wrote: > nit: ditto Done.
https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:224: ['#autocomplete-list'], On 2015/03/06 21:48:27, dhaddock1 wrote: > On 2015/03/06 06:15:08, fukino wrote: > > Shouldn't be ['#autocomplete-list li[selected]']? > > I tried fakeMouseClick on ['#autocomplete-list li[selected]'] first but it > returned false for last two mouse events. > > I tested it though. The li that is selected gets clicked with you click > #autocomplete-list. So if you select the first option and click > #autocomplete-list it does a search. If you select the second option and click > #autocomplete-list it opens the hello.txt. > > Let me know if there is a better way. Using 'fakeMouseDown' instead of 'fakeMouseClick' will solve the problem. In fakeMouseClick (https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...), mouseover, mousedown, mouseup, and click event is sent sequentially, and the target for the event is queried for each event. However, after mousedown event, the autocomplete-list will disappear, so the target for following mouseup/click event will not be found. This should be the cause of failure.
https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/20001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:224: ['#autocomplete-list'], On 2015/03/09 01:22:14, fukino wrote: > On 2015/03/06 21:48:27, dhaddock1 wrote: > > On 2015/03/06 06:15:08, fukino wrote: > > > Shouldn't be ['#autocomplete-list li[selected]']? > > > > I tried fakeMouseClick on ['#autocomplete-list li[selected]'] first but it > > returned false for last two mouse events. > > > > I tested it though. The li that is selected gets clicked with you click > > #autocomplete-list. So if you select the first option and click > > #autocomplete-list it does a search. If you select the second option and click > > #autocomplete-list it opens the hello.txt. > > > > Let me know if there is a better way. > > Using 'fakeMouseDown' instead of 'fakeMouseClick' will solve the problem. > > In fakeMouseClick > (https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...), > mouseover, mousedown, mouseup, and click event is sent sequentially, and the > target for the event is queried for each event. > However, after mousedown event, the autocomplete-list will disappear, so the > target for following mouseup/click event will not be found. This should be the > cause of failure. Aha that makes sense. It worked. Thank you!
I've addressed all comments. Thanks
lgtm with nits from me. Please wait for @fukino lgtm. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:68: then(function(elements) { nit: Indent should be 4 spaces in this case in #68. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:235: TestEntryInfo.getExpectedRows(SEARCH_RESULTS_ENTRY_SET).sort(), nit: Indent should be 4 spaces here. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:261: appId, nit: 4 spaces. In general indent is 2 spaces for blocks {} and arrays [] and 4 spaces for everything else. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:271: TestEntryInfo.getExpectedRows(SEARCH_RESULTS_ENTRY_SET).sort(), nit: ditto
Thank you! lgtm with a nit. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:280: nit: It seems this blank line is inserted unintentionally?
Fixed nits https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... File ui/file_manager/integration_tests/file_manager/drive_specific.js (right): https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:68: then(function(elements) { On 2015/03/10 01:52:28, mtomasz wrote: > nit: Indent should be 4 spaces in this case in #68. Done. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:235: TestEntryInfo.getExpectedRows(SEARCH_RESULTS_ENTRY_SET).sort(), On 2015/03/10 01:52:28, mtomasz wrote: > nit: Indent should be 4 spaces here. Done. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:261: appId, On 2015/03/10 01:52:28, mtomasz wrote: > nit: 4 spaces. > In general indent is 2 spaces for blocks {} and arrays [] and 4 spaces for > everything else. Done. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:271: TestEntryInfo.getExpectedRows(SEARCH_RESULTS_ENTRY_SET).sort(), On 2015/03/10 01:52:28, mtomasz wrote: > nit: ditto Done. https://codereview.chromium.org/983983002/diff/80001/ui/file_manager/integrat... ui/file_manager/integration_tests/file_manager/drive_specific.js:280: On 2015/03/10 01:56:39, fukino wrote: > nit: It seems this blank line is inserted unintentionally? Done.
The CQ bit was checked by dhaddock@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org, fukino@chromium.org Link to the patchset: https://codereview.chromium.org/983983002/#ps100001 (title: "Fixed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983983002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/15064e4a591127eee81c56407558c275160f3b94 Cr-Commit-Position: refs/heads/master@{#319830} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
