Issue 23625015: Consolidate TestURLFetcherFactory::SetFakeResponse

Issue 23625015: Consolidate TestURLFetcherFactory::SetFakeResponse (Closed)

Created:
7 years, 3 months ago by Mattias Nissler (ping if slow)

Modified:
7 years, 2 months ago

Reviewers:
bartfab (slow), Nikita (slow), akalin, Andrew T Wilson (Slow), gene, garykac, mattm, vabr (Chromium), brettw, dewittj

CC:
chromium-reviews, skanuj+watch_chromium.org, weitaosu+watch_chromium.org, nkostylev+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, mad+watch_chromium.org, stevenjb+watch_chromium.org, tim+watch_chromium.org, dcaiafa+watch_chromium.org, cbentzel+watch_chromium.org, melevin+watch_chromium.org, dominich, amit, wez+watch_chromium.org, sanjeevr, haitaol+watch_chromium.org, jfweitz+watch_chromium.org, rmsousa+watch_chromium.org, oshima+watch_chromium.org, sergeyu+watch_chromium.org, Jered, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, donnd+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, alexeypa+watch_chromium.org, rsimha+watch_chromium.org, davemoore+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Consolidate TestURLFetcherFactory::SetFakeResponse Remove the std::string version of the function and convert all callers to the GURL version. BUG=None TBR=brettw@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225952

Patch Set 1 #

Patch Set 2 : Could have spelled it Cloud... #

Total comments: 10

Patch Set 3 : Address comments. #

Total comments: 6

Patch Set 4 : Address mistake pointed out by mattm #

Patch Set 5 : Rebase. #

Patch Set 6 : Rebase. #

Patch Set 7 : Could -> Cloud #

Total comments: 1

Patch Set 8 : Rebase. #

Patch Set 9 : Rebase. #

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+139 lines, -140 lines)			Patch
M	chrome/browser/chromeos/login/eula_browsertest.cc	View		3 chunks	+4 lines, -3 lines	0 comments	Download
M	chrome/browser/chromeos/policy/cloud_external_data_manager_base.h	View	1 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/chromeos/policy/cloud_external_data_manager_base_unittest.cc	View	1 3 4 5 6	21 chunks	+46 lines, -35 lines	0 comments	Download
M	chrome/browser/local_discovery/cloud_print_printer_list_unittest.cc	View		2 chunks	+6 lines, -5 lines	0 comments	Download
M	chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/profile_resetter/profile_resetter_unittest.cc	View		3 chunks	+3 lines, -2 lines	0 comments	Download
M	chrome/browser/safe_browsing/client_side_detection_service.h	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/safe_browsing/client_side_detection_service.cc	View	1 2 3	3 chunks	+7 lines, -8 lines	0 comments	Download
M	chrome/browser/safe_browsing/client_side_detection_service_unittest.cc	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/safe_browsing/download_protection_service.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/safe_browsing/download_protection_service.cc	View	1 2 3	2 chunks	+6 lines, -7 lines	0 comments	Download
M	chrome/browser/signin/signin_browsertest.cc	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/sync/test/integration/sync_test.cc	View		2 chunks	+7 lines, -7 lines	0 comments	Download
M	chrome/browser/ui/search/instant_extended_interactive_uitest.cc	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	chrome/service/cloud_print/printer_job_handler_unittest.cc	View	1 2 3 4 5 6 7 8	7 chunks	+38 lines, -40 lines	0 comments	Download
M	net/url_request/test_url_fetcher_factory.h	View		1 chunk	+1 line, -7 lines	0 comments	Download
M	net/url_request/test_url_fetcher_factory.cc	View		2 chunks	+1 line, -7 lines	0 comments	Download
M	remoting/host/token_validator_factory_impl_unittest.cc	View		4 chunks	+5 lines, -4 lines	0 comments	Download

Messages

Total messages: 34 (0 generated)

Expand Messages | Collapse Messages

Mattias Nissler (ping if slow)

Here's the clean up CL. Please review, I'll add additional OWNERS once I have your ...

7 years, 3 months ago (2013-09-19 15:31:25 UTC) #1

akalin

lgtm after nits https://chromiumcodereview-hr.appspot.com/23625015/diff/5001/chrome/browser/chromeos/login/eula_browsertest.cc File chrome/browser/chromeos/login/eula_browsertest.cc (right): https://chromiumcodereview-hr.appspot.com/23625015/diff/5001/chrome/browser/chromeos/login/eula_browsertest.cc#newcode22 chrome/browser/chromeos/login/eula_browsertest.cc:22: const char kEULAURL[] = may as ...

7 years, 3 months ago (2013-09-19 18:53:59 UTC) #2

akalin

On 2013/09/19 18:53:59, akalin wrote: > lgtm after nits > > https://chromiumcodereview-hr.appspot.com/23625015/diff/5001/chrome/browser/chromeos/login/eula_browsertest.cc > File chrome/browser/chromeos/login/eula_browsertest.cc ...

7 years, 3 months ago (2013-09-19 20:48:39 UTC) #3

Mattias Nissler (ping if slow)

Addressed comments other than the ones that ask for GURLs as static data, regarding which ...

7 years, 3 months ago (2013-09-20 15:33:52 UTC) #4

akalin

https://chromiumcodereview-hr.appspot.com/23625015/diff/5001/chrome/browser/chromeos/login/eula_browsertest.cc File chrome/browser/chromeos/login/eula_browsertest.cc (right): https://chromiumcodereview-hr.appspot.com/23625015/diff/5001/chrome/browser/chromeos/login/eula_browsertest.cc#newcode22 chrome/browser/chromeos/login/eula_browsertest.cc:22: const char kEULAURL[] = On 2013/09/20 15:33:53, Mattias Nissler ...

7 years, 3 months ago (2013-09-20 23:06:42 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/23625015/24001

7 years, 3 months ago (2013-09-23 09:03:31 UTC) #6

commit-bot: I haz the power

Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=26859

7 years, 3 months ago (2013-09-23 09:14:53 UTC) #7

Mattias Nissler (ping if slow)

Adding OWNERS: nkostylev@chromium.org: chrome/browser/chromeos/login gene@chromium.org: chrome/browser/local_discovery chrome/service dewittj@chromium.org: chrome/browser/notifications vabr@chromium.org: chrome/browser/profile_resetter mattm@chromium.org: chrome/browser/safe_browsing atwilson@chromium.org: chrome/browser/signin ...

7 years, 3 months ago (2013-09-23 11:56:28 UTC) #8

vabr (Chromium)

chrome/browser/profile_resetter/profile_resetter_unittest.cc LGTM Cheers, Vaclav

7 years, 3 months ago (2013-09-23 12:10:43 UTC) #9

mattm

https://chromiumcodereview-hr.appspot.com/23625015/diff/24001/chrome/browser/safe_browsing/client_side_detection_service.cc File chrome/browser/safe_browsing/client_side_detection_service.cc (right): https://chromiumcodereview-hr.appspot.com/23625015/diff/24001/chrome/browser/safe_browsing/client_side_detection_service.cc#newcode714 chrome/browser/safe_browsing/client_side_detection_service.cc:714: url = url.Resolve("?key=%s" + net::EscapeQueryParamValue(api_key, true)); remove %s https://chromiumcodereview-hr.appspot.com/23625015/diff/24001/chrome/browser/safe_browsing/download_protection_service.cc ...

7 years, 3 months ago (2013-09-23 21:36:58 UTC) #14

Mattias Nissler (ping if slow)

Nikita: friendly ping. https://chromiumcodereview-hr.appspot.com/23625015/diff/24001/chrome/browser/safe_browsing/client_side_detection_service.cc File chrome/browser/safe_browsing/client_side_detection_service.cc (right): https://chromiumcodereview-hr.appspot.com/23625015/diff/24001/chrome/browser/safe_browsing/client_side_detection_service.cc#newcode714 chrome/browser/safe_browsing/client_side_detection_service.cc:714: url = url.Resolve("?key=%s" + net::EscapeQueryParamValue(api_key, true)); ...

7 years, 3 months ago (2013-09-24 08:53:28 UTC) #15

Mattias Nissler (ping if slow)

Adding more OWNERS: brettw: chrome/browser/ui/search bartfab: chrome/browser/chromeos/policy

7 years, 2 months ago (2013-09-25 09:57:49 UTC) #18

bartfab (slow)

chrome/browser/chromeos/policy LGTM https://chromiumcodereview.appspot.com/23625015/diff/65001/chrome/browser/chromeos/policy/cloud_external_data_manager_base.h File chrome/browser/chromeos/policy/cloud_external_data_manager_base.h (right): https://chromiumcodereview.appspot.com/23625015/diff/65001/chrome/browser/chromeos/policy/cloud_external_data_manager_base.h#newcode67 chrome/browser/chromeos/policy/cloud_external_data_manager_base.h:67: friend class CloudExternalDataManagerBaseTest; Oops. Thanks for the ...

7 years, 2 months ago (2013-09-25 11:54:45 UTC) #19

akalin

oops forgot to send out comments https://codereview.chromium.org/23625015/diff/24001/chrome/browser/safe_browsing/download_protection_service.cc File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/23625015/diff/24001/chrome/browser/safe_browsing/download_protection_service.cc#newcode974 chrome/browser/safe_browsing/download_protection_service.cc:974: url = url.Resolve("?key=%s" ...

7 years, 2 months ago (2013-09-25 21:35:37 UTC) #20

akalin

still lgtm i think at this point you can just tbr the remaining owners

7 years, 2 months ago (2013-09-25 21:38:44 UTC) #21

Mattias Nissler (ping if slow)

brettw to TBR, checking the box. https://codereview.chromium.org/23625015/diff/24001/chrome/browser/safe_browsing/download_protection_service.cc File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/23625015/diff/24001/chrome/browser/safe_browsing/download_protection_service.cc#newcode974 chrome/browser/safe_browsing/download_protection_service.cc:974: url = url.Resolve("?key=%s" ...

7 years, 2 months ago (2013-09-26 08:48:08 UTC) #22

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/23625015/79001

7 years, 2 months ago (2013-09-26 08:51:29 UTC) #23

commit-bot: I haz the power

Failed to apply patch for chrome/service/cloud_print/printer_job_handler_unittest.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 2 months ago (2013-09-26 08:51:40 UTC) #24

Failed to apply patch for
chrome/service/cloud_print/printer_job_handler_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/service/cloud_print/printer_job_handler_unittest.cc
  Hunk #2 FAILED at 203.
  Hunk #3 succeeded at 557 (offset 3 lines).
  Hunk #4 succeeded at 662 (offset 3 lines).
  Hunk #5 succeeded at 681 (offset 3 lines).
  Hunk #6 succeeded at 713 (offset 3 lines).
  Hunk #7 succeeded at 758 (offset 3 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
chrome/service/cloud_print/printer_job_handler_unittest.cc.rej

Patch:       chrome/service/cloud_print/printer_job_handler_unittest.cc
Index: chrome/service/cloud_print/printer_job_handler_unittest.cc
diff --git a/chrome/service/cloud_print/printer_job_handler_unittest.cc
b/chrome/service/cloud_print/printer_job_handler_unittest.cc
index
13736d3afa02aec90a515deba64c04e44d5fafcd..4056903e9a95af1430e730007ac8ec42b93e9229
100644
--- a/chrome/service/cloud_print/printer_job_handler_unittest.cc
+++ b/chrome/service/cloud_print/printer_job_handler_unittest.cc
@@ -22,17 +22,17 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::AtLeast;
+using ::testing::DoAll;
 using ::testing::Exactly;
-using ::testing::Sequence;
+using ::testing::Invoke;
+using ::testing::InvokeWithoutArgs;
+using ::testing::NiceMock;
 using ::testing::Return;
 using ::testing::SaveArg;
-using ::testing::DoAll;
-using ::testing::_;
-using ::testing::NiceMock;
-using ::testing::StrictMock;
-using ::testing::Invoke;
+using ::testing::Sequence;
 using ::testing::SetArgPointee;
-using ::testing::InvokeWithoutArgs;
+using ::testing::StrictMock;
+using ::testing::_;
 
 namespace cloud_print {
 
@@ -203,35 +203,34 @@ std::string JobListResponse(int num_jobs) {
   return StringPrintf(kExampleJobListResponse, job_objects.c_str());
 }
 
-std::string JobListURI(const char* reason) {
-  return StringPrintf(kExamplePrinterJobListURI, reason);
+GURL JobListURI(const char* reason) {
+  return GURL(StringPrintf(kExamplePrinterJobListURI, reason));
 }
 
 std::string JobID(int job_num) {
   return StringPrintf(kExampleJobID, job_num);
 }
 
-std::string DoneURI(int job_num) {
-  return StringPrintf(kExampleUpdateDoneURI, job_num);
+GURL DoneURI(int job_num) {
+  return GURL(StringPrintf(kExampleUpdateDoneURI, job_num));
 }
 
-std::string ErrorURI(int job_num) {
-  return StringPrintf(kExampleUpdateErrorURI, job_num);
+GURL ErrorURI(int job_num) {
+  return GURL(StringPrintf(kExampleUpdateErrorURI, job_num));
 }
 
-std::string TicketURI(int job_num) {
-  return StringPrintf(kExamplePrintTicketURI, job_num);
+GURL TicketURI(int job_num) {
+  return GURL(StringPrintf(kExamplePrintTicketURI, job_num));
 }
 
-std::string DownloadURI(int job_num) {
-  return StringPrintf(kExamplePrintDownloadURI, job_num);
+GURL DownloadURI(int job_num) {
+  return GURL(StringPrintf(kExamplePrintDownloadURI, job_num));
 }
 
-// converts to string for consistency
-std::string InProgressURI(int job_num) {
+GURL InProgressURI(int job_num) {
   return GetUrlForJobStatusUpdate(GURL(kExampleCloudPrintServerURL),
                                   StringPrintf(kExampleJobID, job_num),
-                                  PRINT_JOB_STATUS_IN_PROGRESS).spec();
+                                  PRINT_JOB_STATUS_IN_PROGRESS);
 }
 
 std::string StatusResponse(int job_num, const char* status_string) {
@@ -554,18 +553,17 @@ void PrinterJobHandlerTest::SetUpJobSuccessTest(int
job_num) {
 
   // The times requirement is relaxed for the ticket URI
   // in order to accommodate TicketDownloadFailureTest
-  EXPECT_CALL(url_callback_, OnRequestCreate(
-      GURL(TicketURI(job_num)), _))
+  EXPECT_CALL(url_callback_, OnRequestCreate(TicketURI(job_num), _))
       .Times(AtLeast(1));
 
-  EXPECT_CALL(url_callback_, OnRequestCreate(GURL(DownloadURI(job_num)), _))
+  EXPECT_CALL(url_callback_, OnRequestCreate(DownloadURI(job_num), _))
       .Times(Exactly(1))
       .WillOnce(Invoke(this, &PrinterJobHandlerTest::AddMimeHeader));
 
-  EXPECT_CALL(url_callback_, OnRequestCreate(GURL(InProgressURI(job_num)), _))
+  EXPECT_CALL(url_callback_, OnRequestCreate(InProgressURI(job_num), _))
       .Times(Exactly(1));
 
-  EXPECT_CALL(url_callback_, OnRequestCreate(GURL(DoneURI(job_num)), _))
+  EXPECT_CALL(url_callback_, OnRequestCreate(DoneURI(job_num), _))
       .Times(Exactly(1));
 
   EXPECT_CALL(print_system_->JobSpooler(),
@@ -660,10 +658,10 @@ TEST_F(PrinterJobHandlerTest, DISABLED_HappyPathTest) {
                            JobListResponse(0), true);
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonStartup)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonStartup), _))
       .Times(Exactly(1));
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonQueryMore)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonQueryMore), _))
       .Times(Exactly(1));
 
   SetUpJobSuccessTest(1);
@@ -679,19 +677,19 @@ TEST_F(PrinterJobHandlerTest, TicketDownloadFailureTest) {
                            JobListResponse(0), true);
   factory_.SetFakeResponse(TicketURI(1), std::string(), false);
 
-  EXPECT_CALL(url_callback_, OnRequestCreate(GURL(TicketURI(1)), _))
+  EXPECT_CALL(url_callback_, OnRequestCreate(TicketURI(1), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonStartup)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonStartup), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonQueryMore)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonQueryMore), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonFailure)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonFailure), _))
       .Times(AtLeast(1));
 
   SetUpJobSuccessTest(2);
@@ -711,19 +709,19 @@ TEST_F(PrinterJobHandlerTest, DISABLED_ManyFailureTest) {
                            JobListResponse(0), true);
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonStartup)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonStartup), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonQueryMore)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonQueryMore), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonFailure)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonFailure), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonRetry)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonRetry), _))
       .Times(AtLeast(1));
 
   SetUpJobSuccessTest(1);
@@ -756,23 +754,23 @@ TEST_F(PrinterJobHandlerTest,
DISABLED_CompleteFailureTest) {
   factory_.SetFakeResponse(TicketURI(1), std::string(), false);
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonStartup)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonStartup), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonFailure)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonFailure), _))
       .Times(AtLeast(1));
 
   EXPECT_CALL(url_callback_,
-              OnRequestCreate(GURL(JobListURI(kJobFetchReasonRetry)), _))
+              OnRequestCreate(JobListURI(kJobFetchReasonRetry), _))
       .Times(AtLeast(1));
 
-  EXPECT_CALL(url_callback_, OnRequestCreate(GURL(ErrorURI(1)), _))
+  EXPECT_CALL(url_callback_, OnRequestCreate(ErrorURI(1), _))
       .Times(Exactly(1))
       .WillOnce(InvokeWithoutArgs(
           this, &PrinterJobHandlerTest::MakeJobFetchReturnNoJobs));
 
-  EXPECT_CALL(url_callback_, OnRequestCreate(GURL(TicketURI(1)), _))
+  EXPECT_CALL(url_callback_, OnRequestCreate(TicketURI(1), _))
       .Times(AtLeast(kNumRetriesBeforeAbandonJob));
 
   BeginTest(70);

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/23625015/83001

7 years, 2 months ago (2013-09-26 12:07:01 UTC) #25

commit-bot: I haz the power

Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=81665

7 years, 2 months ago (2013-09-26 15:11:13 UTC) #26

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/23625015/83001

7 years, 2 months ago (2013-09-26 15:15:53 UTC) #27

commit-bot: I haz the power

Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=81762

7 years, 2 months ago (2013-09-26 19:06:41 UTC) #28

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/23625015/83001

7 years, 2 months ago (2013-09-27 09:54:11 UTC) #29

commit-bot: I haz the power

Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=82279

7 years, 2 months ago (2013-09-27 14:31:02 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/23625015/83001

7 years, 2 months ago (2013-09-27 14:51:17 UTC) #31

commit-bot: I haz the power

Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=82432

7 years, 2 months ago (2013-09-27 19:31:26 UTC) #32

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnissler@chromium.org/23625015/83001

7 years, 2 months ago (2013-09-30 08:28:40 UTC) #33

Message was sent while issue was closed.

Change committed as 225952

Expand Messages | Collapse Messages