Move DnsApiTest.DnsResolveIPLiteral and DnsApiTest.DnsResolveHostname to app_shell_browsertests.

apps/shell/browser/dns_apitest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.

[Yoyo Zhou, 2014/07/16 02:30:25]

I put this file in apps/shell/browser to avoid introducing an extensions->apps
dependency. The logical home for it is of course extensions/browser/api/dns.
Discuss.

[James Cook, 2014/07/16 17:15:31]

I think the time has come to move app_shell out of src/apps/shell into
src/extensions/shell. It's just going to get increasingly awkward as more
browser_tests and unit_tests are extracted from Chrome.  We're fighting against
src/apps in CLs like this, namespaces, unit tests, etc.  How should we go about
doing that move?

[Yoyo Zhou, 2014/07/18 02:45:30]

I'll move to src/extensions/shell after this CL. Filed crbug.com/394987 for it.

[tfarina, 2014/07/18 02:47:37]

extensions/shell :) Cool!

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "apps/shell/test/shell_test.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"

[mmenke, 2014/07/16 14:05:54]

You're missing the header for OVERRIDE.  Suggest just using macros.h for that
and DISALLOW_COPY_AND_ASSIGN.

[Yoyo Zhou, 2014/07/18 02:45:30]

Done.

+#include "extensions/browser/api/dns/dns_api.h"
+#include "extensions/browser/api/dns/host_resolver_wrapper.h"
+#include "extensions/browser/api/dns/mock_host_resolver_creator.h"
+#include "extensions/browser/api_test_utils.h"
+#include "extensions/browser/extension_function_dispatcher.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "net/base/net_errors.h"
+#include "net/dns/mock_host_resolver.h"

[mmenke, 2014/07/16 14:05:54]

Are you actual depending on this?  Looks like you just passing pointers to this
around, so could forward declare it instead (And since you're using functions
from other headers that return it, think you're fine not even forward declaring
it).

[Yoyo Zhou, 2014/07/18 02:45:30]

It's used in line 42.

[mmenke, 2014/07/18 21:32:30]

Are you sure?  resolver_creator_->CreateMockHostResolver() calls a function that
returns a net::MockHostResolver*, but you never dereference it, or call any
method defined in MockHostResolver.  Looks like all you need is a forward
declaration.

[Yoyo Zhou, 2014/07/22 00:10:30]

It turns out this is because the header was needed to know that a
MockHostResolver is a HostResolver. I've changed MockHostResolverCreator's
interface to return a HostResolver* (it seems logical for its consumers not to
care what kind of HostResolver it is).

+
+using extensions::ExtensionFunctionDispatcher;
+using extensions::api_test_utils::RunFunctionAndReturnSingleResult;
+
+namespace {
+
+class TestFunctionDispatcherDelegate
+    : public ExtensionFunctionDispatcher::Delegate {
+ public:
+  TestFunctionDispatcherDelegate() {}
+  virtual ~TestFunctionDispatcherDelegate() {}
+
+  // NULL implementation
+ private:

[mmenke, 2014/07/16 14:05:54]

DISALLOW_COPY_AND_ASSIGN (And include the requisite header)?

[Yoyo Zhou, 2014/07/18 02:45:30]

Done.

+};
+}

[mmenke, 2014/07/16 14:05:54]

nit:  Linebreak before ending namespace.

[mmenke, 2014/07/16 14:05:54]

nit:  "}  // namespace"

[Yoyo Zhou, 2014/07/18 02:45:30]

Done.

+
+class DnsApiTest : public apps::AppShellTest {
+ public:
+  DnsApiTest() : resolver_creator_(new extensions::MockHostResolverCreator()) {}
+
+ private:
+  virtual void SetUpOnMainThread() OVERRIDE {
+    apps::AppShellTest::SetUpOnMainThread();
+    extensions::HostResolverWrapper::GetInstance()->SetHostResolverForTesting(
+        resolver_creator_->CreateMockHostResolver());
+  }
+
+  virtual void TearDownOnMainThread() OVERRIDE {
+    extensions::HostResolverWrapper::GetInstance()->SetHostResolverForTesting(
+        NULL);
+    resolver_creator_->DeleteMockHostResolver();
+    apps::AppShellTest::TearDownOnMainThread();
+  }
+
+  // The MockHostResolver asserts that it's used on the same thread on which
+  // it's created, which is actually a stronger rule than its real counterpart.
+  // But that's fine; it's good practice.
+  scoped_refptr<extensions::MockHostResolverCreator> resolver_creator_;
+};
+
+IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsResolveIPLiteral) {
+  scoped_refptr<extensions::DnsResolveFunction> resolve_function(
+      new extensions::DnsResolveFunction());
+  scoped_refptr<extensions::Extension> empty_extension(
+      extensions::ExtensionBuilder()
+          .SetManifest(extensions::DictionaryBuilder().Set("name", "Test").Set(
+              "version", "1.0"))
+          .Build());
+
+  resolve_function->set_extension(empty_extension.get());

[James Cook, 2014/07/16 17:15:31]

The old code has set_has_callback(true) after this line. Is the omission
intentional?

[Yoyo Zhou, 2014/07/18 02:45:30]

No. Don't know why it works without it, though. I'll add it back.

+
+  TestFunctionDispatcherDelegate delegate;

[Yoyo Zhou, 2014/07/16 02:30:25]

Some of this setup (like here) is more difficult than before for writers of
tests. I'll have to look at some more tests where RunFunction.* functions are
used to determine whether this is a bad thing.

+  scoped_ptr<ExtensionFunctionDispatcher> dispatcher(
+      new ExtensionFunctionDispatcher(browser_context(), &delegate));
+
+  scoped_ptr<base::Value> result(
+      RunFunctionAndReturnSingleResult(resolve_function.get(),
+                                       "[\"127.0.0.1\"]",
+                                       browser_context(),
+                                       dispatcher.Pass()));
+  ASSERT_EQ(base::Value::TYPE_DICTIONARY, result->GetType());
+  base::DictionaryValue* value =
+      static_cast<base::DictionaryValue*>(result.get());

[mmenke, 2014/07/16 14:05:54]

No need for a cast, just use:

base::DictionaryValue* dict = NULL;
ASSERT_TRUE(result->GetAsDictionary(&dict));

[Yoyo Zhou, 2014/07/18 02:45:31]

Done.

+
+  int resultCode;

[mmenke, 2014/07/16 14:05:54]

nit:  result_code.

[Yoyo Zhou, 2014/07/18 02:45:30]

Done.

+  EXPECT_TRUE(value->GetInteger("resultCode", &resultCode));
+  EXPECT_EQ(net::OK, resultCode);

[mmenke, 2014/07/16 14:05:54]

We're exposing Chrome network error codes to extensions?

[Yoyo Zhou, 2014/07/18 02:45:30]

Yes, in lots of places already; see
https://code.google.com/p/chromium/codesearch#search/&q=net::ok%20file:extens...

(In case you're concerned about adding this DEP on net/ to apps/shell, we're
actually going to move the apps/shell code to extensions/ soon, as James
mentioned above, so it won't be a new DEP after all since "+net" is already in
extensions/DEPS.)

[mmenke, 2014/07/18 21:32:30]

I'm not really concerned about more stuff depending on net/, it just seems weird
that the raw network error codes (Which don't really have any stability of
meaning guarantees - they don't change that often, but we occasionally split old
codes into new ones for debugging purposes) are exposed to extensions.

[Yoyo Zhou, 2014/07/22 00:10:30]

This happens in other APIs too, I think, e.g.
https://developer.chrome.com/apps/sockets_tcp

"The result code returned from the underlying network call. A negative value
indicates an error."

This is probably not ideal, but it's what we have right now... 

+
+  std::string address;
+  EXPECT_TRUE(value->GetString("address", &address));
+  EXPECT_EQ("127.0.0.1", address);
+}
+
+IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsResolveHostname) {
+  scoped_refptr<extensions::DnsResolveFunction> resolve_function(
+      new extensions::DnsResolveFunction());
+  scoped_refptr<extensions::Extension> empty_extension(
+      extensions::ExtensionBuilder()
+          .SetManifest(extensions::DictionaryBuilder().Set("name", "Test").Set(
+              "version", "1.0"))
+          .Build());
+
+  resolve_function->set_extension(empty_extension.get());
+  resolve_function->set_has_callback(true);
+
+  TestFunctionDispatcherDelegate delegate;
+  scoped_ptr<ExtensionFunctionDispatcher> dispatcher(
+      new ExtensionFunctionDispatcher(browser_context(), &delegate));
+
+  std::string function_arguments("[\"");
+  function_arguments += extensions::MockHostResolverCreator::kHostname;
+  function_arguments += "\"]";
+  scoped_ptr<base::Value> result(
+      RunFunctionAndReturnSingleResult(resolve_function.get(),
+                                       function_arguments,
+                                       browser_context(),
+                                       dispatcher.Pass()));
+  ASSERT_EQ(base::Value::TYPE_DICTIONARY, result->GetType());
+  base::DictionaryValue* value =
+      static_cast<base::DictionaryValue*>(result.get());

[James Cook, 2014/07/16 17:15:31]

ditto

[Yoyo Zhou, 2014/07/18 02:45:30]

Done.

+
+  int resultCode;

[James Cook, 2014/07/16 17:15:31]

ditto

[Yoyo Zhou, 2014/07/18 02:45:30]

Done.

+  EXPECT_TRUE(value->GetInteger("resultCode", &resultCode));
+  EXPECT_EQ(net::OK, resultCode);
+
+  std::string address;
+  EXPECT_TRUE(value->GetString("address", &address));
+  EXPECT_EQ(extensions::MockHostResolverCreator::kAddress, address);
+}