[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..e3c0a814bfa433ffae81b2653fa007e0df7c36a7
--- /dev/null
+++ b/extensions/renderer/api_signature_unittest.cc
@@ -0,0 +1,281 @@
+// 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 "extensions/renderer/argument_spec_builder.h"
+#include "gin/converter.h"
+
+namespace extensions {
+namespace {
+
+using SpecVector = std::vector<std::unique_ptr<ArgumentSpec>>;
+

[jbroman, 2017/04/27 21:24:32]

nit: not a huge win, but you could (here, in ArgumentSpecBuilder, or as an
APISignature constructor if we wanted to allow it in production):

std::unique_ptr<APISignature>
MakeAPISignature(std::initializer_list<std::unique_ptr<ArgumentSpec>> arguments)
{
  SpecVector specs(std::make_move_iterator(arguments.begin()),
std::make_move_iterator(arguments.end()));
  return base::MakeUnique<APISignature>(std::move(specs));
}

Usage:

  return MakeAPISignature({
    spec1,
    spec2
  });

But this is getting a little into the weeds.

[Devlin, 2017/04/28 01:16:44]

Yep, mentioned this here:
https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...

(though perhaps after you had already entered this)

I think for now, that's a little bit overkill (too weeds-y :)), but depending on
how things shake out, that might change.

+std::unique_ptr<APISignature> OneString() {
+  SpecVector specs;
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::STRING, "string"));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> StringAndInt() {
+  SpecVector specs;
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::STRING, "string"));
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::INTEGER, "int"));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> StringOptionalIntAndBool() {
+  SpecVector specs;
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::STRING, "string"));
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::INTEGER, "int").MakeOptional());
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::BOOLEAN, "bool"));
+  return base::MakeUnique<APISignature>(std::move(specs));

[Devlin, 2017/04/27 21:04:43]

As alluded offline, we could get to the world of
return base::MakeUnique<APISignature>({
  spec1,
  spec2
});

if we were to have a ctor of APISignature take an
initializer_list<std::unique_ptr<ArgumentSpec>>.  For now, I don't think it's
quite worth it, but if we end up using this pattern in production more, that may
change.

+}
+
+std::unique_ptr<APISignature> OneObject() {
+  SpecVector specs;
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::OBJECT, "obj")
+          .AddProperty("prop1", ArgumentSpecBuilder(ArgumentType::STRING))
+          .AddProperty(
+              "prop2",
+              ArgumentSpecBuilder(ArgumentType::STRING).MakeOptional()));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> NoArgs() {
+  return base::MakeUnique<APISignature>(SpecVector());
+}
+
+std::unique_ptr<APISignature> IntAndCallback() {
+  SpecVector specs;
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::INTEGER, "int"));
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::FUNCTION, "callback"));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> OptionalIntAndCallback() {
+  SpecVector specs;
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::INTEGER, "int").MakeOptional());
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::FUNCTION, "callback"));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> OptionalCallback() {
+  SpecVector specs;
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::FUNCTION, "callback").MakeOptional());
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> IntAnyOptionalObjectOptionalCallback() {
+  SpecVector specs;
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::INTEGER, "int"));
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::ANY, "any"));
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::OBJECT, "obj")
+          .AddProperty(
+              "prop", ArgumentSpecBuilder(ArgumentType::INTEGER).MakeOptional())
+          .MakeOptional());
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::FUNCTION, "callback").MakeOptional());
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> RefObj() {
+  SpecVector specs;
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::REF, "obj").SetRef("refObj"));
+  return base::MakeUnique<APISignature>(std::move(specs));
+}
+
+std::unique_ptr<APISignature> RefEnum() {
+  SpecVector specs;
+  specs.push_back(
+      ArgumentSpecBuilder(ArgumentType::REF, "enum").SetRef("refEnum"));
+  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));

[jbroman, 2017/04/27 21:24:32]

or:

std::unique_ptr<ArgumentSpec> ref_obj_spec =
    ArgumentSpecBuilder(ArgumentType::OBJECT)
        .AddProperty("prop1", ArgumentSpecBuilder(ArgumentType::STRING))
        .AddProperty("prop2",
                     ArgumentSpecBuilder(ArgumentType::INTEGER).MakeOptional());
type_refs_.AddSpec("refObj", std::move(ref_obj_spec));

[Devlin, 2017/04/28 01:16:44]

Whoops!  Missed this one.  Thanks.

+
+    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));

[jbroman, 2017/04/27 21:24:32]

or (albeit not much difference):

type_refs_.AddSpec(
    "refEnum",
    ArgumentSpecBuilder(ArgumentType::STRING).SetEnumValues({"alpha", "beta"}));

[Devlin, 2017/04/28 01:16:44]

Done.

+  }
+
+  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) {

[jbroman, 2017/04/27 21:24:32]

nit: Foo is not a helpful test name.

[Devlin, 2017/04/28 01:16:44]

D'oh!  Thanks for catching this.  It's a terrible habit that I use these as
placeholders thinking I'll go back and fix them.  Maybe I'll finally learn.

+  v8::HandleScope handle_scope(isolate());
+
+  {
+    auto signature = OneString();

[jbroman, 2017/04/27 21:24:32]

nit: since these seem to all be from above an only used once, consider inlining
them next to the assertions. The names are good enough that it's not a big deal,
but it's nice not to have to jump up and down to double-check exactly what the
names of OneObject's properties were, etc.

[Devlin, 2017/04/28 01:16:44]

If it's okay, I'd like to refrain.  When we add more tests that use these same
signatures, I think it's more helpful to have them defined once.

[jbroman, 2017/04/28 01:30:54]

Sure, sounds good then.

+    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