Make AutomationProxyTest.NavigateToURLWithTimeout* tests not-flaky.

Make AutomationProxyTest.NavigateToURLWithTimeout* tests not-flaky.

I used a mock url request job which is guaranteed to finish after the timeout the test uses.

TEST=none
http://crbug.com/16462

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21668

[huanr, 2009-07-23 04:23:04]

I think you need to add URLRequestSlowHTTPJob to SetFilteredInetTask::Run(). See
code path of 

AutomationProxy::SetFilteredInet()
-> AutomationMsg_SetFilteredInet 
-> AutomationProvider::SetFilteredInet()

[Paweł Hajdan Jr., 2009-07-23 15:53:26]

On 2009/07/23 04:23:04, huanr wrote:
> I think you need to add URLRequestSlowHTTPJob to SetFilteredInetTask::Run().

Done.

[huanr, 2009-07-23 19:06:28]

LGTM. Thanks for the change.

[Paweł Hajdan Jr., 2009-07-24 00:32:12]

It turns out the sleep in this change isn't called. I should have checked
that.
However, while investigating this, it seems that URLRequestFilter is not
used to intercept the requests (?). I'm probably overlooking something.

phajdan@harrowdale:/chromium/src$ git grep URLRequestFilter
chrome/browser/automation/automation_provider.cc:
 URLRequestFilter::GetInstance()->ClearHandlers();
chrome/browser/automation/automation_provider.cc:
 URLRequestFilter::GetInstance()->ClearHandlers();
chrome/browser/automation/url_request_failed_dns_job.cc:  URLRequestFilter*
filter = URLRequestFilter::GetInstance();
chrome/browser/automation/url_request_failed_dns_job.h:  // For UI tests:
adds the testing URLs to the URLRequestFilter.
chrome/browser/automation/url_request_mock_http_job.cc:  // Add
kMockHostname to URLRequestFilter.
chrome/browser/automation/url_request_mock_http_job.cc:  URLRequestFilter*
filter = URLRequestFilter::GetInstance();
chrome/browser/automation/url_request_mock_http_job.h:  // For UI tests:
adds the testing URLs to the URLRequestFilter.
chrome/browser/automation/url_request_mock_net_error_job.cc:
 URLRequestFilter::GetInstance()
chrome/browser/automation/url_request_mock_net_error_job.cc:
 URLRequestFilter::GetInstance()->RemoveUrlHandler(url);
chrome/browser/automation/url_request_slow_download_job.cc:
 URLRequestFilter* filter = URLRequestFilter::GetInstance();
chrome/browser/automation/url_request_slow_download_job.h:  // For UI tests:
adds the testing URLs to the URLRequestFilter.
Binary file chrome/tools/test/reference_build/chrome/chrome_dll.pdb matches
Binary file chrome/tools/test/reference_build/chrome_linux/chrome matches
net/url_request/url_request_filter.cc:URLRequestFilter*
URLRequestFilter::shared_instance_ = NULL;
net/url_request/url_request_filter.cc:URLRequestFilter*
URLRequestFilter::GetInstance() {
net/url_request/url_request_filter.cc:    shared_instance_ = new
URLRequestFilter;
net/url_request/url_request_filter.cc:URLRequestJob*
URLRequestFilter::Factory(URLRequest* request,
net/url_request/url_request_filter.cc:void
URLRequestFilter::AddHostnameHandler(const std::string& scheme,
net/url_request/url_request_filter.cc:
 &URLRequestFilter::Factory);
net/url_request/url_request_filter.cc:void
URLRequestFilter::RemoveHostnameHandler(const std::string& scheme,
net/url_request/url_request_filter.cc:  // would left no protocol factory
for the scheme.  URLRequestFilter::Factory
net/url_request/url_request_filter.cc:bool
URLRequestFilter::AddUrlHandler(const GURL& url,
net/url_request/url_request_filter.cc:
 &URLRequestFilter::Factory);
net/url_request/url_request_filter.cc:void
URLRequestFilter::RemoveUrlHandler(const GURL& url) {
net/url_request/url_request_filter.cc:  // would left no protocol factory
for the scheme.  URLRequestFilter::Factory
net/url_request/url_request_filter.cc:void URLRequestFilter::ClearHandlers()
{
net/url_request/url_request_filter.cc:URLRequestJob*
URLRequestFilter::FindRequestHandler(URLRequest* request,
net/url_request/url_request_filter.h://
URLRequest::RegisterProtocolFactory("http", &URLRequestFilter::Factory);
net/url_request/url_request_filter.h://
URLRequestFilter::GetInstance()->AddUrlHandler(
net/url_request/url_request_filter.h:// If URLRequestFilter::Factory can't
find a handle for the request, it passes
net/url_request/url_request_filter.h:class URLRequestFilter {
net/url_request/url_request_filter.h:  static URLRequestFilter*
GetInstance();
net/url_request/url_request_filter.h:  URLRequestFilter() { }
net/url_request/url_request_filter.h:  static URLRequestFilter*
shared_instance_;
net/url_request/url_request_filter.h:
 DISALLOW_EVIL_CONSTRUCTORS(URLRequestFilter);
webkit/tools/test_shell/test_shell.cc:    URLRequestFilter* filter =
URLRequestFilter::GetInstance();

Who is a good person to ask about URLRequestFilter? wtc?

On Thu, Jul 23, 2009 at 12:06, <huanr@chromium.org> wrote:

> LGTM. Thanks for the change.
>
>
> http://codereview.chromium.org/155941
>

[huanr, 2009-07-24 01:41:37]

Tony, did you make the original change on UrlRequestMockHttpJob? Can
you answer Pawel's question?

Huan

On Thu, Jul 23, 2009 at 5:31 PM, Pawe=C5=82 Hajdan
Jr.<phajdan.jr@chromium.org> wrote:
> It turns out the sleep in this change isn't called. I should have checked
> that.
> However, while investigating this, it seems that URLRequestFilter is not
> used to intercept the requests (?). I'm probably overlooking something.
> phajdan@harrowdale:/chromium/src$ git grep URLRequestFilter
> chrome/browser/automation/automation_provider.cc:
> =C2=A0URLRequestFilter::GetInstance()->ClearHandlers();
> chrome/browser/automation/automation_provider.cc:
> =C2=A0URLRequestFilter::GetInstance()->ClearHandlers();
> chrome/browser/automation/url_request_failed_dns_job.cc: =C2=A0URLRequest=
Filter*
> filter =3D URLRequestFilter::GetInstance();
> chrome/browser/automation/url_request_failed_dns_job.h: =C2=A0// For UI t=
ests:
> adds the testing URLs to the URLRequestFilter.
> chrome/browser/automation/url_request_mock_http_job.cc: =C2=A0// Add
> kMockHostname to URLRequestFilter.
> chrome/browser/automation/url_request_mock_http_job.cc: =C2=A0URLRequestF=
ilter*
> filter =3D URLRequestFilter::GetInstance();
> chrome/browser/automation/url_request_mock_http_job.h: =C2=A0// For UI te=
sts:
> adds the testing URLs to the URLRequestFilter.
> chrome/browser/automation/url_request_mock_net_error_job.cc:
> =C2=A0URLRequestFilter::GetInstance()
> chrome/browser/automation/url_request_mock_net_error_job.cc:
> =C2=A0URLRequestFilter::GetInstance()->RemoveUrlHandler(url);
> chrome/browser/automation/url_request_slow_download_job.cc:
> =C2=A0URLRequestFilter* filter =3D URLRequestFilter::GetInstance();
> chrome/browser/automation/url_request_slow_download_job.h: =C2=A0// For U=
I tests:
> adds the testing URLs to the URLRequestFilter.
> Binary file chrome/tools/test/reference_build/chrome/chrome_dll.pdb match=
es
> Binary file chrome/tools/test/reference_build/chrome_linux/chrome matches
> net/url_request/url_request_filter.cc:URLRequestFilter*
> URLRequestFilter::shared_instance_ =3D NULL;
> net/url_request/url_request_filter.cc:URLRequestFilter*
> URLRequestFilter::GetInstance() {
> net/url_request/url_request_filter.cc: =C2=A0 =C2=A0shared_instance_ =3D =
new
> URLRequestFilter;
> net/url_request/url_request_filter.cc:URLRequestJob*
> URLRequestFilter::Factory(URLRequest* request,
> net/url_request/url_request_filter.cc:void
> URLRequestFilter::AddHostnameHandler(const std::string& scheme,
> net/url_request/url_request_filter.cc:
> =C2=A0&URLRequestFilter::Factory);
> net/url_request/url_request_filter.cc:void
> URLRequestFilter::RemoveHostnameHandler(const std::string& scheme,
> net/url_request/url_request_filter.cc: =C2=A0// would left no protocol fa=
ctory
> for the scheme. =C2=A0URLRequestFilter::Factory
> net/url_request/url_request_filter.cc:bool
> URLRequestFilter::AddUrlHandler(const GURL& url,
> net/url_request/url_request_filter.cc:
> =C2=A0&URLRequestFilter::Factory);
> net/url_request/url_request_filter.cc:void
> URLRequestFilter::RemoveUrlHandler(const GURL& url) {
> net/url_request/url_request_filter.cc: =C2=A0// would left no protocol fa=
ctory
> for the scheme. =C2=A0URLRequestFilter::Factory
> net/url_request/url_request_filter.cc:void URLRequestFilter::ClearHandler=
s()
> {
> net/url_request/url_request_filter.cc:URLRequestJob*
> URLRequestFilter::FindRequestHandler(URLRequest* request,
> net/url_request/url_request_filter.h://
> URLRequest::RegisterProtocolFactory("http", &URLRequestFilter::Factory);
> net/url_request/url_request_filter.h://
> URLRequestFilter::GetInstance()->AddUrlHandler(
> net/url_request/url_request_filter.h:// If URLRequestFilter::Factory can'=
t
> find a handle for the request, it passes
> net/url_request/url_request_filter.h:class URLRequestFilter {
> net/url_request/url_request_filter.h: =C2=A0static URLRequestFilter*
> GetInstance();
> net/url_request/url_request_filter.h: =C2=A0URLRequestFilter() { }
> net/url_request/url_request_filter.h: =C2=A0static URLRequestFilter*
> shared_instance_;
> net/url_request/url_request_filter.h:
> =C2=A0DISALLOW_EVIL_CONSTRUCTORS(URLRequestFilter);
> webkit/tools/test_shell/test_shell.cc: =C2=A0 =C2=A0URLRequestFilter* fil=
ter =3D
> URLRequestFilter::GetInstance();
> Who is a good person to ask about URLRequestFilter? wtc?
> On Thu, Jul 23, 2009 at 12:06, <huanr@chromium.org> wrote:
>>
>> LGTM. Thanks for the change.
>>
>> http://codereview.chromium.org/155941
>
>

[tony, 2009-07-24 17:22:42]

If you're going to use the mock requests you need to call
automation()->SetFilteredInet(true) which will enable the filtering.  Sorry this
isn't documented better, maybe you can add a comment to the
chrome/browser/automation code somewhere explaining this?

[Paweł Hajdan Jr., 2009-07-24 19:46:16]

It seems to have no effect. The request is still not being filtered. And it
seems that the tests don't use it directly:
phajdan@harrowdale:/chromium/src$ git grep SetFilteredInet
chrome/browser/automation/automation_provider.cc:
 IPC_MESSAGE_HANDLER(AutomationMsg_SetFilteredInet, SetFilteredInet)
chrome/browser/automation/automation_provider.cc:class SetFilteredInetTask :
public Task {
chrome/browser/automation/automation_provider.cc:  explicit
SetFilteredInetTask(bool enabled) : enabled_(enabled) { }
chrome/browser/automation/automation_provider.cc:void
AutomationProvider::SetFilteredInet(const IPC::Message& message,
chrome/browser/automation/automation_provider.cc:      new
SetFilteredInetTask(enabled));
chrome/browser/automation/automation_provider.h:  void SetFilteredInet(const
IPC::Message& message, bool enabled);
chrome/test/automation/automation_messages_internal.h:
 IPC_MESSAGE_ROUTED1(AutomationMsg_SetFilteredInet,
chrome/test/automation/automation_proxy.cc:bool
AutomationProxy::SetFilteredInet(bool enabled) {
chrome/test/automation/automation_proxy.cc:  return Send(new
AutomationMsg_SetFilteredInet(0, enabled));
chrome/test/automation/automation_proxy.h:  bool SetFilteredInet(bool
enabled);
chrome/test/ui/ui_test.cc:  automation()->SetFilteredInet(true);
chrome/test/ui/ui_test.cc:    automation()->SetFilteredInet(false);
Binary file chrome/tools/test/reference_build/chrome/chrome.dll matches
Binary file chrome/tools/test/reference_build/chrome/chrome_dll.pdb matches
Binary file chrome/tools/test/reference_build/chrome_linux/chrome matches
Binary file
chrome/tools/test/reference_build/chrome_mac/Chromium.app/Contents/MacOS/Chromium
matches
Binary file chrome/tools/test/reference_build/chrome_mac/Google
Chrome.app/Contents/MacOS/Google Chrome matches

Please give me some pointers, because investigating URLRequestFilter leads
to this:

phajdan@harrowdale:/chromium/src$ git grep URLRequestFilter
chrome/browser/automation/automation_provider.cc:
 URLRequestFilter::GetInstance()->ClearHandlers();
chrome/browser/automation/automation_provider.cc:
 URLRequestFilter::GetInstance()->ClearHandlers();
chrome/browser/automation/url_request_failed_dns_job.cc:  URLRequestFilter*
filter = URLRequestFilter::GetInstance();
chrome/browser/automation/url_request_failed_dns_job.h:  // For UI tests:
adds the testing URLs to the URLRequestFilter.
chrome/browser/automation/url_request_mock_http_job.cc:  // Add
kMockHostname to URLRequestFilter.
chrome/browser/automation/url_request_mock_http_job.cc:  URLRequestFilter*
filter = URLRequestFilter::GetInstance();
chrome/browser/automation/url_request_mock_http_job.h:  // For UI tests:
adds the testing URLs to the URLRequestFilter.
chrome/browser/automation/url_request_mock_net_error_job.cc:
 URLRequestFilter::GetInstance()
chrome/browser/automation/url_request_mock_net_error_job.cc:
 URLRequestFilter::GetInstance()->RemoveUrlHandler(url);
chrome/browser/automation/url_request_slow_download_job.cc:
 URLRequestFilter* filter = URLRequestFilter::GetInstance();
chrome/browser/automation/url_request_slow_download_job.h:  // For UI tests:
adds the testing URLs to the URLRequestFilter.
chrome/browser/automation/url_request_slow_http_job.cc:  // Add
kMockHostname to URLRequestFilter.
chrome/browser/automation/url_request_slow_http_job.cc:  URLRequestFilter*
filter = URLRequestFilter::GetInstance();
chrome/browser/automation/url_request_slow_http_job.h:  // For UI tests:
adds the testing URLs to the URLRequestFilter.
Binary file chrome/tools/test/reference_build/chrome/chrome_dll.pdb matches
Binary file chrome/tools/test/reference_build/chrome_linux/chrome matches
net/url_request/url_request_filter.cc:URLRequestFilter*
URLRequestFilter::shared_instance_ = NULL;
net/url_request/url_request_filter.cc:URLRequestFilter*
URLRequestFilter::GetInstance() {
net/url_request/url_request_filter.cc:    shared_instance_ = new
URLRequestFilter;
net/url_request/url_request_filter.cc:URLRequestJob*
URLRequestFilter::Factory(URLRequest* request,
net/url_request/url_request_filter.cc:void
URLRequestFilter::AddHostnameHandler(const std::string& scheme,
net/url_request/url_request_filter.cc:
 &URLRequestFilter::Factory);
net/url_request/url_request_filter.cc:void
URLRequestFilter::RemoveHostnameHandler(const std::string& scheme,
net/url_request/url_request_filter.cc:  // would left no protocol factory
for the scheme.  URLRequestFilter::Factory
net/url_request/url_request_filter.cc:bool
URLRequestFilter::AddUrlHandler(const GURL& url,
net/url_request/url_request_filter.cc:
 &URLRequestFilter::Factory);
net/url_request/url_request_filter.cc:void
URLRequestFilter::RemoveUrlHandler(const GURL& url) {
net/url_request/url_request_filter.cc:  // would left no protocol factory
for the scheme.  URLRequestFilter::Factory
net/url_request/url_request_filter.cc:void URLRequestFilter::ClearHandlers()
{
net/url_request/url_request_filter.cc:URLRequestJob*
URLRequestFilter::FindRequestHandler(URLRequest* request,
net/url_request/url_request_filter.h://
URLRequest::RegisterProtocolFactory("http", &URLRequestFilter::Factory);
net/url_request/url_request_filter.h://
URLRequestFilter::GetInstance()->AddUrlHandler(
net/url_request/url_request_filter.h:// If URLRequestFilter::Factory can't
find a handle for the request, it passes
net/url_request/url_request_filter.h:class URLRequestFilter {
net/url_request/url_request_filter.h:  static URLRequestFilter*
GetInstance();
net/url_request/url_request_filter.h:  URLRequestFilter() { }
net/url_request/url_request_filter.h:  static URLRequestFilter*
shared_instance_;
net/url_request/url_request_filter.h:
 DISALLOW_EVIL_CONSTRUCTORS(URLRequestFilter);
webkit/tools/test_shell/test_shell.cc:    URLRequestFilter* filter =
URLRequestFilter::GetInstance();

(I couldn't find a place where anything asks URLRequestFilter to perform the
substitution).

On Fri, Jul 24, 2009 at 10:22, <tony@chromium.org> wrote:

> If you're going to use the mock requests you need to call
> automation()->SetFilteredInet(true) which will enable the filtering.
> Sorry this isn't documented better, maybe you can add a comment to the
> chrome/browser/automation code somewhere explaining this?
>
>
> http://codereview.chromium.org/155941
>

[Paweł Hajdan Jr., 2009-07-24 22:01:57]

So, finally made it working as intended. I increased the timeout in the test, so
that the browser really tries to request the URL, and doesn't timeout even
before creating a URLRequest.

I had to move the sleep to the factory. The method where it was earlier wasn't
being called.

Tony: Please review these changes, especially this sleep in the factory.

[tony, 2009-07-24 22:15:04]

http://codereview.chromium.org/155941/diff/2006/2010
File chrome/browser/automation/url_request_slow_http_job.cc (right):

http://codereview.chromium.org/155941/diff/2006/2010#newcode21
Line 21: PlatformThread::Sleep(kDelayMs);
Using a sleep seems scary because it sleeps the whole network thread.  Is it
possible to instead override GetMoreData and ReadRawData and act as if there's
more data pending until time::now() + timedelta(kSleepMs) has passed?

[Paweł Hajdan Jr., 2009-07-27 17:14:32]

I removed that Sleep. I used a timer, because the method with GetMoreData and
ReadRawData didn't work (they weren't being called).

[tony, 2009-07-27 17:22:13]

LG if you're able to remove the call to Sleep.

http://codereview.chromium.org/155941/diff/2019/1017
File chrome/browser/automation/url_request_slow_http_job.cc (right):

http://codereview.chromium.org/155941/diff/2019/1017#newcode24
Line 24: PlatformThread::Sleep(kDelayMs);
Do you still need this?

[huanr, 2009-07-27 17:29:47]

Looks like it is needed to make the job become "slow http". Pawel, to
eliminate sleep, you probably need a timer task here.

Huan

On Mon, Jul 27, 2009 at 10:22 AM, <tony@chromium.org> wrote:
> LG if you're able to remove the call to Sleep.
>
>
> http://codereview.chromium.org/155941/diff/2019/1017
> File chrome/browser/automation/url_request_slow_http_job.cc (right):
>
> http://codereview.chromium.org/155941/diff/2019/1017#newcode24
> Line 24: PlatformThread::Sleep(kDelayMs);
> Do you still need this?
>
> http://codereview.chromium.org/155941
>

[Paweł Hajdan Jr., 2009-07-27 17:57:55]

Oops, I forgot to remove the call to Sleep. Should be good now.

I verified that the method which adds delay is called.

[huanr, 2009-07-27 18:24:51]

LGTM. Thanks for the change!

http://codereview.chromium.org/155941/diff/2024/1031
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/155941/diff/2024/1031#newcode294
Line 294: 250, &is_timeout);
Can you add a comment here that kDelayMs = 1000?

[Paweł Hajdan Jr., 2009-07-27 18:36:13]

On 2009/07/27 18:24:51, huanr wrote:
> LGTM. Thanks for the change!
> 
> http://codereview.chromium.org/155941/diff/2024/1031
> File chrome/test/automation/automation_proxy_uitest.cc (right):
> 
> http://codereview.chromium.org/155941/diff/2024/1031#newcode294
> Line 294: 250, &is_timeout);
> Can you add a comment here that kDelayMs = 1000?

I added an ASSERT. Should be harder to overlook. What do you think?

[huanr, 2009-07-27 18:45:11]

Certainly a better change. LG

[wtc, 2009-07-29 00:38:37]

On 2009/07/24 00:32:12, Paweł Hajdan Jr. wrote:
>
> Who is a good person to ask about URLRequestFilter? wtc?

Pawel, do you still have questions about URLRequestFilter.
I am not familiar with that class.  If you have questions,
I can help you understand the code...