[sync] Allow FakeURLFetcher to return arbitrary HTTP response codes

[sync] Allow FakeURLFetcher to return arbitrary HTTP response codes

As of today, FakeURLFetcher takes a boolean "success" parameter, and
returns one of two HTTP response codes: 200 for success and 500 for
failure.

In order to write sync tests for auth errors, we need to be able to fake
arbitrary HTTP response codes.

This patch modifies FakeURLFetcher to return arbitrary HTTP response
codes and fixes all existing call sites. Tests that return arbitrary
error codes will be added in a separate CL.

R=achuith@chromium.org, ajwong@chromium.org, akalin@chromium.org, bengr@chromium.org, nyquist@chromium.org, sergeyu@chromium.org, vitalybuka@chromium.org
TBR=battre, mattm, sky, stevenjb, tim
BUG=313905
TEST=All existing tests pass trybots and waterfall

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232724

[Raghu Simha, 2013-11-01 00:06:51]

Folks, please review.

The meat of this CL is contained in
net/url_request/test_url_fetcher_factory.{h|cc}. The remaining changes are
mostly mechanical find/replace edits.

akalin: Overall review

OWNERS:
darin: chrome/browser
ajwong: chrome/service
nyquist: components/dom_distiller
bengr: components/precache
sergeyu: remoting

[Sergey Ulanov, 2013-11-01 00:12:19]

remoting - lgtm

[nyquist, 2013-11-01 16:22:12]

components/dom_distiller/ lgtm

[Raghu Simha, 2013-11-01 20:13:04]

Pinging other reviewers.

[bengr, 2013-11-01 20:16:40]

On 2013/11/01 20:13:04, Raghu Simha wrote:
> Pinging other reviewers.

components/precache/ lgtm

[awong, 2013-11-01 20:17:52]

service: lgtm

[Raghu Simha, 2013-11-01 20:49:58]

Adding more reviewers for directories under chrome/browser. Once the meat of
this CL is reviewed, I'll TBR owners of files with only minor changes.

achuith@: chrome/browser/chromeos
vitalybuka@: chrome/browser/local_discovery
mmenke@: chrome/browser/net
stevenjb@: chrome/browser/notifications
battre@: chrome/browser/profile_resetter
mattm@: chrome/browser/safe_browsing
tim@: chrome/browser/{signin/sync}
sky@: chrome/browser/ui

[achuithb, 2013-11-01 20:57:03]

lgtm for c/b/cros

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:240:
bool success) {
Any reason not to fix this param instead?

[mattm, 2013-11-01 20:57:13]

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(right):

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:265:
SetModelFetchResponse("blamodel", false /* failure */);
The tests would probably be easier to read if the set functions just took the
response code, if you don't mind changing all that.

[Vitaly Buka (NO REVIEWS), 2013-11-01 21:01:00]

lgtm with nits
chrome/service/cloud_print/
chrome/browser/local_discovery/cloud_print_printer_list_unittest.cc

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
File chrome/service/cloud_print/printer_job_handler_unittest.cc (right):

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
chrome/service/cloud_print/printer_job_handler_unittest.cc:288: d,
According
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

new net::FakeURLFetcher(url, d, response_data,
                        response_code));

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
chrome/service/cloud_print/printer_job_handler_unittest.cc:685:
factory_.SetFakeResponse(TicketURI(1),
same

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
chrome/service/cloud_print/printer_job_handler_unittest.cc:765:
StatusResponse(1, "ERROR"),
same

[mmenke, 2013-11-01 21:05:00]

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:291: URLRequestStatus::FAILED,
I don't believe this is correct.  I think we actually report SUCCESS on most
HTTP status codes - or at least I can't find code that only reports success on
2xx responses.  This is mostly used for network errors.

My suggestion is just to take a response code and a network status code.  Set
URLRequestStatus to SUCCESS for net::OK, CANCELLED for net::ERR_ABORTED, and
FAILED for everything else.

[Raghu Simha, 2013-11-01 21:55:14]

Thanks for the reviews, everyone. Feedback addressed.

mattm / mmenke: PTAL.

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc
(right):

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc:240:
bool success) {
On 2013/11/01 20:57:04, achuith.bhandarkar wrote:
> Any reason not to fix this param instead?

I only fixed calls to FakeURLFetcherFactory::SetFakeResponse during my first
pass. Makes sense to fix this too. Done.

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(right):

https://codereview.chromium.org/48713008/diff/610001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:265:
SetModelFetchResponse("blamodel", false /* failure */);
On 2013/11/01 20:57:14, mattm wrote:
> The tests would probably be easier to read if the set functions just took the
> response code, if you don't mind changing all that.

Not a problem :) Done.

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
File chrome/service/cloud_print/printer_job_handler_unittest.cc (right):

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
chrome/service/cloud_print/printer_job_handler_unittest.cc:288: d,
On 2013/11/01 21:01:02, Vitaly Buka wrote:
> According
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> 
> new net::FakeURLFetcher(url, d, response_data,
>                         response_code));

After a rebase, this all fits in one line. Fixed.

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
chrome/service/cloud_print/printer_job_handler_unittest.cc:685:
factory_.SetFakeResponse(TicketURI(1),
On 2013/11/01 21:01:02, Vitaly Buka wrote:
> same

Done.

https://codereview.chromium.org/48713008/diff/610001/chrome/service/cloud_pri...
chrome/service/cloud_print/printer_job_handler_unittest.cc:765:
StatusResponse(1, "ERROR"),
On 2013/11/01 21:01:02, Vitaly Buka wrote:
> same

Done here and the line below.

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:291: URLRequestStatus::FAILED,
On 2013/11/01 21:05:02, mmenke wrote:
> I don't believe this is correct.  I think we actually report SUCCESS on most
> HTTP status codes - or at least I can't find code that only reports success on
> 2xx responses.  This is mostly used for network errors.

Good point. The logic I was using worked because the only two codes being
returned were HTTP_OK and HTTP_INTERNAL_SERVER_ERROR.

> My suggestion is just to take a response code and a network status code.  Set
> URLRequestStatus to SUCCESS for net::OK, CANCELLED for net::ERR_ABORTED, and
> FAILED for everything else.

Given that this class is used only for testing, and returns HTTP_OK a vast
majority of the time, I'm hesitant to add an extra parameter due to the number
of call sites that need to be updated.

I searched through the code base for an existing mapping between HttpStatusCode
(net/http/http_status_code_list.h) and URLRequestStatus::Status
(net/url_request/url_request_status.h), but couldn't find any. I've changed the
logic to return FAILED for all HTTP/5xx server errors, and SUCCESS for all
HTTP/2xx, 3xx, and 4xx errors. Does this seem all right?

As of today, there's no test code in the code base that uses anything other than
HTTP_OK or HTTP_INTERNAL_SERVER_ERROR, so the new logic I suggest above will
continue to work as before. In future, I plan on adding tests that return
HTTP_UNAUTHORIZED, and will make sure URLRequestStatus::SUCCESS works in that
case.

Let me know what you think.

[akalin, 2013-11-01 22:36:58]

lgtm

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:291: URLRequestStatus::FAILED,
On 2013/11/01 21:55:16, Raghu Simha wrote:
> On 2013/11/01 21:05:02, mmenke wrote:
> > I don't believe this is correct.  I think we actually report SUCCESS on most
> > HTTP status codes - or at least I can't find code that only reports success
on
> > 2xx responses.  This is mostly used for network errors.
> 
> Good point. The logic I was using worked because the only two codes being
> returned were HTTP_OK and HTTP_INTERNAL_SERVER_ERROR.
> 
> > My suggestion is just to take a response code and a network status code. 
Set
> > URLRequestStatus to SUCCESS for net::OK, CANCELLED for net::ERR_ABORTED, and
> > FAILED for everything else.
> 
> Given that this class is used only for testing, and returns HTTP_OK a vast
> majority of the time, I'm hesitant to add an extra parameter due to the number
> of call sites that need to be updated.
> 
> I searched through the code base for an existing mapping between
HttpStatusCode
> (net/http/http_status_code_list.h) and URLRequestStatus::Status
> (net/url_request/url_request_status.h), but couldn't find any. I've changed
the
> logic to return FAILED for all HTTP/5xx server errors, and SUCCESS for all
> HTTP/2xx, 3xx, and 4xx errors. Does this seem all right?
> 
> As of today, there's no test code in the code base that uses anything other
than
> HTTP_OK or HTTP_INTERNAL_SERVER_ERROR, so the new logic I suggest above will
> continue to work as before. In future, I plan on adding tests that return
> HTTP_UNAUTHORIZED, and will make sure URLRequestStatus::SUCCESS works in that
> case.
> 
> Let me know what you think.

This sounds reasonable, but you probably should document this in a comment.

[mmenke, 2013-11-01 23:10:54]

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
File net/url_request/test_url_fetcher_factory.cc (right):

https://codereview.chromium.org/48713008/diff/610001/net/url_request/test_url...
net/url_request/test_url_fetcher_factory.cc:291: URLRequestStatus::FAILED,
On 2013/11/01 22:36:59, akalin wrote:
> On 2013/11/01 21:55:16, Raghu Simha wrote:
> > On 2013/11/01 21:05:02, mmenke wrote:
> > > I don't believe this is correct.  I think we actually report SUCCESS on
most
> > > HTTP status codes - or at least I can't find code that only reports
success
> on
> > > 2xx responses.  This is mostly used for network errors.
> > 
> > Good point. The logic I was using worked because the only two codes being
> > returned were HTTP_OK and HTTP_INTERNAL_SERVER_ERROR.
> > 
> > > My suggestion is just to take a response code and a network status code. 
> Set
> > > URLRequestStatus to SUCCESS for net::OK, CANCELLED for net::ERR_ABORTED,
and
> > > FAILED for everything else.
> > 
> > Given that this class is used only for testing, and returns HTTP_OK a vast
> > majority of the time, I'm hesitant to add an extra parameter due to the
number
> > of call sites that need to be updated.
> > 
> > I searched through the code base for an existing mapping between
> HttpStatusCode
> > (net/http/http_status_code_list.h) and URLRequestStatus::Status
> > (net/url_request/url_request_status.h), but couldn't find any. I've changed
> the
> > logic to return FAILED for all HTTP/5xx server errors, and SUCCESS for all
> > HTTP/2xx, 3xx, and 4xx errors. Does this seem all right?
> > 
> > As of today, there's no test code in the code base that uses anything other
> than
> > HTTP_OK or HTTP_INTERNAL_SERVER_ERROR, so the new logic I suggest above will
> > continue to work as before. In future, I plan on adding tests that return
> > HTTP_UNAUTHORIZED, and will make sure URLRequestStatus::SUCCESS works in
that
> > case.
> > 
> > Let me know what you think.
> 
> This sounds reasonable, but you probably should document this in a comment.

I think the point about updating callsites is valid, and complexity for those
who don't have to care about it is a valid concern as well.

However, I worry about having people test bogus behaviors here.  If consumers
expect to get failures on 5xx errors, they could end up misusing the class,
resulting in bugs.  Not sure about a reasonable solution, I'll think about it.

[Raghu Simha, 2013-11-04 15:55:01]

Thanks for the review, everyone. I'm going to land this CL as is, and will be
happy to revisit the URLRequestStatus SUCCESS / FAILED logic separately in a new
CL if we end up with a better alternative.

On 2013/11/01 22:36:58, akalin wrote:
> This sounds reasonable, but you probably should document this in a comment.

Done.

[Raghu Simha, 2013-11-04 17:34:24]

Committed patchset #5 manually as r232724 (presubmit successful).