Migrating tests to use EmbeddedTestServer (/chrome/browser misc)

Migrating tests to use EmbeddedTestServer (/chrome/browser 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=thestig@chromium.org,mek@chromium.org

Committed: https://crrev.com/a01f7d9eae0080e07b6eb128e1235d472bb554b8
Cr-Commit-Position: refs/heads/master@{#360358}

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

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

[vabr (Chromium), 2015-11-03 09:38:59]

chrome/browser/password_manager/* lgtm

[davidben, 2015-11-03 21:53:41]

(Which files do you want me to look at, or all of them? I'm not sure I have
OWNERS coverage for any of them.)

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

Rebase

[svaldez, 2015-11-11 15:26:36]

On 2015/11/03 22:05:21, svaldez wrote:
> Rebase

mmenke: Just double-checking that you were going to look over this before we TBR
it?

[mmenke, 2015-11-12 19:20:30]

On 2015/11/11 15:26:36, svaldez wrote:
> On 2015/11/03 22:05:21, svaldez wrote:
> > Rebase
> 
> mmenke: Just double-checking that you were going to look over this before we
TBR
> it?

Sorry, forgot about this one.  I'll get to it early next week (Or may find time
Friday).

[mmenke, 2015-11-17 21:54:12]

Sorry for the delay.  CL is just kind of massive.  :(

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:1523:
result_id, GetFilename("slow.txt").c_str())));
Hrm...Why are all of these reformatted?

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:2169: " 
\"mime\": \"text/html\","
Did you deliberately remove this?

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
File chrome/browser/extensions/extension_apitest.cc (right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
chrome/browser/extensions/extension_apitest.cc:416: bool
ExtensionApiTest::StartTestServer() {
This is now identical to StartEmbeddedTestServer, except the other one uses
kTestServerPort.  Just remove one or the other and update callers?  Also make
sure the kBlahServerPort matches the BlahServer() method.

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1475: GURL
GetCrossDomainTestUrl(const std::string& path) {
Should this require a leading slash, to match the URLs passed to GetURL?

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:779:
GURL(embedded_test_server()->GetURL(image_path).spec()),
Erm...how did this test work before, without setting up the test server?

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/people/people_provider_browsertest.cc
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/people/people_provider_browsertest.cc:169:
test_server_.reset(new net::EmbeddedTestServer);
optional:  Just use embedded_test_server in these tests (Same for next file).

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:954:
embedded_test_server()->Start();
ASSERT_TRUE

https://codereview.chromium.org/1409163006/diff/130001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webstore_private/noframe2.html
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webstore_private/noframe2.html:16: s =
s.replace('/', '/nonesuch/');
This doesn't look right...

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

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
File chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:1523:
result_id, GetFilename("slow.txt").c_str())));
On 2015/11/17 21:54:12, mmenke wrote:
> Hrm...Why are all of these reformatted?

git cl format's fault. I could revert them, but since cl format is going to do
this everytime on this file, it might make more sense to leave as is?

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc:2169: " 
\"mime\": \"text/html\","
On 2015/11/17 21:54:12, mmenke wrote:
> Did you deliberately remove this?

Yes. We return a Content-Length, so fileSize actually gets set.

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
File chrome/browser/extensions/extension_apitest.cc (right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/extensi...
chrome/browser/extensions/extension_apitest.cc:416: bool
ExtensionApiTest::StartTestServer() {
On 2015/11/17 21:54:12, mmenke wrote:
> This is now identical to StartEmbeddedTestServer, except the other one uses
> kTestServerPort.  Just remove one or the other and update callers?  Also make
> sure the kBlahServerPort matches the BlahServer() method.

Done.

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:1475: GURL
GetCrossDomainTestUrl(const std::string& path) {
On 2015/11/17 21:54:12, mmenke wrote:
> Should this require a leading slash, to match the URLs passed to GetURL?

I think this also came up on one of the other CLs. Most non-ETS GetUrl style
methods don't require slashes, it might be a reasonable idea to make a separate
CL to match up the styles across those and ETS.

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/rendere...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:779:
GURL(embedded_test_server()->GetURL(image_path).spec()),
On 2015/11/17 21:54:12, mmenke wrote:
> Erm...how did this test work before, without setting up the test server?

I believe because this test doesn't actually check whether the image load
completes successfully.

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/search/people/people_provider_browsertest.cc
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/search/people/people_provider_browsertest.cc:169:
test_server_.reset(new net::EmbeddedTestServer);
On 2015/11/17 21:54:12, mmenke wrote:
> optional:  Just use embedded_test_server in these tests (Same for next file).

Done.

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:954:
embedded_test_server()->Start();
On 2015/11/17 21:54:12, mmenke wrote:
> ASSERT_TRUE

Done.

https://codereview.chromium.org/1409163006/diff/130001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webstore_private/noframe2.html
(right):

https://codereview.chromium.org/1409163006/diff/130001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webstore_private/noframe2.html:16: s =
s.replace('/', '/nonesuch/');
On 2015/11/17 21:54:12, mmenke wrote:
> This doesn't look right...

Done.

[mmenke, 2015-11-17 23:34:27]

LGTM

[svaldez, 2015-11-18 14:55:03]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-18 14:55:04]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1409163006/#ps230001
(title: "Actual revert of sdch_view.js")

[commit-bot: I haz the power, 2015-11-18 14:55:46]

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

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

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

[commit-bot: I haz the power, 2015-11-18 15:07:24]

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 15:10:14]

Description was changed from

==========
Migrating tests to use EmbeddedTestServer (/chrome/browser 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 (/chrome/browser 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=thestig@chromium.org,mek@chromium.org
==========

[svaldez, 2015-11-18 16:02:06]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-18 16:02:07]

The CQ bit was unchecked by svaldez@chromium.org

[svaldez, 2015-11-18 16:03:06]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-18 16:03:07]

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

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

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

[commit-bot: I haz the power, 2015-11-18 17:48:07]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-11-18 17:48:59]

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

[Lei Zhang, 2015-11-24 01:35:41]

thestig@chromium.org changed reviewers:
+ mek@chromium.org, thestig@chromium.org

[Lei Zhang, 2015-11-24 01:35:42]

It would be nice to actually add the people you TBR as reviewers.

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

On 2015/11/24 01:35:42, Lei Zhang wrote:
> It would be nice to actually add the people you TBR as reviewers.

Oops, sorry about that. I thought adding people to the TBR line would
automatically include them in the CL.

[Lei Zhang, 2015-11-24 18:10:26]

On 2015/11/24 16:07:18, svaldez wrote:
> On 2015/11/24 01:35:42, Lei Zhang wrote:
> > It would be nice to actually add the people you TBR as reviewers.
> 
> Oops, sorry about that. I thought adding people to the TBR line would
> automatically include them in the CL.

I thought so too. Anyway, I spot checked and rs lgtm.

[Evan Stade, 2015-11-24 18:31:48]

On 2015/11/24 18:10:26, Lei Zhang wrote:
> On 2015/11/24 16:07:18, svaldez wrote:
> > On 2015/11/24 01:35:42, Lei Zhang wrote:
> > > It would be nice to actually add the people you TBR as reviewers.
> > 
> > Oops, sorry about that. I thought adding people to the TBR line would
> > automatically include them in the CL.
> 
> I thought so too. Anyway, I spot checked and rs lgtm.

it does, but only if you specify TBR= in the initial upload description
(changing in the web interface won't do it)