WebSocket test server migration on browser_tests

WebSocket test server migration on browser_tests.

This change contains following tests migration.
 - ProxyBrowserTest.BasicAuthWSConnect
 - SSLUITest.TestWSSInvalidCertAndGoFoward
 - SSLUITest.TestWSSInvalidCertAndClose
 - SSLUITestIgnoreCertErrors.TestWSS

BUG=137639
TEST=browser_test

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

[Takashi Toyoshima, 2012-10-10 08:14:44]

Hi Jay,
This change may be trivial for chrome/ files because this change just replaces
underlying test server. But, could you review this change as a
chrome/test/OWNERS who is familiar to SSL tests.

Hi Ryan,
This is a change for browser_tests.
Could you review this, too?

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:103: FilePath(kWsRoot)) {}
I just follow existing style here.
What do you think about this?
Should I replace them with FilePath().Append()... style?

[Jay Civelli, 2012-10-10 15:39:45]

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:103: FilePath(kWsRoot)) {}
On 2012/10/10 08:14:44, toyoshim wrote:
> I just follow existing style here.
> What do you think about this?
> Should I replace them with FilePath().Append()... style?
I am not sure I understand what you mean.

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server....
net/test/base_test_server.h:182: // TYPE_WS or TYPE_WSS serves HTTP or HTTPS
simultaneously, and http:// like
Did you mean respectively instead of simultaneously?

[Ryan Sleevi, 2012-10-10 17:59:57]

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/net/proxy_b...
File chrome/browser/net/proxy_browsertest.cc (right):

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/net/proxy_b...
chrome/browser/net/proxy_browsertest.cc:92:
.Append(FILE_PATH_LITERAL("websocket")));
As per the comment on the other CL, using
FILE_PATH_LITERAL("net/data/websocket") seems acceptable, based on the existing
codebase.

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:88: typedef net::TestServer::SSLOptions
SSLOptions;
style bug: Public -> Protected -> Private.

I realize this wasn't your fault (pre-existing issue), but it's not clear to me
why this was ordered as such. The type should be resolvable if moved later in
the file, unless I'm mistaken.

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server....
net/test/base_test_server.h:183: // URL is often used for providing JavaScripts
that contain actual tests.
The comment and name of this method are very confusing.

"Alternate" URL is problematic, because it does not sufficiently highlight that
the whole notion is specific to WebSockets (which may have WS and HTTP requests
served over the same server)

I'm not sure how widespread this method is, but I wonder if it makes more sense
to have callers do

GURL::Replacements replacements;
if (type == TYPE_WS) {
  replacements.SetSchemeStr("http");
} else if (type == TYPE_WSS) {
  replacements.SetSchemeStr("https");
}
return GetURL(path).ReplaceComponents(replacements);


Either way, I think the comment should be updated.

https://codereview.chromium.org/11085039/diff/4001/net/tools/testserver/tests...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/11085039/diff/4001/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2259: option.certificate =
options.cert_and_key_file
I'm not sure this change is correct, combined with the change to the test server
to check UsingSSL.

For example, you're ignoring options like ocsp, and you're not performing the
same checks that the HTTP side does (eg: lines 2189-2195)

Can you please try to keep parity between what the C++ side sends and what the
Python side can handle (and to make sure the Python handling is consistent)

[Takashi Toyoshima, 2012-10-11 04:53:21]

For Jay's comments.

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:103: FilePath(kWsRoot)) {}
Sorry for vague comment.
Ryan and I have seen two styles to express file path in a series of my changes
on the migration.

 - FilePath(FILE_PATH_LITERAL("foo/bar"))
 - FilePath(FILE_PATH_LITERAL("foo")).Append("bar")

Firstly, I thought I use the former style consistently. But now I decide to use
latter style here and there.

So, please ignore this comment.

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server....
net/test/base_test_server.h:182: // TYPE_WS or TYPE_WSS serves HTTP or HTTPS
simultaneously, and http:// like
TYPE_WS serves WS and HTTP simultaneously.
TYPE_WSS serves WSS and HTTPS simultaneously.

So, how about following expression.
TYPE_WS or TYPE_WSS services WS and HTTP, or WSS and HTTPS respectively.

[Takashi Toyoshima, 2012-10-11 05:53:28]

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/net/proxy_b...
File chrome/browser/net/proxy_browsertest.cc (right):

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/net/proxy_b...
chrome/browser/net/proxy_browsertest.cc:92:
.Append(FILE_PATH_LITERAL("websocket")));
On 2012/10/10 17:59:57, Ryan Sleevi wrote:
> As per the comment on the other CL, using
> FILE_PATH_LITERAL("net/data/websocket") seems acceptable, based on the
existing
> codebase.

Done.

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/11085039/diff/4001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:88: typedef net::TestServer::SSLOptions
SSLOptions;
You are right.
I moved this later in the file.
It didn't break build.

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server.h
File net/test/base_test_server.h (right):

https://codereview.chromium.org/11085039/diff/4001/net/test/base_test_server....
net/test/base_test_server.h:183: // URL is often used for providing JavaScripts
that contain actual tests.
Thank you for advice.
GURL::Replacements looks useful.
Currently we have only three test where this function is needed.
Using GURL::Replacements looks enough for now.

https://codereview.chromium.org/11085039/diff/4001/net/tools/testserver/tests...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/11085039/diff/4001/net/tools/testserver/tests...
net/tools/testserver/testserver.py:2259: option.certificate =
options.cert_and_key_file
As you said, currently testserver.py doesn't support ocsp and client cert for
WebSocket.
I'll fix C++ side to limit arguments.

[Takashi Toyoshima, 2012-10-11 14:25:51]

Hum...I could not understand the reason, but GURL::Replacements doesn't work
correctly in browser_tests, and content_browsertests. Calling ReplaceComponents
breaks contents of GURL.
The same code works in gurl_unittests correctly.
I'll continue this tomorrow.

[Takashi Toyoshima, 2012-10-15 07:51:37]

Fixed. I should keep reference to std::string of SetSchemeStr argument because
it doesn't have a copy internally.

So, could you take another look?

[Ryan Sleevi, 2012-10-15 16:51:54]

Moving myself to CC - putting phajdan.jr as reviewer (net/test,
net/tools/testserver OWNER)

https://codereview.chromium.org/11085039/diff/13002/net/test/base_test_server.cc
File net/test/base_test_server.cc (right):

https://codereview.chromium.org/11085039/diff/13002/net/test/base_test_server...
net/test/base_test_server.cc:359: // TODO(toyoshim): TYPE_WSS should support
client auth, too.
Do you have a BUG # to track this?

[Takashi Toyoshima, 2012-10-16 08:29:34]

I splited net/test, and net/tools/testserver changes to
https://codereview.chromium.org/11175002/ .

phajdan.jr,
I'll remove you from this CL again, so could you review former 11175002?

[Takashi Toyoshima, 2012-10-17 12:21:29]

+wtc as a chrome/browser/net reviewer.

I need one more authority for chrome/browser/ssl, but I think Jay is the best
person for ssl_browser_test.cc. I'll ask a stamp on it after Jay's review.

Thanks.

[Takashi Toyoshima, 2012-10-19 04:30:21]

+abarth for chrome/browser/ssl

Hi Adam,
Could you also take a look on this CL?
Basically, this change just replaces testserver from old one to new one.

Hi Wan-Teh,
Could you take a look?
The change for chrome/browser/net is more trivial one.

Also, this is the last change for the test server migration.
Once this change is landed, I'll remove old server.

Thanks.

[Takashi Toyoshima, 2012-10-23 02:40:59]

ping.

Jay, Adam, and Wan-Teh,
Could you take a look this CL as OWNERS?

I really need to commit this CL to complete test server migration.

[wtc, 2012-10-23 19:02:55]

Patch set 8 LGTM. I have an important question about these
tests using test data inside src/net.

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:58: const FilePath::CharType kWsRoot[] =
FILE_PATH_LITERAL("net/data/websocket");

(NOTE: my comments in this file also apply to
chrome/browser/net/proxy_browsertest.cc.)

It seems bad to put test data inside src/net. I am worried
that someone who works on src/net may not know there are
unit tests outside src/net that depend on the test data in
net/data/websocket.

Can this be avoided?

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:611: replacements.SetSchemeStr(scheme);

Just curious: why does this unit test need to replace the
scheme? What is the original scheme in the URL for
"connect_check.html"?

[wtc, 2012-10-23 19:04:32]

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:304: typedef net::TestServer::SSLOptions
SSLOptions;

It seems that this typedef can be private. Does that work?

[Ryan Sleevi, 2012-10-23 19:04:35]

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:58: const FilePath::CharType kWsRoot[] =
FILE_PATH_LITERAL("net/data/websocket");
On 2012/10/23 19:02:55, wtc wrote:
> 
> (NOTE: my comments in this file also apply to
> chrome/browser/net/proxy_browsertest.cc.)
> 
> It seems bad to put test data inside src/net. I am worried
> that someone who works on src/net may not know there are
> unit tests outside src/net that depend on the test data in
> net/data/websocket.
> 
> Can this be avoided?

We do this for the SSL certs already.

We could wrap this up in an API for net:: to expose the path for WS test data,
but I'm not sure that really resolves the issue you highlighted, just shuffles
it (from looking for usage of files to looking for usage of callers)

[Paweł Hajdan Jr., 2012-10-24 01:13:46]

LGTM

[Takashi Toyoshima, 2012-10-24 05:29:08]

Thank you guys.
I'll commit this.

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:58: const FilePath::CharType kWsRoot[] =
FILE_PATH_LITERAL("net/data/websocket");
I have a same idea with Ryan.
But, the directory for SSL certs have a good README document.
So, at least, net/data/websocket should have similar one, I think.

In addition, usually, we don't have information for reverse dependency, right?
I think 'git grep net/data/websocket' works fine to know a reverse dependency.

Anyway, I will file a new bug, and consider what should be done to avoid
mistakes.

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:304: typedef net::TestServer::SSLOptions
SSLOptions;
Oops.
Thanks. You are right.
I'll fix this.

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:611: replacements.SetSchemeStr(scheme);
|wss_server_expired_| is launched with TYPE_WSS.
So, its GetURL() returns wss scheme for 'connect_check.html'.
This is because TYPE_WS serves http and ws, and TYPE_WSS does https and wss.

[wtc, 2012-10-25 00:18:09]

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/11085039/diff/42001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:611: replacements.SetSchemeStr(scheme);

On 2012/10/24 05:29:08, Takashi Toyoshima (chromium) wrote:
> |wss_server_expired_| is launched with TYPE_WSS.
> So, its GetURL() returns wss scheme for 'connect_check.html'.
> This is because TYPE_WS serves http and ws, and TYPE_WSS does https and wss.

I see. Does this mean ui_test_utils::NavigateToURL() cannot
handle wss and ws URLs, so you have to change the scheme
in the URL to https or http?

[Takashi Toyoshima, 2012-10-25 03:23:48]

Actual test is written in JavaScript and exists in the connect_check.html, and
WebSocket connection can be established only by JavaScript WebSocket instance.

Here, wss_server_expired_ provides this HTML file containing the test via HTTPS,
and also handle WebSocket connection which is requested by a WebSocket instance
generated in the test.

Of course, we can provide the HTML file from another http_server. But at that
case, we could not share the certificate information between the test hosting
HTML and WebSocket connection under test.

Note: WebSocket can share the same host and port with HTTP. The opening
handshake of WebSocket is compatible with HTTP/1.1 and it upgrades the
connection from HTTP/1.1 to WebSocket if the server supports WebSocket. So, some
URLs provide HTML files, and the other URLs may provide WebSocket connection at
the same origin server.

[abarth-chromium, 2012-10-25 03:25:23]

Delayed LGTM for browser/ssl