Created policy QuicAllowed

chrome/browser/policy/policy_network_browsertest.cc

Index: chrome/browser/policy/policy_network_browsertest.cc
diff --git a/chrome/browser/policy/policy_network_browsertest.cc b/chrome/browser/policy/policy_network_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c6c9b21fca37f1d6228e2cd863a662ca440771cc
--- /dev/null
+++ b/chrome/browser/policy/policy_network_browsertest.cc
@@ -0,0 +1,71 @@
+// Copyright (c) 2015 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.
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/policy/core/browser/browser_policy_connector.h"
+#include "components/policy/core/common/mock_configuration_policy_provider.h"
+#include "components/policy/core/common/policy_map.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/browser_test.h"
+#include "net/http/http_transaction_factory.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "policy/policy_constants.h"
+
+namespace {
+
+void getContext(net::URLRequestContextGetter* getter,

[Andrew T Wilson (Slow), 2015/04/09 09:02:04]

nit: getContext->GetContext

[peletskyi, 2015/04/09 09:43:33]

Done.

+    const base::Closure& done_callback) {

[Andrew T Wilson (Slow), 2015/04/09 09:02:04]

Should be indented to match the open paren above.

[peletskyi, 2015/04/09 09:43:33]

Done.

+  net::URLRequestContext* context = getter->GetURLRequestContext();
+  bool quic_enabled = context->http_transaction_factory()->GetSession()->
+        params().enable_quic;
+  EXPECT_FALSE(quic_enabled);
+
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      done_callback);
+}
+
+}  // namespace
+

[Andrew T Wilson (Slow), 2015/04/09 09:02:04]

Should probably put this stuff in namespace policy since it lives in
browser/policy. Then you don't have to use policy:: below.

[peletskyi, 2015/04/09 09:43:33]

Done.

+class QuicAllowedPolicyTest: public InProcessBrowserTest {
+public:

[Andrew T Wilson (Slow), 2015/04/09 09:02:04]

indent this

[peletskyi, 2015/04/09 09:43:33]

Done.

+  QuicAllowedPolicyTest() : InProcessBrowserTest(){}
+ protected:

[Andrew T Wilson (Slow), 2015/04/09 09:02:04]

blank line before protected:

[peletskyi, 2015/04/09 09:43:33]

Done.

+  void SetUpInProcessBrowserTestFixture() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch("enable-quic");
+    EXPECT_CALL(provider_, IsInitializationComplete(testing::_))
+        .WillRepeatedly(testing::Return(true));
+
+    policy::BrowserPolicyConnector::SetPolicyProviderForTesting(&provider_);
+
+    policy::PolicyMap values;
+    values.Set(policy::key::kQuicAllowed,
+               policy::POLICY_LEVEL_MANDATORY,
+               policy::POLICY_SCOPE_MACHINE,
+               new base::FundamentalValue(false),
+               NULL);
+    provider_.UpdateChromePolicy(values);
+
+  }
+
+ private:
+  policy::MockConfigurationPolicyProvider provider_;
+  DISALLOW_COPY_AND_ASSIGN(QuicAllowedPolicyTest);
+};
+
+IN_PROC_BROWSER_TEST_F(QuicAllowedPolicyTest, QuicDisabled) {
+  base::RunLoop run_loop;
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(getContext,

[Andrew T Wilson (Slow), 2015/04/09 09:02:04]

Maybe rename getContext to VerifyQuicEnabledStatus()?

If you pass in the expected value of quick_enabled, then you can easily write 3
tests: to check the behavior when no policy is set, when policy == true, and
when policy == false. You should probably do that.

[peletskyi, 2015/04/09 09:43:33]

Rename - done.
About three tests. I think it is better to leave it as it is now. For the cases
QuicAllowed == true and QuicAllowed is not set we do not guarantee that Quic is
enabled. We just care when QuicAllowed == false, QUIC is disabled. The rest
cases are out of the scope.

[Andrew T Wilson (Slow), 2015/04/09 12:13:15]

OK, let's imagine someone goes into io_thread.cc line 600-601 and makes this
change:

if (value)
  is_quic_allowed_by_policy_ = false;

So whenever this policy is set at all, quic is disabled. Would your test catch
this bug? Let's imagine that someone breaks the enable-quic flag or changes its
semantics so in your test quic is disabled by default - would your test catch
this?

I would disagree that testing these cases are out of scope, because the
documentation you have added to policy_templates.json explicitly says:

If this policy is set to true or not set usage of QUIC protocol in <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> is allowed.

But you don't verify that this behavior is actually happening via a test.

[peletskyi, 2015/04/10 08:26:33]

Acknowledged.

+          make_scoped_refptr(g_browser_process->system_request_context()),
+          run_loop.QuitClosure()));
+  run_loop.Run();
+}