Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
177052:
Add autocomplete_edit_view_browsertest.cc (Closed)
|
Created:
11 years, 3 months ago by James Su Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, Ben Goodger (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd autocomplete_edit_view_browsertest.cc.
This CL adds automated tests for AutocompleteEditView.
BUG=20422
: Autocomplete edit view needs automated testing
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26190
Patch Set 1 #Patch Set 2 : '' #
Total comments: 12
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 10
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 20
Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 6
Patch Set 13 : '' #Patch Set 14 : '' #
Total comments: 4
Messages
Total messages: 24 (0 generated)
Hi, It's a preliminary CL to add automated tests for autocomplete edit view. It lacks lots of cases yet and still fails on Windows. I just want you to help review it to see if it's a right approach. And if possible, please help me try it on Windows to see why it fails. I have no Windows machine right now. Regards James Su
Drive-by (and I have a watchlist for that). NACK. This patch is going to induce flakiness. I can help with making the test more solid. http://codereview.chromium.org/177052/diff/1008/9 File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc (right): http://codereview.chromium.org/177052/diff/1008/9#newcode26 Line 26: browser_ = automation()->GetBrowserWindow(0); ASSERT that browser_ is not NULL. http://codereview.chromium.org/177052/diff/1008/9#newcode27 Line 27: window_ = browser_->GetWindow(); ASSERT that window_ is not NULL http://codereview.chromium.org/177052/diff/1008/9#newcode28 Line 28: autocomplete_edit_ = browser_->GetAutocompleteEdit(); ASSERT that autocomplete_edit_ is not NULL http://codereview.chromium.org/177052/diff/1008/9#newcode35 Line 35: PlatformThread::Sleep(200); This Sleep makes me very suspicious. It seems it will induce flakiness at some point. Would adding some new automation message help (to wait on the browser side)? I can explain more if needed. http://codereview.chromium.org/177052/diff/1008/9#newcode43 Line 43: static const int kWaitTimeout = 10000; Could you use something from ui_test instead? Like action_max_timeout() etc? http://codereview.chromium.org/177052/diff/1008/9#newcode60 Line 60: ASSERT_TRUE(autocomplete_edit_->IsSelectAll((&is_select_all))); nit: Seems that some parentheses are redundant (( )) http://codereview.chromium.org/177052/diff/1008/9#newcode114 Line 114: PlatformThread::Sleep(200); Suspicious Sleep, see above. http://codereview.chromium.org/177052/diff/1008/9#newcode127 Line 127: PlatformThread::Sleep(200); Oh, no! A Sleep again. http://codereview.chromium.org/177052/diff/1008/11 File chrome/test/automation/autocomplete_edit_proxy.cc (right): http://codereview.chromium.org/177052/diff/1008/11#newcode112 Line 112: while (TimeTicks::Now() - start < timeout) { This kind of code is known by the State of California to be flaky. Please add a new automation message and wait on the browser side. http://codereview.chromium.org/177052/diff/1008/11#newcode142 Line 142: while (TimeTicks::Now() - start < timeout) { Another flaky code to be replaced by waiting on the browser side. http://codereview.chromium.org/177052/diff/1008/11#newcode172 Line 172: while (TimeTicks::Now() - start < timeout) { And yet another one.
http://codereview.chromium.org/177052/diff/1008/9 File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc (right): http://codereview.chromium.org/177052/diff/1008/9#newcode35 Line 35: PlatformThread::Sleep(200); why won't keyword mode be triggered without this sleep?
Thanks a lot for your feedback. I don't want to use Sleep as well. It'll be best if we can have an automation message to check if an input event has already been processed or not. But I don't have any idea on how to implement it. Do you have any suggestion about it? Regards James Su On 2009/09/01 15:37:41, Paweł Hajdan Jr. wrote: > Drive-by (and I have a watchlist for that). > > NACK. This patch is going to induce flakiness. I can help with making the test > more solid. > > http://codereview.chromium.org/177052/diff/1008/9 > File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc (right): > > http://codereview.chromium.org/177052/diff/1008/9#newcode26 > Line 26: browser_ = automation()->GetBrowserWindow(0); > ASSERT that browser_ is not NULL. > > http://codereview.chromium.org/177052/diff/1008/9#newcode27 > Line 27: window_ = browser_->GetWindow(); > ASSERT that window_ is not NULL > > http://codereview.chromium.org/177052/diff/1008/9#newcode28 > Line 28: autocomplete_edit_ = browser_->GetAutocompleteEdit(); > ASSERT that autocomplete_edit_ is not NULL > > http://codereview.chromium.org/177052/diff/1008/9#newcode35 > Line 35: PlatformThread::Sleep(200); > This Sleep makes me very suspicious. It seems it will induce flakiness at some > point. Would adding some new automation message help (to wait on the browser > side)? > > I can explain more if needed. > > http://codereview.chromium.org/177052/diff/1008/9#newcode43 > Line 43: static const int kWaitTimeout = 10000; > Could you use something from ui_test instead? Like action_max_timeout() etc? > > http://codereview.chromium.org/177052/diff/1008/9#newcode60 > Line 60: ASSERT_TRUE(autocomplete_edit_->IsSelectAll((&is_select_all))); > nit: Seems that some parentheses are redundant (( )) > > http://codereview.chromium.org/177052/diff/1008/9#newcode114 > Line 114: PlatformThread::Sleep(200); > Suspicious Sleep, see above. > > http://codereview.chromium.org/177052/diff/1008/9#newcode127 > Line 127: PlatformThread::Sleep(200); > Oh, no! A Sleep again. > > http://codereview.chromium.org/177052/diff/1008/11 > File chrome/test/automation/autocomplete_edit_proxy.cc (right): > > http://codereview.chromium.org/177052/diff/1008/11#newcode112 > Line 112: while (TimeTicks::Now() - start < timeout) { > This kind of code is known by the State of California to be flaky. Please add a > new automation message and wait on the browser side. > > http://codereview.chromium.org/177052/diff/1008/11#newcode142 > Line 142: while (TimeTicks::Now() - start < timeout) { > Another flaky code to be replaced by waiting on the browser side. > > http://codereview.chromium.org/177052/diff/1008/11#newcode172 > Line 172: while (TimeTicks::Now() - start < timeout) { > And yet another one.
But the code of AutocompleteEditProxy::WaitForIsSelectAllToBecome(), WaitForIsKeywordHintToBecome() and WaitForTextToBecome() are actually copied from BrowserProxy::WaitForTabCountToBecome(). Regards James Su On 2009/09/02 00:55:40, James Su wrote: > Thanks a lot for your feedback. > I don't want to use Sleep as well. It'll be best if we can have an automation > message to check if an input event has already been processed or not. But I > don't have any idea on how to implement it. Do you have any suggestion about it? > > Regards > James Su > > On 2009/09/01 15:37:41, Paweł Hajdan Jr. wrote: > > Drive-by (and I have a watchlist for that). > > > > NACK. This patch is going to induce flakiness. I can help with making the test > > more solid. > > > > http://codereview.chromium.org/177052/diff/1008/9 > > File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc (right): > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode26 > > Line 26: browser_ = automation()->GetBrowserWindow(0); > > ASSERT that browser_ is not NULL. > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode27 > > Line 27: window_ = browser_->GetWindow(); > > ASSERT that window_ is not NULL > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode28 > > Line 28: autocomplete_edit_ = browser_->GetAutocompleteEdit(); > > ASSERT that autocomplete_edit_ is not NULL > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode35 > > Line 35: PlatformThread::Sleep(200); > > This Sleep makes me very suspicious. It seems it will induce flakiness at some > > point. Would adding some new automation message help (to wait on the browser > > side)? > > > > I can explain more if needed. > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode43 > > Line 43: static const int kWaitTimeout = 10000; > > Could you use something from ui_test instead? Like action_max_timeout() etc? > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode60 > > Line 60: ASSERT_TRUE(autocomplete_edit_->IsSelectAll((&is_select_all))); > > nit: Seems that some parentheses are redundant (( )) > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode114 > > Line 114: PlatformThread::Sleep(200); > > Suspicious Sleep, see above. > > > > http://codereview.chromium.org/177052/diff/1008/9#newcode127 > > Line 127: PlatformThread::Sleep(200); > > Oh, no! A Sleep again. > > > > http://codereview.chromium.org/177052/diff/1008/11 > > File chrome/test/automation/autocomplete_edit_proxy.cc (right): > > > > http://codereview.chromium.org/177052/diff/1008/11#newcode112 > > Line 112: while (TimeTicks::Now() - start < timeout) { > > This kind of code is known by the State of California to be flaky. Please add > a > > new automation message and wait on the browser side. > > > > http://codereview.chromium.org/177052/diff/1008/11#newcode142 > > Line 142: while (TimeTicks::Now() - start < timeout) { > > Another flaky code to be replaced by waiting on the browser side. > > > > http://codereview.chromium.org/177052/diff/1008/11#newcode172 > > Line 172: while (TimeTicks::Now() - start < timeout) { > > And yet another one.
I don't know the exactly reason that why keyword mode won't be triggered without the sleep. It just happened when I tried it on Linux. Regards James Su On 2009/09/01 17:55:37, Evan Stade wrote: > http://codereview.chromium.org/177052/diff/1008/9 > File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc (right): > > http://codereview.chromium.org/177052/diff/1008/9#newcode35 > Line 35: PlatformThread::Sleep(200); > why won't keyword mode be triggered without this sleep?
you can take a look at ui_controls.h, we have some functions in there that will send an event and notify you when the event's been processed. On Tue, Sep 1, 2009 at 6:01 PM, <suzhe@chromium.org> wrote: > I don't know the exactly reason that why keyword mode won't be triggered > without the sleep. It just happened when I tried it on Linux. it would be good to look into. > > Regards > James Su > > On 2009/09/01 17:55:37, Evan Stade wrote: >> >> http://codereview.chromium.org/177052/diff/1008/9 >> File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc > > (right): > >> http://codereview.chromium.org/177052/diff/1008/9#newcode35 >> Line 35: PlatformThread::Sleep(200); >> why won't keyword mode be triggered without this sleep? > > > > http://codereview.chromium.org/177052 >
Thanks for your information. I'll look into these issues. Regards James Su On 2009/09/02 01:06:52, Evan Stade wrote: > you can take a look at ui_controls.h, we have some functions in there > that will send an event and notify you when the event's been > processed. > > On Tue, Sep 1, 2009 at 6:01 PM, <mailto:suzhe@chromium.org> wrote: > > I don't know the exactly reason that why keyword mode won't be triggered > > without the sleep. It just happened when I tried it on Linux. > > it would be good to look into. > > > > > Regards > > James Su > > > > On 2009/09/01 17:55:37, Evan Stade wrote: > >> > >> http://codereview.chromium.org/177052/diff/1008/9 > >> File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc > > > > (right): > > > >> http://codereview.chromium.org/177052/diff/1008/9#newcode35 > >> Line 35: PlatformThread::Sleep(200); > >> why won't keyword mode be triggered without this sleep? > > > > > > > > http://codereview.chromium.org/177052 > >
Hi, I updated this CL to use InProcessBrowserTest instead of UITest to make it simpler. It works on Linux, but unfortunately it still fails on Windows. Can you help me try it on Windows to see why it fails? Otherwise I must checkout and install a Visual Studio in my Windows virtual machine and build chromium in it, which would be very costly. I'll complete this CL after it's confirmed to feasible for both Windows and Linux. Regards James Su On 2009/09/02 01:08:59, James Su wrote: > Thanks for your information. I'll look into these issues. > > Regards > James Su > > On 2009/09/02 01:06:52, Evan Stade wrote: > > you can take a look at ui_controls.h, we have some functions in there > > that will send an event and notify you when the event's been > > processed. > > > > On Tue, Sep 1, 2009 at 6:01 PM, <mailto:suzhe@chromium.org> wrote: > > > I don't know the exactly reason that why keyword mode won't be triggered > > > without the sleep. It just happened when I tried it on Linux. > > > > it would be good to look into. > > > > > > > > Regards > > > James Su > > > > > > On 2009/09/01 17:55:37, Evan Stade wrote: > > >> > > >> http://codereview.chromium.org/177052/diff/1008/9 > > >> File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc > > > > > > (right): > > > > > >> http://codereview.chromium.org/177052/diff/1008/9#newcode35 > > >> Line 35: PlatformThread::Sleep(200); > > >> why won't keyword mode be triggered without this sleep? > > > > > > > > > > > > http://codereview.chromium.org/177052 > > >
http://codereview.chromium.org/177052/diff/10001/10002 File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc (right): http://codereview.chromium.org/177052/diff/10001/10002#newcode28 Line 28: static const int kSleepTime = 250; Please, don't use that. Now that the test is in-process, it will be much easier to wait properly. http://codereview.chromium.org/177052/diff/10001/10002#newcode39 Line 39: DCHECK(window); This DCHECK does more harm than good. NULL pointer dereference is very clear in a debugger. And it potentially terminates the entire binary, which is *very* bad in case of a test. If you need, use ASSERT and possibly HasFatalFailure. http://codereview.chromium.org/177052/diff/10001/10002#newcode45 Line 45: DCHECK(native_window); Same as above. http://codereview.chromium.org/177052/diff/10001/10002#newcode51 Line 51: DCHECK(loc_bar); Here too. http://codereview.chromium.org/177052/diff/10001/10002#newcode53 Line 53: DCHECK(edit_view); And here. http://codereview.chromium.org/177052/diff/10001/10002#newcode85 Line 85: void CheckQueryInProgressTask() { That's bad. To fix that, please add a new NotificationType if there is not one suitable, and fire the notification in appropriate place in the code being tested. Then wait for the notification in the test. http://codereview.chromium.org/177052/diff/10001/10002#newcode110 Line 110: ui_controls::SendKeyPressNotifyWhenDone(window, key, control, shift, alt, OK, that waits properly! http://codereview.chromium.org/177052/diff/10001/10002#newcode130 Line 130: DCHECK(model); ASSERT_TRUE(model) seems more suited for a test. http://codereview.chromium.org/177052/diff/10001/10002#newcode160 Line 160: ASSERT_EQ(tab_count + 1, browser()->tab_count()); I think it should be safe to change these to EXPECT_ instead of ASSERT_. Potentially gives more info in the logs. http://codereview.chromium.org/177052/diff/10001/10003 File chrome/chrome.gyp (right): http://codereview.chromium.org/177052/diff/10001/10003#newcode37 Line 37: 'browser/autocomplete/autocomplete_edit_view_uitest.cc', Now it should be named _browsertest.cc
Hi, Thanks so much for your feedback. I'll update this CL according to your comments asap. I'm now just wondering why it failed on Windows trybot. Do you have any idea about it? I checked existing tests and found that all tests involving key event emulation are compiled into interactive_ui_tests rather than browser_tests. And seems that trybot does not execute interactive_ui_tests. I'm wondering if it's necessary to move this test into interactive_ui_tests? Regards James Su On 2009/09/02 15:56:00, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/177052/diff/10001/10002 > File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc (right): > > http://codereview.chromium.org/177052/diff/10001/10002#newcode28 > Line 28: static const int kSleepTime = 250; > Please, don't use that. Now that the test is in-process, it will be much easier > to wait properly. > > http://codereview.chromium.org/177052/diff/10001/10002#newcode39 > Line 39: DCHECK(window); > This DCHECK does more harm than good. NULL pointer dereference is very clear in > a debugger. And it potentially terminates the entire binary, which is *very* bad > in case of a test. If you need, use ASSERT and possibly HasFatalFailure. > > http://codereview.chromium.org/177052/diff/10001/10002#newcode45 > Line 45: DCHECK(native_window); > Same as above. > > http://codereview.chromium.org/177052/diff/10001/10002#newcode51 > Line 51: DCHECK(loc_bar); > Here too. > > http://codereview.chromium.org/177052/diff/10001/10002#newcode53 > Line 53: DCHECK(edit_view); > And here. > > http://codereview.chromium.org/177052/diff/10001/10002#newcode85 > Line 85: void CheckQueryInProgressTask() { > That's bad. To fix that, please add a new NotificationType if there is not one > suitable, and fire the notification in appropriate place in the code being > tested. Then wait for the notification in the test. > > http://codereview.chromium.org/177052/diff/10001/10002#newcode110 > Line 110: ui_controls::SendKeyPressNotifyWhenDone(window, key, control, shift, > alt, > OK, that waits properly! > > http://codereview.chromium.org/177052/diff/10001/10002#newcode130 > Line 130: DCHECK(model); > ASSERT_TRUE(model) seems more suited for a test. > > http://codereview.chromium.org/177052/diff/10001/10002#newcode160 > Line 160: ASSERT_EQ(tab_count + 1, browser()->tab_count()); > I think it should be safe to change these to EXPECT_ instead of ASSERT_. > Potentially gives more info in the logs. > > http://codereview.chromium.org/177052/diff/10001/10003 > File chrome/chrome.gyp (right): > > http://codereview.chromium.org/177052/diff/10001/10003#newcode37 > Line 37: 'browser/autocomplete/autocomplete_edit_view_uitest.cc', > Now it should be named _browsertest.cc
On Thu, Sep 3, 2009 at 3:07 AM, <suzhe@chromium.org> wrote: > Hi, > =C2=A0Thanks so much for your feedback. I'll update this CL according to > your comments asap. I'm now just wondering why it failed on Windows > trybot. Do you have any idea about it? > =C2=A0I checked existing tests and found that all tests involving key eve= nt > emulation are compiled into interactive_ui_tests rather than > browser_tests. that's not quite true, see browser/gtk/bookmark_manager_browsertest.cc > And seems that trybot does not execute > interactive_ui_tests. I'm wondering if it's necessary to move this test > into interactive_ui_tests? > > Regards > James Su > > On 2009/09/02 15:56:00, Pawe=C5=82 Hajdan Jr. wrote: >> >> http://codereview.chromium.org/177052/diff/10001/10002 >> File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc > > (right): > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode28 >> Line 28: static const int kSleepTime =3D 250; >> Please, don't use that. Now that the test is in-process, it will be > > much easier >> >> to wait properly. > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode39 >> Line 39: DCHECK(window); >> This DCHECK does more harm than good. NULL pointer dereference is very > > clear in >> >> a debugger. And it potentially terminates the entire binary, which is > > *very* bad >> >> in case of a test. If you need, use ASSERT and possibly > > HasFatalFailure. > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode45 >> Line 45: DCHECK(native_window); >> Same as above. > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode51 >> Line 51: DCHECK(loc_bar); >> Here too. > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode53 >> Line 53: DCHECK(edit_view); >> And here. > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode85 >> Line 85: void CheckQueryInProgressTask() { >> That's bad. To fix that, please add a new NotificationType if there is > > not one >> >> suitable, and fire the notification in appropriate place in the code > > being >> >> tested. Then wait for the notification in the test. > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode110 >> Line 110: ui_controls::SendKeyPressNotifyWhenDone(window, key, > > control, shift, >> >> alt, >> OK, that waits properly! > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode130 >> Line 130: DCHECK(model); >> ASSERT_TRUE(model) seems more suited for a test. > >> http://codereview.chromium.org/177052/diff/10001/10002#newcode160 >> Line 160: ASSERT_EQ(tab_count + 1, browser()->tab_count()); >> I think it should be safe to change these to EXPECT_ instead of > > ASSERT_. >> >> Potentially gives more info in the logs. > >> http://codereview.chromium.org/177052/diff/10001/10003 >> File chrome/chrome.gyp (right): > >> http://codereview.chromium.org/177052/diff/10001/10003#newcode37 >> Line 37: 'browser/autocomplete/autocomplete_edit_view_uitest.cc', >> Now it should be named _browsertest.cc > > > > http://codereview.chromium.org/177052 >
Hi,
I'm still struggling with the Windows support. Now these tests can run on my
windows machine, but they always fail on windows trybot. And when running on my
windows machine, they sometimes may fail with following error message:
[2820:5668:0910/131457:263873000:FATAL:navigation_controller.cc(492)] Check
failed: !GetActiveEntry(). Got an invalid page ID but we seem to be navigated
to a valid page. This should be impossible.
c:\chromium\src\base/test_suite.h(108): error: Failed
[2820:5668:0910/131457:263873000:FATAL:navigation_controller.cc(492)] Check
failed: !GetActiveEntry(). Got an invalid page ID but we seem to be navigated
to a valid page. This should be impossible.
Backtrace:
StackTrace::StackTrace [0x1029B651+33]
(c:\chromium\src\base\debug_util_win.cc:226)
logging::LogMessage::~LogMessage [0x1025DDFA+618]
(c:\chromium\src\base\logging.cc:540)
NavigationController::ClassifyNavigation [0x1055B094+180]
(c:\chromium\src\chrome\browser\tab_contents\navigation_controller.cc:494)
NavigationController::RendererDidNavigate [0x1055ACD6+198]
(c:\chromium\src\chrome\browser\tab_contents\navigation_controller.cc:424)
TabContents::DidNavigate [0x1062DF98+248]
(c:\chromium\src\chrome\browser\tab_contents\tab_contents.cc:1953)
RenderViewHost::OnMsgNavigate [0x105710EB+459]
(c:\chromium\src\chrome\browser\renderer_host\render_view_host.cc:955)
Do you have any idea about these issues?
Regards
James Su
On 2009/09/03 18:38:21, Evan Stade wrote:
> On Thu, Sep 3, 2009 at 3:07 AM, <mailto:suzhe@chromium.org> wrote:
> > Hi,
> > =C2=A0Thanks so much for your feedback. I'll update this CL according to
> > your comments asap. I'm now just wondering why it failed on Windows
> > trybot. Do you have any idea about it?
> > =C2=A0I checked existing tests and found that all tests involving key eve=
> nt
> > emulation are compiled into interactive_ui_tests rather than
> > browser_tests.
>
> that's not quite true, see browser/gtk/bookmark_manager_browsertest.cc
>
> > And seems that trybot does not execute
> > interactive_ui_tests. I'm wondering if it's necessary to move this test
> > into interactive_ui_tests?
> >
> > Regards
> > James Su
> >
> > On 2009/09/02 15:56:00, Pawe=C5=82 Hajdan Jr. wrote:
> >>
> >> http://codereview.chromium.org/177052/diff/10001/10002
> >> File chrome/browser/autocomplete/autocomplete_edit_view_uitest.cc
> >
> > (right):
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode28
> >> Line 28: static const int kSleepTime =3D 250;
> >> Please, don't use that. Now that the test is in-process, it will be
> >
> > much easier
> >>
> >> to wait properly.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode39
> >> Line 39: DCHECK(window);
> >> This DCHECK does more harm than good. NULL pointer dereference is very
> >
> > clear in
> >>
> >> a debugger. And it potentially terminates the entire binary, which is
> >
> > *very* bad
> >>
> >> in case of a test. If you need, use ASSERT and possibly
> >
> > HasFatalFailure.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode45
> >> Line 45: DCHECK(native_window);
> >> Same as above.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode51
> >> Line 51: DCHECK(loc_bar);
> >> Here too.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode53
> >> Line 53: DCHECK(edit_view);
> >> And here.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode85
> >> Line 85: void CheckQueryInProgressTask() {
> >> That's bad. To fix that, please add a new NotificationType if there is
> >
> > not one
> >>
> >> suitable, and fire the notification in appropriate place in the code
> >
> > being
> >>
> >> tested. Then wait for the notification in the test.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode110
> >> Line 110: ui_controls::SendKeyPressNotifyWhenDone(window, key,
> >
> > control, shift,
> >>
> >> alt,
> >> OK, that waits properly!
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode130
> >> Line 130: DCHECK(model);
> >> ASSERT_TRUE(model) seems more suited for a test.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10002#newcode160
> >> Line 160: ASSERT_EQ(tab_count + 1, browser()->tab_count());
> >> I think it should be safe to change these to EXPECT_ instead of
> >
> > ASSERT_.
> >>
> >> Potentially gives more info in the logs.
> >
> >> http://codereview.chromium.org/177052/diff/10001/10003
> >> File chrome/chrome.gyp (right):
> >
> >> http://codereview.chromium.org/177052/diff/10001/10003#newcode37
> >> Line 37: 'browser/autocomplete/autocomplete_edit_view_uitest.cc',
> >> Now it should be named _browsertest.cc
> >
> >
> >
> > http://codereview.chromium.org/177052
> >
+pkasting
You should ask Brett about your NavigationController issues, he knows that code best. http://codereview.chromium.org/177052/diff/14005/20002 File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/177052/diff/14005/20002#newcode105 Line 105: AutocompletePopupModel* popup_model() const { return popup_; } Don't add this. You should go get the popup model from whoever actually owns it. http://codereview.chromium.org/177052/diff/14005/20001 File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right): http://codereview.chromium.org/177052/diff/14005/20001#newcode121 Line 121: printf("SendKey: 0x%02x, ctrl:%d, shift:%d, alt:%d\n", I thought we were supposed to use LOG statements or similar, not printf()? http://codereview.chromium.org/177052/diff/14005/20001#newcode159 Line 159: void WaitForAutocompleteResultUpdated() { This seems misleading, since you actually wait until the controller is entirely done, not just when the result is updated. http://codereview.chromium.org/177052/diff/14005/20001#newcode215 Line 215: // is "right now" or it will nondeterministically appear in the results. Doesn't the history system take care of bumping the visit times to ensure they're not the same? http://codereview.chromium.org/177052/diff/14005/20001#newcode251 Line 251: // A hack to avoid hitting issue http://crbug.com/15513 This bug claims to have been fixed. Either reopen the bug or delete this code. http://codereview.chromium.org/177052/diff/14005/20001#newcode261 Line 261: // Test if ctrl-* and alt-* accelerators are workable in omnibox. Doesn't the first accelerator defocus the omnibox, so that the rest of the accelerators aren't actually testing this? http://codereview.chromium.org/177052/diff/14005/20001#newcode286 Line 286: // Try alt-D to focus location bar. I'm not sure we intend to support alt-D on all platforms. Ctrl-L is probably a better choice. http://codereview.chromium.org/177052/diff/14005/20001#newcode340 Line 340: ASSERT_EQ(std::wstring(L""), edit_view->model()->keyword()); Nit: Use std::wstring(), not std::wstring(L""). http://codereview.chromium.org/177052/diff/14005/20001#newcode393 Line 393: // alt-Enter opens a new tab. Shouldn't you put something _in_ the address bar first? There are no guarantees it's non-empty, are there? http://codereview.chromium.org/177052/diff/14005/20001#newcode441 Line 441: while (popup_model->selected_line() != 0) { You should just fail immediately if the first line isn't already selected. The first line should _always_ be selected after typing.
Thanks a lot for your feedback. CL has been updated, and please see below my reply of some issues. Regards James Su http://codereview.chromium.org/177052/diff/14005/20002 File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/177052/diff/14005/20002#newcode105 Line 105: AutocompletePopupModel* popup_model() const { return popup_; } On 2009/09/10 18:52:15, Peter Kasting wrote: > Don't add this. You should go get the popup model from whoever actually owns > it. AutocompletePopupModel is owned by AutocompletePopupView, which is owned by AutocompleteEditView. Then in order to get popup model, AutocompleteEditView interface and each implementations need modifying to provide a method to return popup view. But seems that AutocompletePopupView is some kind of private member of AutocompleteEditView. And as AutocompleteEditModel is owned by AutocompleteEditView, I think it would be ok to retrieve popup model from AutocompleteEditModel. If you think it's not feasible, then if it's ok to add a method to AutocompleteEditView to return popup model directly? Though it also needs to modify the interface and all implementations. http://codereview.chromium.org/177052/diff/14005/20001 File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right): http://codereview.chromium.org/177052/diff/14005/20001#newcode121 Line 121: printf("SendKey: 0x%02x, ctrl:%d, shift:%d, alt:%d\n", On 2009/09/10 18:52:15, Peter Kasting wrote: > I thought we were supposed to use LOG statements or similar, not printf()? It's temporary code, will be removed. http://codereview.chromium.org/177052/diff/14005/20001#newcode159 Line 159: void WaitForAutocompleteResultUpdated() { On 2009/09/10 18:52:15, Peter Kasting wrote: > This seems misleading, since you actually wait until the controller is entirely > done, not just when the result is updated. Done. http://codereview.chromium.org/177052/diff/14005/20001#newcode215 Line 215: // is "right now" or it will nondeterministically appear in the results. On 2009/09/10 18:52:15, Peter Kasting wrote: > Doesn't the history system take care of bumping the visit times to ensure > they're not the same? AddPageWithDetails() will store the given page information into database without any modification, including the last visit time. http://codereview.chromium.org/177052/diff/14005/20001#newcode251 Line 251: // A hack to avoid hitting issue http://crbug.com/15513 On 2009/09/10 18:52:15, Peter Kasting wrote: > This bug claims to have been fixed. Either reopen the bug or delete this code. I should have mentioned issue 18372 and 18373 here. http://codereview.chromium.org/177052/diff/14005/20001#newcode261 Line 261: // Test if ctrl-* and alt-* accelerators are workable in omnibox. On 2009/09/10 18:52:15, Peter Kasting wrote: > Doesn't the first accelerator defocus the omnibox, so that the rest of the > accelerators aren't actually testing this? Done. http://codereview.chromium.org/177052/diff/14005/20001#newcode286 Line 286: // Try alt-D to focus location bar. On 2009/09/10 18:52:15, Peter Kasting wrote: > I'm not sure we intend to support alt-D on all platforms. Ctrl-L is probably a > better choice. Done. http://codereview.chromium.org/177052/diff/14005/20001#newcode340 Line 340: ASSERT_EQ(std::wstring(L""), edit_view->model()->keyword()); On 2009/09/10 18:52:15, Peter Kasting wrote: > Nit: Use std::wstring(), not std::wstring(L""). Done. http://codereview.chromium.org/177052/diff/14005/20001#newcode393 Line 393: // alt-Enter opens a new tab. On 2009/09/10 18:52:15, Peter Kasting wrote: > Shouldn't you put something _in_ the address bar first? There are no guarantees > it's non-empty, are there? Done. http://codereview.chromium.org/177052/diff/14005/20001#newcode441 Line 441: while (popup_model->selected_line() != 0) { On 2009/09/10 18:52:15, Peter Kasting wrote: > You should just fail immediately if the first line isn't already selected. The > first line should _always_ be selected after typing. I can't find any code guarantee that. Especially according to the comment of AutocompleteResult::default_match(): // Get the default match for the query (not necessarily the first). ...
Just one comment. Thanks for your effort to make the test reliable. I'm really happy reading it now. http://codereview.chromium.org/177052/diff/19004/12008 File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right): http://codereview.chromium.org/177052/diff/19004/12008#newcode241 Line 241: // http://crbug.com/18373 Please add a TODO for yourself to remove the hack when the bugs get fixed.
On 2009/09/11 07:28:43, James Su wrote: > http://codereview.chromium.org/177052/diff/14005/20002 > File chrome/browser/autocomplete/autocomplete_edit.h (right): > > http://codereview.chromium.org/177052/diff/14005/20002#newcode105 > Line 105: AutocompletePopupModel* popup_model() const { return popup_; } > On 2009/09/10 18:52:15, Peter Kasting wrote: > > Don't add this. You should go get the popup model from whoever actually owns > > it. > > AutocompletePopupModel is owned by AutocompletePopupView, which is owned by > AutocompleteEditView. Then in order to get popup model, AutocompleteEditView > interface and each implementations need modifying to provide a method to return > popup view. But seems that AutocompletePopupView is some kind of private member > of AutocompleteEditView. > > And as AutocompleteEditModel is owned by AutocompleteEditView, I think it would > be ok to retrieve popup model from AutocompleteEditModel. If you think it's not > feasible, then if it's ok to add a method to AutocompleteEditView to return > popup model directly? Though it also needs to modify the interface and all > implementations. Argh. This ownership chain is so retarded. I guess you can leave as-is, but perhaps you can put in some #ifdef so this accessor is only compiled in for the tests? At the very least you should add a comment that it should only be used by testing code. > http://codereview.chromium.org/177052/diff/14005/20001#newcode215 > Line 215: // is "right now" or it will nondeterministically appear in the > results. > On 2009/09/10 18:52:15, Peter Kasting wrote: > > Doesn't the history system take care of bumping the visit times to ensure > > they're not the same? > > AddPageWithDetails() will store the given page information into database without > any modification, including the last visit time. Hmm, then maybe you can make the intervals less than one day? I'm worried about weird effects if we get too many pages with too long of intervals (since in real life the history system regularly expires old data). Just making them a minute apart would be OK, no? > http://codereview.chromium.org/177052/diff/14005/20001#newcode441 > Line 441: while (popup_model->selected_line() != 0) { > On 2009/09/10 18:52:15, Peter Kasting wrote: > > You should just fail immediately if the first line isn't already selected. > The > > first line should _always_ be selected after typing. > > I can't find any code guarantee that. Especially according to the comment of > AutocompleteResult::default_match(): > > // Get the default match for the query (not necessarily the first). ... Trust me, it needs to be guaranteed. The comment you're seeing applies to the case when the user has a manually selected match, which is not true just after we've run a query.
http://codereview.chromium.org/177052/diff/19004/12008 File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right): http://codereview.chromium.org/177052/diff/19004/12008#newcode99 Line 99: SetInitialTimeoutInMS(120000); What is this? Timeout values always raise my "flakiness" alarm bells. http://codereview.chromium.org/177052/diff/19004/12008#newcode508 Line 508: } Nit: File seemingly does not end with trailing LF.
thanks for writing these tests! http://codereview.chromium.org/177052/diff/19004/12008 File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right): http://codereview.chromium.org/177052/diff/19004/12008#newcode252 Line 252: // See issue 19193: omnibox blocks ctrl-* commands linkify this http://codereview.chromium.org/177052/diff/19004/12008#newcode420 Line 420: // See issue 20934: Omnibox keyboard behavior wrong for instead of "issue 20934" can you put "http://crbug.com/20934"? http://codereview.chromium.org/177052/diff/19004/12008#newcode508 Line 508: } On 2009/09/11 16:05:25, Peter Kasting wrote: > Nit: File seemingly does not end with trailing LF. New files always do that on rietveld. The linter is just broken.
Hi, CL has been updated according to your feedback. And I moved this test into interactive_ui_tests, according to Jay Campan's suggestion. Regards James Su
LGTM, feel free to ignore this nit. http://codereview.chromium.org/177052/diff/17009/17011 File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/177052/diff/17009/17011#newcode105 Line 105: // It should only be used by testing code. nit: Why not #ifdef UNIT_TEST then?
LGTM, I agree with Pawel's nit http://codereview.chromium.org/177052/diff/17009/17010 File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right): http://codereview.chromium.org/177052/diff/17009/17010#newcode242 Line 242: // TODO(suzhe): Remove ths hack as soon as these bugs are fixed. Nit: ths -> this
Thanks a lot. CL submitted with these changes. Regards James Su http://codereview.chromium.org/177052/diff/17009/17011 File chrome/browser/autocomplete/autocomplete_edit.h (right): http://codereview.chromium.org/177052/diff/17009/17011#newcode105 Line 105: // It should only be used by testing code. On 2009/09/14 16:48:41, Paweł Hajdan Jr. wrote: > nit: Why not #ifdef UNIT_TEST then? Done. http://codereview.chromium.org/177052/diff/17009/17010 File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right): http://codereview.chromium.org/177052/diff/17009/17010#newcode242 Line 242: // TODO(suzhe): Remove ths hack as soon as these bugs are fixed. On 2009/09/14 17:54:57, Peter Kasting wrote: > Nit: ths -> this Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
