Migrating tests to use EmbeddedTestServer (misc)

Migrating tests to use EmbeddedTestServer (misc)

- Moving tests to use EmbeddedTestServer where possible.
- net::test_server::EmbeddedTestServer references to net::EmbeddedTestServer
- Using server.Start() instead of server.InitializeAndWaitUntilReady()
- Renaming test_server() to be spawned_test_server() or embedded_test_server()
- Enabling tests disabled due to SpawnedTestServer compatibility

BUG=496825
TBR=brettw@chromium.org,jochen@chromium.org

Committed: https://crrev.com/969a51186f89a385a8f5bd9ddc49f35dccf9cdd2
Cr-Commit-Position: refs/heads/master@{#360598}

[svaldez, 2015-11-02 19:09:14]

svaldez@chromium.org changed reviewers:
+ davidben@chromium.org, mmenke@chromium.org

[svaldez, 2015-11-03 22:05:57]

Rebase

[davidben, 2015-11-04 00:04:24]

c/b/prerender lgtm (Did you want me to look at other bits? I don't have the
OWNERS coverage for it anyway.)

[mmenke, 2015-11-05 17:38:13]

Looks good.

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc:159: //
TODO(ajwong): Merge this into BrowserTestBase.
Can we just address this in this CL?  Same goes for other safebrowsing files. 
Fine to keep as-is, if it looks like too much effort for this CL.

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc
(right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:292:
url_str += "/safe_browsing/secure_link_features.html";
Not a big fan of manually manupulating URLs if we don't have to.  Here and
below, can we just get the URL from the https_server, and then use
GURL::Replacements to change the host?

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:195:
src_dir.AppendASCII("chrome/test/data")));
Why this change?  This seems he exactly match test_data_dir (Which if you change
this, becomes unused, and should be removed).

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:22: #include
"net/test/embedded_test_server/embedded_test_server.h"
This is included in the header, so not needed (Could forward declare in the
header, alternatively, but then I suspect you'd break other files that aren't
IWYU-clean).

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
File chrome/test/ppapi/ppapi_test.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:36: #include
"net/test/embedded_test_server/embedded_test_server.h"
This is already included in the header for this file.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:193: // Start the servers in parallel.
Comment outdated.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:211: // Start the servers in parallel.
Remove comment.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:213: ASSERT_TRUE(ws_server.StartInBackground());
I don't think we get anything out of using calls here any more?

https://codereview.chromium.org/1431653003/diff/100001/components/suggestions...
File components/suggestions/image_manager_unittest.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/components/suggestions...
components/suggestions/image_manager_unittest.cc:31: const char
kInvalidImagePath[] = "/DOESNOTEXIST";
How does this test use the embedded test server?

[mmenke, 2015-11-05 17:39:28]

Also, looks like this CL breaks
PPAPIFileChooserTestWithSBService.FileChooser_SaveAs_DangerousExecutable_Whitelist
somehow.

[svaldez, 2015-11-05 18:15:56]

I believe the crash is a result of missing chrome/browser files.

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc:159: //
TODO(ajwong): Merge this into BrowserTestBase.
On 2015/11/05 17:38:13, mmenke wrote:
> Can we just address this in this CL?  Same goes for other safebrowsing files. 
> Fine to keep as-is, if it looks like too much effort for this CL.

Done.

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc
(right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:292:
url_str += "/safe_browsing/secure_link_features.html";
On 2015/11/05 17:38:13, mmenke wrote:
> Not a big fan of manually manupulating URLs if we don't have to.  Here and
> below, can we just get the URL from the https_server, and then use
> GURL::Replacements to change the host?

Done.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/base/in_pr...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/base/in_pr...
chrome/test/base/in_process_browser_test.cc:195:
src_dir.AppendASCII("chrome/test/data")));
On 2015/11/05 17:38:13, mmenke wrote:
> Why this change?  This seems he exactly match test_data_dir (Which if you
change
> this, becomes unused, and should be removed).

Done.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:22: #include
"net/test/embedded_test_server/embedded_test_server.h"
On 2015/11/05 17:38:13, mmenke wrote:
> This is included in the header, so not needed (Could forward declare in the
> header, alternatively, but then I suspect you'd break other files that aren't
> IWYU-clean).

Done.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
File chrome/test/ppapi/ppapi_test.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:36: #include
"net/test/embedded_test_server/embedded_test_server.h"
On 2015/11/05 17:38:13, mmenke wrote:
> This is already included in the header for this file.

Done.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:193: // Start the servers in parallel.
On 2015/11/05 17:38:13, mmenke wrote:
> Comment outdated.

Done.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:211: // Start the servers in parallel.
On 2015/11/05 17:38:13, mmenke wrote:
> Remove comment.

Done.

https://codereview.chromium.org/1431653003/diff/100001/chrome/test/ppapi/ppap...
chrome/test/ppapi/ppapi_test.cc:213: ASSERT_TRUE(ws_server.StartInBackground());
On 2015/11/05 17:38:13, mmenke wrote:
> I don't think we get anything out of using calls here any more?

Done.

https://codereview.chromium.org/1431653003/diff/100001/components/suggestions...
File components/suggestions/image_manager_unittest.cc (right):

https://codereview.chromium.org/1431653003/diff/100001/components/suggestions...
components/suggestions/image_manager_unittest.cc:31: const char
kInvalidImagePath[] = "/DOESNOTEXIST";
On 2015/11/05 17:38:13, mmenke wrote:
> How does this test use the embedded test server?

Done.

[mmenke, 2015-11-05 18:21:03]

LGTM

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:204:
return embedded_test_server_->Start();
Per comment in other file, can also switch this to embedded_test_server()

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc
(right):

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:141:
return embedded_test_server_->Start();
Per comment in other file, can also switch this to embedded_test_server()

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:340:
url_str += base::UintToString(https_server.host_port_pair().port());
Again, use GURL::Replacements.

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:360:
std::string port = base::UintToString(embedded_test_server_->port());
Use GURL::Replacements.

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:474:
base::UintToString(embedded_test_server_->port()) +
Use GURL::Replacements.

[svaldez, 2015-11-05 18:34:51]

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
(right):

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:204:
return embedded_test_server_->Start();
On 2015/11/05 18:21:03, mmenke wrote:
> Per comment in other file, can also switch this to embedded_test_server()

Done.

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc
(right):

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:141:
return embedded_test_server_->Start();
On 2015/11/05 18:21:03, mmenke wrote:
> Per comment in other file, can also switch this to embedded_test_server()

Done.

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:340:
url_str += base::UintToString(https_server.host_port_pair().port());
On 2015/11/05 18:21:03, mmenke wrote:
> Again, use GURL::Replacements.

Done.

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:360:
std::string port = base::UintToString(embedded_test_server_->port());
On 2015/11/05 18:21:03, mmenke wrote:
> Use GURL::Replacements.

Done.

https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:474:
base::UintToString(embedded_test_server_->port()) +
On 2015/11/05 18:21:03, mmenke wrote:
> Use GURL::Replacements.

Done.

[svaldez, 2015-11-11 15:25:10]

On 2015/11/05 18:34:51, svaldez wrote:
>
https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
> File chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
> chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:204:
> return embedded_test_server_->Start();
> On 2015/11/05 18:21:03, mmenke wrote:
> > Per comment in other file, can also switch this to embedded_test_server()
> 
> Done.
> 
>
https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
> File
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
>
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:141:
> return embedded_test_server_->Start();
> On 2015/11/05 18:21:03, mmenke wrote:
> > Per comment in other file, can also switch this to embedded_test_server()
> 
> Done.
> 
>
https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
>
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:340:
> url_str += base::UintToString(https_server.host_port_pair().port());
> On 2015/11/05 18:21:03, mmenke wrote:
> > Again, use GURL::Replacements.
> 
> Done.
> 
>
https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
>
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:360:
> std::string port = base::UintToString(embedded_test_server_->port());
> On 2015/11/05 18:21:03, mmenke wrote:
> > Use GURL::Replacements.
> 
> Done.
> 
>
https://codereview.chromium.org/1431653003/diff/120001/chrome/renderer/safe_b...
>
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:474:
> base::UintToString(embedded_test_server_->port()) +
> On 2015/11/05 18:21:03, mmenke wrote:
> > Use GURL::Replacements.
> 
> Done.

I believe that this CL is dependent on
https://codereview.chromium.org/1409163006/, so I'm waiting on getting that one
submitted.

[svaldez, 2015-11-18 18:21:30]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-18 18:21:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1431653003/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-11-18 18:23:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1431653003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1431653003/200001

[commit-bot: I haz the power, 2015-11-18 18:42:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-18 18:42:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[svaldez, 2015-11-18 19:10:25]

Description was changed from

==========
Migrating tests to use EmbeddedTestServer (misc)

- Moving tests to use EmbeddedTestServer where possible.
- net::test_server::EmbeddedTestServer references to net::EmbeddedTestServer
- Using server.Start() instead of server.InitializeAndWaitUntilReady()
- Renaming test_server() to be spawned_test_server() or embedded_test_server()
- Enabling tests disabled due to SpawnedTestServer compatibility

BUG=496825
==========

to

==========
Migrating tests to use EmbeddedTestServer (misc)

- Moving tests to use EmbeddedTestServer where possible.
- net::test_server::EmbeddedTestServer references to net::EmbeddedTestServer
- Using server.Start() instead of server.InitializeAndWaitUntilReady()
- Renaming test_server() to be spawned_test_server() or embedded_test_server()
- Enabling tests disabled due to SpawnedTestServer compatibility

BUG=496825
TBR=brettw@chromium.org,jochen@chromium.org
==========

[svaldez, 2015-11-18 19:10:30]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-18 19:10:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1431653003/#ps220001
(title: "Fixing prerender rebase.")

[commit-bot: I haz the power, 2015-11-18 19:12:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1431653003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1431653003/220001

[commit-bot: I haz the power, 2015-11-18 20:01:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-18 20:01:21]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[svaldez, 2015-11-18 22:17:56]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-18 22:17:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1431653003/#ps240001
(title: "Fix broken rebase.")

[commit-bot: I haz the power, 2015-11-18 22:19:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1431653003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1431653003/240001

[commit-bot: I haz the power, 2015-11-18 23:16:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-18 23:16:17]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[svaldez, 2015-11-19 15:34:32]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-19 15:34:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1431653003/#ps260001
(title: "Fixing tests.")

[commit-bot: I haz the power, 2015-11-19 15:35:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1431653003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1431653003/260001

[commit-bot: I haz the power, 2015-11-19 16:27:40]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-19 16:28:23]

Patchset 14 (id:??) landed as
https://crrev.com/969a51186f89a385a8f5bd9ddc49f35dccf9cdd2
Cr-Commit-Position: refs/heads/master@{#360598}

[svaldez, 2015-11-24 16:07:44]

Description was changed from

==========
Migrating tests to use EmbeddedTestServer (misc)

- Moving tests to use EmbeddedTestServer where possible.
- net::test_server::EmbeddedTestServer references to net::EmbeddedTestServer
- Using server.Start() instead of server.InitializeAndWaitUntilReady()
- Renaming test_server() to be spawned_test_server() or embedded_test_server()
- Enabling tests disabled due to SpawnedTestServer compatibility

BUG=496825
TBR=brettw@chromium.org,jochen@chromium.org

Committed: https://crrev.com/969a51186f89a385a8f5bd9ddc49f35dccf9cdd2
Cr-Commit-Position: refs/heads/master@{#360598}
==========

to

==========
Migrating tests to use EmbeddedTestServer (misc)

- Moving tests to use EmbeddedTestServer where possible.
- net::test_server::EmbeddedTestServer references to net::EmbeddedTestServer
- Using server.Start() instead of server.InitializeAndWaitUntilReady()
- Renaming test_server() to be spawned_test_server() or embedded_test_server()
- Enabling tests disabled due to SpawnedTestServer compatibility

BUG=496825
TBR=brettw@chromium.org,jochen@chromium.org

Committed: https://crrev.com/969a51186f89a385a8f5bd9ddc49f35dccf9cdd2
Cr-Commit-Position: refs/heads/master@{#360598}
==========

[svaldez, 2015-11-24 16:07:44]

svaldez@chromium.org changed reviewers:
+ brettw@chromium.org, jochen@chromium.org