Add end to end tests for WebSockets in net/websockets/

net/websockets/websocket_end_to_end_test.cc

Index: net/websockets/websocket_end_to_end_test.cc
diff --git a/net/websockets/websocket_end_to_end_test.cc b/net/websockets/websocket_end_to_end_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1a0de09debc4f3d33124fac1360525cb9e0b4211
--- /dev/null
+++ b/net/websockets/websocket_end_to_end_test.cc
@@ -0,0 +1,321 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// End-to-end tests for WebSocket. Usage guidelines:
+// There are at least 4 sets of end-to-end tests in the source tree:
+// 1) Blink layout tests. These are easy to write, and efficient because the

[rvargas (doing something else), 2015/01/21 00:47:04]

I don't think this file (net) should be talking about tests that live on upper
layers of the code. The opposite is not a problem, so you can add comments on
the other files pointing readers to write a net_unittest instead without any
layering violation.

[Adam Rice, 2015/01/21 09:45:56]

I think the end-to-end tests need to be viewed holistically. If net/ existed in
a vacuum, then we'd need to duplicate all the Blink WebSocket layout tests here,
which would be ridiculous.

From a pragmatic point of view, there's nowhere else to put this comment where
people have a reasonable chance of seeing it.

[rvargas (doing something else), 2015/01/21 19:46:53]

I don't agree with the conclusion. net_unittests should make sure that the
functionality implemented by net works as advertised. If that means test both
ends of a connection, and a proxy in the middle, so be it. Blink requires net
(in that in order to build the whole thing and run a layout test, net is a
required), so it can know what is implemented (and tested) at the net layer and
avoid duplication (only test the functionality that is added by blink, or some
aspect of the integration).

... and net somewhat exists in a vacuum these days, as we explicitly want to
support users that are not tied to content or above. So talking about layout
tests or browser tests here is a layering violation. (sure, very small given
that this is a comment on a unit test file)

I must admit that I'm completely unfamiliar with how we've been moving WS code
to net, but I don't think any module (much less something like net) should be
delegating testing on some other module that may or may not be present (layout
tests)
I would agree with the whole thing if I believed this statement. But I think it
is perfectly fine to add relevant comments on
browser/ppapi/layout/whatever-test* stating what _kind_ of tests it is fine to
add at that layer, and that net_unittest may be a better place to add tests that
don't fit those requirements.

If I'm adding code say at the ppapi layer, I'm very likely to be aware that
there is WS code living in net, so there should be some unit tests there... the
opposite is not true.

This is philosophically the same as base code talking about chrome_threads just
because they are implemented with the threading primitives exposed by base.

[Adam Rice, 2015/01/22 12:06:28]

I didn't find out about the PPAPI tests until I'd already been working on
WebSockets for a year. If someone is looking in there, they're already having a
bad day.

Anyway, I have removed the offending comments.

+//    client and server processes are both re-used. They are easy to write as
+//    they are just Javascript. They should be used for the majority of
+//    functional tests.
+// 2) Browser tests in //chrome/browser/net/websocket_browsertest.cc. These
+//    are inefficient because both the server and client are restarted for each
+//    test. They are also hard to write and hard to debug. They should generally
+//    only be used for testing UI-related features.
+// 3) PPAPI tests in //chrome/test/ppapi/ppapi_browsertest.cc. These are
+//    very hard to write and debug, and should only be used for verifying the
+//    operation of the Pepper API.
+// 4) These tests. They are somewhat inefficient because the server is restarted

[rvargas (doing something else), 2015/01/21 00:47:05]

This comment makes sense by itself: when it is ok to write a test here.

[Adam Rice, 2015/01/22 12:06:28]

Okay. This comment is now by itself.

+//    for each test. They are well-suited for tests which require special server
+//    configurations.
+//
+// Tests should generally not be duplicated in the different places, except for

[rvargas (doing something else), 2015/01/21 00:47:04]

isn't this always true?

[Adam Rice, 2015/01/21 09:45:56]

Yes. But you don't have to look far to find pointless duplicate tests. I'm
afraid that without this comment, in  a few years there'd be 10,000 lines of
tests here.

[rvargas (doing something else), 2015/01/21 19:46:53]

To be honest, I don't think a line saying don't duplicate code is going to have
any effect... it will have to be a popup warning on Rietveld :P

[Adam Rice, 2015/01/22 12:06:28]

Removed.

+// regression tests and basic smoke tests.
+
+#include <string>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "net/base/auth.h"
+#include "net/base/network_delegate.h"
+#include "net/base/test_data_directory.h"
+#include "net/proxy/proxy_service.h"
+#include "net/test/spawned_test_server/spawned_test_server.h"
+#include "net/url_request/url_request_test_util.h"
+#include "net/websockets/websocket_channel.h"
+#include "net/websockets/websocket_event_interface.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/origin.h"
+
+namespace net {
+
+namespace {
+
+static const char kEchoServer[] = "echo-with-no-extension";
+
+// An implementation of WebSocketEventInterface that waits for and records the
+// results of the connect.
+class ConnectTestingEventInterface : public WebSocketEventInterface {

[rvargas (doing something else), 2015/01/21 00:47:04]

Nit: I think this class is slightly too long to have declaration mixed with
definition. It makes the code harder to read.

[Adam Rice, 2015/01/21 09:45:56]

Done.

+ public:
+  ConnectTestingEventInterface() : fail_(true) {}
+
+  void WaitForResponse() { run_loop_.Run(); }
+
+  // fail() is true if the handshake failed (ie. OnFailChannel was called).

[rvargas (doing something else), 2015/01/21 00:47:04]

Shouldn't it be called failed_ then? And the comment belongs with the member
variable.

[Adam Rice, 2015/01/21 09:45:55]

Okay. I made just this definition online, so that it is easy to refer to the
comment.

+  bool fail() const { return fail_; }
+
+  // Only set if the handshake failed, otherwise empty.
+  // Failure messages appear on the console and are checked by the layout tests,

[rvargas (doing something else), 2015/01/21 00:47:04]

layout tests?

[Adam Rice, 2015/01/21 09:45:56]

Blink layout tests. I clarified the comment slightly.

[rvargas (doing something else), 2015/01/21 19:46:53]

My point is that this file should not know what a layout test is, or what is it
going to do with error messages generated by net code... and presumably the
caller of this method will not be a layout test (right?) so it shouldn't care
what other code is going to do with a message.

[Adam Rice, 2015/01/22 12:06:28]

Done.

+  // so they are expected to stay reasonably stable across versions.
+  std::string failure_message() const { return failure_message_; }
+
+  std::string selected_subprotocol() const { return selected_subprotocol_; }
+
+  std::string extensions() const { return extensions_; }
+
+  ChannelState OnAddChannelResponse(bool fail,
+                                    const std::string& selected_subprotocol,
+                                    const std::string& extensions) override {
+    fail_ = fail;
+    selected_subprotocol_ = selected_subprotocol;
+    extensions_ = extensions;
+    QuitNestedEventLoop();
+    return fail ? CHANNEL_DELETED : CHANNEL_ALIVE;
+  }
+
+  ChannelState OnDataFrame(bool fin,
+                           WebSocketMessageType type,
+                           const std::vector<char>& data) override {
+    return CHANNEL_ALIVE;
+  }
+
+  ChannelState OnFlowControl(int64 quota) override { return CHANNEL_ALIVE; }
+
+  ChannelState OnClosingHandshake() override { return CHANNEL_ALIVE; }
+
+  ChannelState OnDropChannel(bool was_clean,
+                             uint16 code,
+                             const std::string& reason) override {
+    return CHANNEL_DELETED;
+  }
+
+  ChannelState OnFailChannel(const std::string& message) override {
+    fail_ = true;
+    failure_message_ = message;
+    QuitNestedEventLoop();
+    return CHANNEL_DELETED;
+  }
+
+  ChannelState OnStartOpeningHandshake(
+      scoped_ptr<WebSocketHandshakeRequestInfo> request) override {
+    return CHANNEL_ALIVE;
+  }
+
+  ChannelState OnFinishOpeningHandshake(
+      scoped_ptr<WebSocketHandshakeResponseInfo> response) override {
+    return CHANNEL_ALIVE;
+  }
+
+  ChannelState OnSSLCertificateError(
+      scoped_ptr<SSLErrorCallbacks> ssl_error_callbacks,
+      const GURL& url,
+      const SSLInfo& ssl_info,
+      bool fatal) override {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(&SSLErrorCallbacks::CancelSSLRequest,
+                              base::Owned(ssl_error_callbacks.release()),
+                              ERR_SSL_PROTOCOL_ERROR, &ssl_info));
+    return CHANNEL_ALIVE;
+  }
+
+ private:
+  void QuitNestedEventLoop() { run_loop_.Quit(); }
+
+  bool fail_;
+  std::string selected_subprotocol_;
+  std::string extensions_;
+  std::string failure_message_;
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConnectTestingEventInterface);
+};
+
+// A subclass of TestNetworkDelegate that additionally implements the
+// OnResolveProxy callback and records the information passed to it.
+class TestNetworkDelegateWithProxyInfo : public TestNetworkDelegate {
+ public:
+  TestNetworkDelegateWithProxyInfo() {}
+
+  struct OnResolveProxyInfo {

[rvargas (doing something else), 2015/01/21 00:47:04]

nit: Don't declare a type named OnFoo. That pattern is strongly tied to a
callback method. ResolvedProxyInfo sounds much better.

[Adam Rice, 2015/01/21 09:45:55]

Thanks!

+    GURL url;
+    ProxyInfo proxy_info;
+  };
+
+  const OnResolveProxyInfo& on_resolve_proxy_info() const {
+    return on_resolve_proxy_info_;
+  }
+
+ protected:
+  void OnResolveProxy(const GURL& url,
+                      int load_flags,
+                      const ProxyService& proxy_service,
+                      ProxyInfo* result) override {
+    on_resolve_proxy_info_.url = url;
+    on_resolve_proxy_info_.proxy_info = *result;
+  }
+
+ private:
+  OnResolveProxyInfo on_resolve_proxy_info_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestNetworkDelegateWithProxyInfo);
+};
+
+class WebSocketEndToEndTest : public ::testing::Test {
+ protected:
+  WebSocketEndToEndTest()
+      : event_interface_(new ConnectTestingEventInterface),
+        network_delegate_(new TestNetworkDelegateWithProxyInfo),
+        context_(true),
+        channel_(make_scoped_ptr(event_interface_), &context_),
+        initialised_context_(false) {}
+
+  // Initialise the URLRequestContext. Normally done automatically by
+  // ConnectAndWait(). This method is for the use of tests that need the
+  // URLRequestContext initialised before calling ConnectAndWait().
+  void InitialiseContext() {
+    context_.set_network_delegate(network_delegate_.get());
+    context_.Init();
+    initialised_context_ = true;
+  }
+
+  // Send the connect request to |socket_url| and wait for a response. Returns
+  // true if the handshake succeeded.
+  bool ConnectAndWait(const GURL& socket_url) {
+    if (!initialised_context_) {
+      InitialiseContext();
+    }
+    std::vector<std::string> sub_protocols;
+    url::Origin origin("http://localhost");
+    channel_.SendAddChannelRequest(GURL(socket_url), sub_protocols, origin);
+    event_interface_->WaitForResponse();
+    return !event_interface_->fail();
+  }
+
+  ConnectTestingEventInterface* event_interface_;  // owned by channel_
+  scoped_ptr<TestNetworkDelegateWithProxyInfo> network_delegate_;
+  TestURLRequestContext context_;
+  WebSocketChannel channel_;
+  bool initialised_context_;
+};
+
+// None of these tests work on Android.
+// TODO(ricea): Make these tests work on Android. See crbug.com/441711.
+#if defined(OS_ANDROID)
+#define DISABLED_ON_ANDROID(test) DISABLED_##test
+#else
+#define DISABLED_ON_ANDROID(test) test
+#endif
+
+// Basic test of connectivity. If this test fails, nothing else can be expected
+// to work.
+TEST_F(WebSocketEndToEndTest, DISABLED_ON_ANDROID(BasicSmokeTest)) {
+  SpawnedTestServer ws_server(SpawnedTestServer::TYPE_WS,
+                              SpawnedTestServer::kLocalhost,
+                              GetWebSocketTestDataDirectory());
+  ASSERT_TRUE(ws_server.Start());
+  EXPECT_TRUE(ConnectAndWait(ws_server.GetURL(kEchoServer)));
+}
+
+// Test for issue crbug.com/433695 "Unencrypted WebSocket connection via
+// authenticated proxy times out"
+// TODO(ricea): Enable this when the issue is fixed.
+TEST_F(WebSocketEndToEndTest, DISABLED_HttpsProxyUnauthedFails) {
+  SpawnedTestServer proxy_server(SpawnedTestServer::TYPE_BASIC_AUTH_PROXY,
+                                 SpawnedTestServer::kLocalhost,
+                                 base::FilePath());
+  SpawnedTestServer ws_server(SpawnedTestServer::TYPE_WS,
+                              SpawnedTestServer::kLocalhost,
+                              GetWebSocketTestDataDirectory());
+  ASSERT_TRUE(proxy_server.StartInBackground());
+  ASSERT_TRUE(ws_server.StartInBackground());
+  ASSERT_TRUE(proxy_server.BlockUntilStarted());
+  ASSERT_TRUE(ws_server.BlockUntilStarted());
+  std::string proxy_config =
+      "https=" + proxy_server.host_port_pair().ToString();
+  scoped_ptr<ProxyService> proxy_service(
+      ProxyService::CreateFixed(proxy_config));
+  ASSERT_TRUE(proxy_service);
+  context_.set_proxy_service(proxy_service.get());
+  EXPECT_FALSE(ConnectAndWait(ws_server.GetURL(kEchoServer)));
+  EXPECT_EQ("Proxy authentication failed", event_interface_->failure_message());
+}
+
+TEST_F(WebSocketEndToEndTest, DISABLED_ON_ANDROID(HttpsWssProxyUnauthedFails)) {
+  SpawnedTestServer proxy_server(SpawnedTestServer::TYPE_BASIC_AUTH_PROXY,
+                                 SpawnedTestServer::kLocalhost,
+                                 base::FilePath());
+  SpawnedTestServer wss_server(SpawnedTestServer::TYPE_WSS,
+                               SpawnedTestServer::kLocalhost,
+                               GetWebSocketTestDataDirectory());
+  ASSERT_TRUE(proxy_server.StartInBackground());
+  ASSERT_TRUE(wss_server.StartInBackground());
+  ASSERT_TRUE(proxy_server.BlockUntilStarted());
+  ASSERT_TRUE(wss_server.BlockUntilStarted());
+  std::string proxy_config =
+      "https=" + proxy_server.host_port_pair().ToString();
+  scoped_ptr<ProxyService> proxy_service(
+      ProxyService::CreateFixed(proxy_config));
+  ASSERT_TRUE(proxy_service);
+  context_.set_proxy_service(proxy_service.get());
+  EXPECT_FALSE(ConnectAndWait(wss_server.GetURL(kEchoServer)));
+  EXPECT_EQ("Proxy authentication failed", event_interface_->failure_message());
+}
+
+// Regression test for crbug/426736 "WebSocket connections not using configured
+// system HTTPS Proxy".
+TEST_F(WebSocketEndToEndTest, DISABLED_ON_ANDROID(HttpsProxyUsed)) {
+  SpawnedTestServer proxy_server(SpawnedTestServer::TYPE_BASIC_AUTH_PROXY,
+                                 SpawnedTestServer::kLocalhost,
+                                 base::FilePath());
+  SpawnedTestServer ws_server(SpawnedTestServer::TYPE_WS,
+                              SpawnedTestServer::kLocalhost,
+                              GetWebSocketTestDataDirectory());
+  ASSERT_TRUE(proxy_server.StartInBackground());
+  ASSERT_TRUE(ws_server.StartInBackground());
+  ASSERT_TRUE(proxy_server.BlockUntilStarted());
+  ASSERT_TRUE(ws_server.BlockUntilStarted());
+  std::string proxy_config = "https=" +
+                             proxy_server.host_port_pair().ToString() + ";" +
+                             "http=" + proxy_server.host_port_pair().ToString();
+  scoped_ptr<ProxyService> proxy_service(
+      ProxyService::CreateFixed(proxy_config));
+  context_.set_proxy_service(proxy_service.get());
+  InitialiseContext();
+
+  // The test server doesn't have an unauthenticated proxy mode. WebSockets
+  // cannot provide auth information that isn't already cached, so it's
+  // necessary to preflight an HTTP request to authenticate against the proxy.
+  std::string scheme("http");
+  GURL::Replacements replacements;
+  replacements.SetSchemeStr(scheme);
+  // It doesn't matter what the URL is, as long as it is an HTTP navigation.
+  GURL http_page =
+      ws_server.GetURL("connect_check.html").ReplaceComponents(replacements);
+  TestDelegate delegate;
+  delegate.set_credentials(
+      AuthCredentials(base::ASCIIToUTF16("foo"), base::ASCIIToUTF16("bar")));
+  {
+    scoped_ptr<URLRequest> request(
+        context_.CreateRequest(http_page, DEFAULT_PRIORITY, &delegate, NULL));
+    request->Start();
+    // TestDelegate exits the message loop when the request completes by
+    // default.
+    base::RunLoop().Run();
+    EXPECT_TRUE(delegate.auth_required_called());
+  }
+
+  GURL ws_url = ws_server.GetURL(kEchoServer);
+  EXPECT_TRUE(ConnectAndWait(ws_url));
+  const TestNetworkDelegateWithProxyInfo::OnResolveProxyInfo& info =
+      network_delegate_->on_resolve_proxy_info();
+  EXPECT_EQ(ws_url, info.url);
+  EXPECT_TRUE(info.proxy_info.is_http());
+}
+
+}  // namespace
+
+}  // namespace net