[Extensions Bindings] Move signature parsing tests to their own file

extensions/renderer/api_signature_unittest.cc

Index: extensions/renderer/api_signature_unittest.cc
diff --git a/extensions/renderer/api_signature_unittest.cc b/extensions/renderer/api_signature_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..380008b92250499ada75e9379527de702fa0c665
--- /dev/null
+++ b/extensions/renderer/api_signature_unittest.cc
@@ -0,0 +1,320 @@
+// Copyright 2017 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 "extensions/renderer/api_signature.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/values.h"
+#include "extensions/renderer/api_binding_test.h"
+#include "extensions/renderer/api_binding_test_util.h"
+#include "extensions/renderer/api_type_reference_map.h"
+#include "extensions/renderer/argument_spec.h"
+#include "gin/converter.h"
+
+namespace extensions {
+namespace {
+
+using SpecVector = std::vector<std::unique_ptr<ArgumentSpec>>;
+
+std::unique_ptr<APISignature> OneString() {
+  auto string_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::STRING);
+  string_spec->set_name("string");
+  SpecVector specs;
+  specs.push_back(std::move(string_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> StringAndInt() {
+  auto string_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::STRING);
+  string_spec->set_name("string");
+  auto int_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::INTEGER);
+  int_spec->set_name("int");
+  SpecVector specs;
+  specs.push_back(std::move(string_spec));
+  specs.push_back(std::move(int_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> StringOptionalIntAndBool() {
+  auto string_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::STRING);
+  string_spec->set_name("string");
+  auto opt_int_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::INTEGER);
+  opt_int_spec->set_name("int");
+  opt_int_spec->set_optional(true);
+  auto bool_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::BOOLEAN);
+  bool_spec->set_name("bool");
+  SpecVector specs;
+  specs.push_back(std::move(string_spec));
+  specs.push_back(std::move(opt_int_spec));
+  specs.push_back(std::move(bool_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> OneObject() {
+  auto object_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::OBJECT);
+  object_spec->set_name("obj");
+  auto prop1_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::STRING);
+  auto prop2_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::STRING);
+  prop2_spec->set_optional(true);
+  ArgumentSpec::PropertiesMap properties;
+  properties["prop1"] = std::move(prop1_spec);
+  properties["prop2"] = std::move(prop2_spec);
+  object_spec->set_properties(std::move(properties));
+  SpecVector specs;
+  specs.push_back(std::move(object_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));

[jbroman, 2017/04/27 19:18:17]

super-nit: Or this could just be, if you'd want to skip the explicit push_back
calls (here and elsewhere):

return base::MakeUnique<APISignature>({std::move(object_spec)});

Or if you take my other suggestion, this might be moot.

[jbroman, 2017/04/27 19:59:05]

Update: urgh, nope, I'm wrong about this point.

[Devlin, 2017/04/27 20:55:46]

Yep, originally tried to do that (and failed).

+}
+
+std::unique_ptr<APISignature> NoArgs() {
+  return base::MakeUnique<APISignature>(SpecVector());
+}
+
+std::unique_ptr<APISignature> IntAndCallback() {
+  auto int_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::INTEGER);
+  int_spec->set_name("int");
+  auto callback_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::FUNCTION);
+  callback_spec->set_name("callback");
+  SpecVector specs;
+  specs.push_back(std::move(int_spec));
+  specs.push_back(std::move(callback_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> OptionalIntAndCallback() {
+  auto int_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::INTEGER);
+  int_spec->set_name("int");
+  int_spec->set_optional(true);
+  auto callback_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::FUNCTION);
+  callback_spec->set_name("callback");
+  SpecVector specs;
+  specs.push_back(std::move(int_spec));
+  specs.push_back(std::move(callback_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> OptionalCallback() {
+  auto callback_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::FUNCTION);
+  callback_spec->set_name("callback");
+  callback_spec->set_optional(true);
+  SpecVector specs;
+  specs.push_back(std::move(callback_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> IntAnyOptionalObjectOptionalCallback() {
+  auto int_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::INTEGER);
+  int_spec->set_name("int");
+  auto any_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::ANY);
+  any_spec->set_name("any");
+  auto object_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::OBJECT);
+  object_spec->set_name("obj");
+  object_spec->set_optional(true);
+  auto prop_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::INTEGER);
+  prop_spec->set_optional(true);
+  ArgumentSpec::PropertiesMap properties;
+  properties["prop"] = std::move(prop_spec);
+  object_spec->set_properties(std::move(properties));
+  auto callback_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::FUNCTION);
+  callback_spec->set_name("callback");
+  callback_spec->set_optional(true);
+  SpecVector specs;
+  specs.push_back(std::move(int_spec));
+  specs.push_back(std::move(any_spec));
+  specs.push_back(std::move(object_spec));
+  specs.push_back(std::move(callback_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}

[jbroman, 2017/04/27 19:18:17]

nit: Hmm. This is only unit tests, so maybe it's not a big deal, but this is
hard enough to follow that I start to wonder if we don't want a builder so that
this can look something like:

return base::MakeUnique<APISignature>(
    {ArgumentSpecBuilder(ArgumentType::INTEGER).set_name("int").Build(),
     ArgumentSpecBuilder(ArgumentType::ANY).set_name("any").Build(),
     ArgumentSpecBuilder(ArgumentType::OBJECT)
         .set_name("obj")
         .set_optional(true)
         .AddProperty("prop",
                      ArgumentSpecBuilder(ArgumentType::INTEGER)
                          .set_optional(true)
                          .Build())
         .Build(),
     ArgumentSpecBuilder(ArgumentType::FUNCTION)
         .set_name("callback")
         .set_optional(true)
         .Build()});

Might be useful elsewhere, though it would be mildly annoying to have to forward
all the setters. Could go further, by:
- adding helpers that set the name and optional fields, which are the most
common
- using implicit conversion to avoid having to explicitly Build() -- probably
going too far, but does make it pretty

return base::MakeUnique<APISignature>(
    {RequiredArg(ArgumentType::INTEGER, "int"),
     RequiredArg(ArgumentType::ANY, "any"),
     OptionalArg(ArgumentType::OBJECT, "obj")
         .AddProperty("prop", OptionalArg(ArgumentType::INTEGER)),
     OptionalArg(ArgumentType::FUNCTION, "callback")});

If these become sufficiently readable, it might be practical to inline them into
the tests in which they are used, so that the definition and assertions are
close together.

[jbroman, 2017/04/27 19:59:05]

To correct myself slightly here, for some frustrating reason you can't use
move-only types in an initializer list. There are workarounds though (so if you
like this approach maybe I can make it work and send it to you as a followup?).

[Devlin, 2017/04/27 20:55:45]

Interesting thought.  Short answer: why not!  Longer answer:

This sounds good modulo a few things:
- I don't think RequiredArg/OptionalArg add quite enough value to make them
worth it.
- I think this should remain test-only, since it's only syntactic sugar (and
limited at that), and we generally want to discourage hand-construction of
arguments.

+
+std::unique_ptr<APISignature> RefObj() {
+  auto ref_obj_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::REF);
+  ref_obj_spec->set_name("obj");
+  ref_obj_spec->set_ref("refObj");
+  SpecVector specs;
+  specs.push_back(std::move(ref_obj_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> RefEnum() {
+  auto ref_enum_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::REF);
+  ref_enum_spec->set_name("enum");
+  ref_enum_spec->set_ref("refEnum");
+  SpecVector specs;
+  specs.push_back(std::move(ref_enum_spec));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+}  // namespace
+
+class APISignatureTest : public APIBindingTest {
+ public:
+  APISignatureTest()
+      : type_refs_(APITypeReferenceMap::InitializeTypeCallback()) {}
+  ~APISignatureTest() override = default;
+
+  void SetUp() override {
+    APIBindingTest::SetUp();
+
+    auto ref_obj_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::OBJECT);
+    auto prop1_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::STRING);
+    auto prop2_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::INTEGER);
+    prop2_spec->set_optional(true);
+    ArgumentSpec::PropertiesMap properties;
+    properties["prop1"] = std::move(prop1_spec);
+    properties["prop2"] = std::move(prop2_spec);
+    ref_obj_spec->set_properties(std::move(properties));
+    type_refs_.AddSpec("refObj", std::move(ref_obj_spec));
+
+    auto ref_enum_spec = base::MakeUnique<ArgumentSpec>(ArgumentType::STRING);
+    ref_enum_spec->set_enum_values({"alpha", "beta"});
+    type_refs_.AddSpec("refEnum", std::move(ref_enum_spec));
+  }
+
+  void ExpectPass(const APISignature& signature,
+                  base::StringPiece arg_values,
+                  base::StringPiece expected_parsed_args,
+                  bool expect_callback) {
+    RunTest(signature, arg_values, expected_parsed_args, expect_callback, true);
+  }
+
+  void ExpectFailure(const APISignature& signature,
+                     base::StringPiece arg_values) {
+    RunTest(signature, arg_values, base::StringPiece(), false, false);
+  }
+
+ private:
+  void RunTest(const APISignature& signature,
+               base::StringPiece arg_values,
+               base::StringPiece expected_parsed_args,
+               bool expect_callback,
+               bool should_succeed) {
+    SCOPED_TRACE(arg_values);
+    v8::Local<v8::Context> context = MainContext();
+    v8::Local<v8::Value> v8_args = V8ValueFromScriptSource(context, arg_values);
+    ASSERT_FALSE(v8_args.IsEmpty());
+    ASSERT_TRUE(v8_args->IsArray());
+    std::vector<v8::Local<v8::Value>> vector_args;
+    ASSERT_TRUE(gin::ConvertFromV8(isolate(), v8_args, &vector_args));
+
+    std::unique_ptr<base::ListValue> result;
+    v8::Local<v8::Function> callback;
+    std::string error;
+    bool success = signature.ParseArgumentsToJSON(
+        context, vector_args, type_refs_, &result, &callback, &error);
+    EXPECT_EQ(should_succeed, success);
+    ASSERT_EQ(should_succeed, !!result);
+    EXPECT_EQ(expect_callback, !callback.IsEmpty());
+    if (should_succeed) {
+      EXPECT_EQ(ReplaceSingleQuotes(expected_parsed_args),
+                ValueToString(*result));
+    }
+  }
+
+  APITypeReferenceMap type_refs_;
+
+  DISALLOW_COPY_AND_ASSIGN(APISignatureTest);
+};
+
+TEST_F(APISignatureTest, Foo) {
+  v8::HandleScope handle_scope(isolate());
+
+  {
+    auto signature = OneString();
+    ExpectPass(*signature, "['foo']", "['foo']", false);
+    ExpectPass(*signature, "['']", "['']", false);
+    ExpectFailure(*signature, "[1]");
+    ExpectFailure(*signature, "[]");
+    ExpectFailure(*signature, "[{}]");
+    ExpectFailure(*signature, "['foo', 'bar']");
+  }
+
+  {
+    auto signature = StringAndInt();
+    ExpectPass(*signature, "['foo', 42]", "['foo',42]", false);
+    ExpectPass(*signature, "['foo', -1]", "['foo',-1]", false);
+    ExpectFailure(*signature, "[1]");
+    ExpectFailure(*signature, "['foo'];");
+    ExpectFailure(*signature, "[1, 'foo']");
+    ExpectFailure(*signature, "['foo', 'foo']");
+    ExpectFailure(*signature, "['foo', '1']");
+    ExpectFailure(*signature, "['foo', 2.3]");
+  }
+
+  {
+    auto signature = StringOptionalIntAndBool();
+    ExpectPass(*signature, "['foo', 42, true]", "['foo',42,true]", false);
+    ExpectPass(*signature, "['foo', true]", "['foo',null,true]", false);
+    ExpectFailure(*signature, "['foo', 'bar', true]");
+  }
+
+  {
+    auto signature = OneObject();
+    ExpectPass(*signature, "[{prop1: 'foo'}]", "[{'prop1':'foo'}]", false);
+    ExpectFailure(*signature,
+                  "[{ get prop1() { throw new Error('Badness'); } }]");
+  }
+
+  {
+    auto signature = NoArgs();
+    ExpectPass(*signature, "[]", "[]", false);
+    ExpectFailure(*signature, "[0]");
+    ExpectFailure(*signature, "['']");
+    ExpectFailure(*signature, "[null]");
+    ExpectFailure(*signature, "[undefined]");
+  }
+
+  {
+    auto signature = IntAndCallback();
+    ExpectPass(*signature, "[1, function() {}]", "[1]", true);
+    ExpectFailure(*signature, "[function() {}]");
+    ExpectFailure(*signature, "[1]");
+  }
+
+  {
+    auto signature = OptionalIntAndCallback();
+    ExpectPass(*signature, "[1, function() {}]", "[1]", true);
+    ExpectPass(*signature, "[function() {}]", "[null]", true);
+    ExpectFailure(*signature, "[1]");
+  }
+
+  {
+    auto signature = OptionalCallback();
+    ExpectPass(*signature, "[function() {}]", "[]", true);
+    ExpectPass(*signature, "[]", "[]", false);
+    ExpectPass(*signature, "[undefined]", "[]", false);
+    ExpectFailure(*signature, "[0]");
+  }
+
+  {
+    auto signature = IntAnyOptionalObjectOptionalCallback();
+    ExpectPass(*signature, "[4, {foo: 'bar'}, function() {}]",
+               "[4,{'foo':'bar'},null]", true);
+    ExpectPass(*signature, "[4, {foo: 'bar'}]", "[4,{'foo':'bar'},null]",
+               false);
+    ExpectPass(*signature, "[4, {foo: 'bar'}, {}]", "[4,{'foo':'bar'},{}]",
+               false);
+    ExpectFailure(*signature, "[4, function() {}]");
+    ExpectFailure(*signature, "[4]");
+  }
+}
+
+TEST_F(APISignatureTest, TypeRefsTest) {
+  v8::HandleScope handle_scope(isolate());
+
+  {
+    auto signature = RefObj();
+    ExpectPass(*signature, "[{prop1: 'foo'}]", "[{'prop1':'foo'}]", false);
+    ExpectPass(*signature, "[{prop1: 'foo', prop2: 2}]",
+               "[{'prop1':'foo','prop2':2}]", false);
+    ExpectFailure(*signature, "[{prop1: 'foo', prop2: 'a'}]");
+  }
+
+  {
+    auto signature = RefEnum();
+    ExpectPass(*signature, "['alpha']", "['alpha']", false);
+    ExpectPass(*signature, "['beta']", "['beta']", false);
+    ExpectFailure(*signature, "['gamma']");
+  }
+}
+
+}  // namespace extensions