Create DeclarativeConditionSet, DeclarativeActionSet, and DeclarativeRule templates

chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc

Index: chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
diff --git a/chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc b/chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6ebbf65ede9eec4f8037da65332c24965ce9d4d9
--- /dev/null
+++ b/chrome/browser/extensions/api/declarative/declarative_rule_unittest.cc
@@ -0,0 +1,382 @@
+// Copyright (c) 2013 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 "chrome/browser/extensions/api/declarative/declarative_rule.h"
+
+#include "base/message_loop.h"
+#include "base/test/values_test_util.h"
+#include "base/values.h"
+#include "chrome/common/extensions/matcher/url_matcher_constants.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+using json_schema_compiler::any::Any;
+using base::test::ParseJson;
+
+struct RecordingCondition {
+  typedef int MatchData;
+
+  URLMatcherConditionFactory* factory;
+  scoped_ptr<base::Value> value;
+
+  void GetURLMatcherConditionSets(
+      URLMatcherConditionSet::Vector* condition_sets) const {
+    // No condition sets.
+  }
+
+  bool has_url_matcher_condition_set() const {
+    return false;
+  }
+
+  static scoped_ptr<RecordingCondition> Create(
+      URLMatcherConditionFactory* url_matcher_condition_factory,
+      const base::Value& condition,
+      std::string* error) {
+    const base::DictionaryValue* dict;

[battre, 2013/01/16 15:41:10]

nit: initialize with NULL?

[Jeffrey Yasskin, 2013/01/16 23:11:14]

Sure, that might fix a problem with some gcc builds.

+    if (error_on_key &&
+        condition.GetAsDictionary(&dict) &&
+        dict->HasKey(error_on_key)) {
+      *error = "Found error key";
+      return scoped_ptr<RecordingCondition>();
+    }
+
+    scoped_ptr<RecordingCondition> result(new RecordingCondition());
+    result->factory = url_matcher_condition_factory;
+    result->value.reset(condition.DeepCopy());
+    return result.Pass();
+  }
+
+  static const char* error_on_key;
+};
+const char* RecordingCondition::error_on_key = NULL;

[battre, 2013/01/16 15:41:10]

I think it might be nicer to have error_on_key a const value and then
differentiating the test cases by sometimes having conditions with this key and
other conditions without the key. This way the tests don't have any permanent
side effects which might cause flakyness for sharded test execution.

[Jeffrey Yasskin, 2013/01/16 23:11:14]

Ok.

+typedef DeclarativeConditionSet<RecordingCondition> RecordingConditionSet;
+
+TEST(DeclarativeConditionTest, ErrorConditionSet) {
+  URLMatcher matcher;
+  RecordingConditionSet::AnyVector conditions;
+  conditions.push_back(make_linked_ptr(new Any(ParseJson("{\"key\": 1}"))));
+  conditions.push_back(make_linked_ptr(new Any(ParseJson("{\"bad_key\": 2}"))));
+  RecordingCondition::error_on_key = "bad_key";
+
+  std::string error;
+  scoped_ptr<RecordingConditionSet> result =
+      RecordingConditionSet::Create(matcher.condition_factory(),
+                                    conditions, &error);
+  EXPECT_EQ("Found error key", error);
+  ASSERT_FALSE(result);
+}
+
+TEST(DeclarativeConditionTest, CreateConditionSet) {
+  URLMatcher matcher;
+  RecordingConditionSet::AnyVector conditions;
+  conditions.push_back(make_linked_ptr(new Any(ParseJson("{\"key\": 1}"))));
+  conditions.push_back(make_linked_ptr(new Any(ParseJson("[\"val1\", 2]"))));
+  RecordingCondition::error_on_key = NULL;
+
+  // Test insertion
+  std::string error;
+  scoped_ptr<RecordingConditionSet> result =
+      RecordingConditionSet::Create(matcher.condition_factory(),
+                                    conditions, &error);
+  EXPECT_EQ("", error);
+  ASSERT_TRUE(result);
+  EXPECT_EQ(2u, result->conditions().size());
+
+  EXPECT_EQ(matcher.condition_factory(), result->conditions()[0]->factory);
+  EXPECT_TRUE(ParseJson("{\"key\": 1}")->Equals(
+      result->conditions()[0]->value.get()));
+}
+
+struct FulfillableCondition {
+  typedef int MatchData;
+
+  scoped_refptr<URLMatcherConditionSet> condition_set;
+  int condition_set_id;
+  int max_value;
+
+  URLMatcherConditionSet::ID url_matcher_condition_set_id() const {
+    return condition_set_id;
+  }
+
+  bool has_url_matcher_condition_set() const {
+    return true;
+  }
+
+  scoped_refptr<URLMatcherConditionSet> url_matcher_condition_set() const {
+    return condition_set;
+  }
+
+  void GetURLMatcherConditionSets(
+      URLMatcherConditionSet::Vector* condition_sets) const {
+    condition_sets->push_back(condition_set);
+  }
+
+  bool IsFulfilled(const std::set<URLMatcherConditionSet::ID>& url_matches,
+                   int match_data) const {

[battre, 2013/01/16 15:41:10]

nit/opt: replace int with MatchData?

[Jeffrey Yasskin, 2013/01/16 23:11:14]

MatchData mostly exists to communicate with the templates. In non-templated
code, avoiding the typedef tends to be more readable.

+    return ContainsKey(url_matches, condition_set_id) &&
+        match_data <= max_value;
+  }
+
+  static scoped_ptr<FulfillableCondition> Create(
+      URLMatcherConditionFactory* url_matcher_condition_factory,
+      const base::Value& condition,
+      std::string* error) {
+    scoped_ptr<FulfillableCondition> result(new FulfillableCondition());
+    const base::DictionaryValue* dict;
+    if (!condition.GetAsDictionary(&dict)) {
+      *error = "Expected dict";
+      return result.Pass();
+    }
+    if (!dict->GetInteger("url_id", &result->condition_set_id))
+      *error = "Expected integer at ['url_id']";
+    if (!dict->GetInteger("max", &result->max_value))
+      *error = "Expected integer at ['max']";
+    result->condition_set = new URLMatcherConditionSet(
+        result->condition_set_id,
+        URLMatcherConditionSet::Conditions());
+    return result.Pass();
+  }
+};
+
+TEST(DeclarativeConditionTest, FulfilConditionSet) {

[battre, 2013/01/16 15:41:10]

sp: Fulfill

[Jeffrey Yasskin, 2013/01/16 23:11:14]

But http://en.wiktionary.org/wiki/fulfil. I guess we're better off using the US
spelling.

+  typedef DeclarativeConditionSet<FulfillableCondition> FulfillableConditionSet;
+  FulfillableConditionSet::AnyVector conditions;
+  conditions.push_back(make_linked_ptr(new Any(ParseJson(
+      "{\"url_id\": 1, \"max\": 3}"))));
+  conditions.push_back(make_linked_ptr(new Any(ParseJson(
+      "{\"url_id\": 2, \"max\": 5}"))));
+  conditions.push_back(make_linked_ptr(new Any(ParseJson(
+      "{\"url_id\": 3, \"max\": 1}"))));
+
+  // Test insertion
+  std::string error;
+  scoped_ptr<FulfillableConditionSet> result =
+      FulfillableConditionSet::Create(NULL, conditions, &error);
+  ASSERT_EQ("", error);
+  ASSERT_TRUE(result);
+  EXPECT_EQ(3u, result->conditions().size());
+
+  std::set<URLMatcherConditionSet::ID> url_matches;
+  url_matches.insert(1);
+  EXPECT_TRUE(result->IsFulfilled(1, url_matches, 3));
+  EXPECT_FALSE(result->IsFulfilled(2, url_matches, 3));
+  EXPECT_FALSE(result->IsFulfilled(1, url_matches, 4));
+  url_matches.insert(2);
+  EXPECT_TRUE(result->IsFulfilled(2, url_matches, 4));
+  url_matches.clear();
+  url_matches.insert(3);
+  EXPECT_FALSE(result->IsFulfilled(3, url_matches, 2));

[battre, 2013/01/16 15:41:10]

does this test anything that is not tested in line 166?

[Jeffrey Yasskin, 2013/01/16 23:11:14]

I guess not.

+
+  // Check the condition sets:
+  URLMatcherConditionSet::Vector condition_sets;
+  result->GetURLMatcherConditionSets(&condition_sets);
+  ASSERT_EQ(3U, condition_sets.size());
+  EXPECT_EQ(1, condition_sets[0]->id());
+  EXPECT_EQ(2, condition_sets[1]->id());
+  EXPECT_EQ(3, condition_sets[2]->id());
+}
+
+// DeclarativeAction
+
+struct SummingAction {
+  typedef int ApplyInfo;
+
+  int increment;
+  int min_priority;
+
+  static scoped_ptr<SummingAction> Create(const base::Value& action,
+                                          std::string* error,
+                                          bool* bad_message) {
+    scoped_ptr<SummingAction> result(new SummingAction());
+    const base::DictionaryValue* dict = NULL;
+    EXPECT_TRUE(action.GetAsDictionary(&dict));
+    if (dict->HasKey("error")) {
+      EXPECT_TRUE(dict->GetString("error", error));
+      return result.Pass();
+    }
+    if (dict->HasKey("bad")) {
+      *bad_message = true;
+      return result.Pass();
+    }
+
+    EXPECT_TRUE(dict->GetInteger("value", &result->increment));
+    dict->GetInteger("priority", &result->min_priority);
+    return result.Pass();
+  }
+
+  void Apply(const std::string& extension_id,
+             const base::Time& install_time,
+             int* sum) const {

[battre, 2013/01/16 15:41:10]

nit: replace int with ApplyInfo?

[Jeffrey Yasskin, 2013/01/16 23:11:14]

ApplyInfo mostly exists to communicate with the templates. In non-templated
code, avoiding the typedef tends to be more readable.

+    *sum += increment;
+  }
+
+  int GetMinimumPriority() const {
+    return min_priority;
+  }
+};
+typedef DeclarativeActionSet<SummingAction> SummingActionSet;
+
+TEST(DeclarativeActionTest, ErrorActionSet) {
+  SummingActionSet::AnyVector actions;
+  actions.push_back(make_linked_ptr(new Any(ParseJson("{\"value\": 1}"))));
+  actions.push_back(make_linked_ptr(new Any(ParseJson(
+      "{\"error\": \"the error\"}"))));
+
+  std::string error;
+  bool bad = false;
+  scoped_ptr<SummingActionSet> result =
+      SummingActionSet::Create(actions, &error, &bad);
+  EXPECT_EQ("the error", error);
+  EXPECT_FALSE(bad);
+  EXPECT_FALSE(result);
+
+  actions.clear();
+  actions.push_back(make_linked_ptr(new Any(ParseJson("{\"value\": 1}"))));
+  actions.push_back(make_linked_ptr(new Any(ParseJson("{\"bad\": 3}"))));
+  result = SummingActionSet::Create(actions, &error, &bad);
+  EXPECT_EQ("", error);
+  EXPECT_TRUE(bad);
+  EXPECT_FALSE(result);
+}
+
+TEST(DeclarativeActionTest, ApplyActionSet) {
+  SummingActionSet::AnyVector actions;
+  actions.push_back(make_linked_ptr(new Any(ParseJson(
+      "{\"value\": 1,"
+      " \"priority\": 5}"))));
+  actions.push_back(make_linked_ptr(new Any(ParseJson("{\"value\": 2}"))));
+
+  // Test insertion
+  std::string error;
+  bool bad = false;
+  scoped_ptr<SummingActionSet> result =
+      SummingActionSet::Create(actions, &error, &bad);
+  EXPECT_EQ("", error);
+  EXPECT_FALSE(bad);
+  ASSERT_TRUE(result);
+  EXPECT_EQ(2u, result->actions().size());
+
+  int sum = 0;
+  result->Apply("ext_id", base::Time(), &sum);
+  EXPECT_EQ(3, sum);
+  EXPECT_EQ(5, result->GetMinimumPriority());
+}
+
+TEST(DeclarativeRuleTest, Create) {
+  typedef DeclarativeRule<FulfillableCondition, SummingAction> Rule;
+  linked_ptr<Rule::JsonRule> json_rule(new Rule::JsonRule);
+  ASSERT_TRUE(Rule::JsonRule::Populate(
+      *ParseJson("{ \n"
+                 "  \"id\": \"rule1\", \n"
+                 "  \"conditions\": [ \n"
+                 "    {\"url_id\": 1, \"max\": 3}, \n"
+                 "    {\"url_id\": 2, \"max\": 5}, \n"
+                 "  ], \n"
+                 "  \"actions\": [ \n"
+                 "    { \n"
+                 "      \"value\": 2 \n"
+                 "    } \n"
+                 "  ], \n"
+                 "  \"priority\": 200 \n"
+                 "}"),
+      json_rule.get()));
+
+  const char kExtensionId[] = "ext1";
+
+  base::Time install_time = base::Time::Now();
+
+  URLMatcher matcher;
+  std::string error;
+  scoped_ptr<Rule> rule(Rule::Create(matcher.condition_factory(), kExtensionId,
+                                     install_time, json_rule, NULL, &error));
+  EXPECT_EQ("", error);
+  ASSERT_TRUE(rule.get());
+
+  EXPECT_EQ(kExtensionId, rule->id().first);
+  EXPECT_EQ("rule1", rule->id().second);
+
+  EXPECT_EQ(200, rule->priority());
+
+  const Rule::ConditionSet& condition_set = rule->conditions();
+  const Rule::ConditionSet::Conditions conditions =

[battre, 2013/01/16 15:41:10]

nit: Conditions&

[Jeffrey Yasskin, 2013/01/16 23:11:14]

Done.

+      condition_set.conditions();
+  ASSERT_EQ(2u, conditions.size());
+  EXPECT_EQ(3, conditions[0]->max_value);
+  EXPECT_EQ(5, conditions[1]->max_value);
+
+  const Rule::ActionSet& action_set = rule->actions();
+  const Rule::ActionSet::Actions& actions = action_set.actions();
+  ASSERT_EQ(1u, actions.size());
+  EXPECT_EQ(2, actions[0]->increment);
+
+  int sum = 0;
+  rule->Apply(&sum);
+  EXPECT_EQ(2, sum);
+}
+
+bool AtLeastOneConditionAndAction(
+    DeclarativeConditionSet<FulfillableCondition>* conditions,
+    DeclarativeActionSet<SummingAction>* actions,
+    std::string* error) {
+  if (conditions->conditions().size() < 1) {

[battre, 2013/01/16 15:41:10]

nit: replace .size() < 1 with .empty()?

[Jeffrey Yasskin, 2013/01/16 23:11:14]

Done.

+    *error = "No conditions";
+    return false;
+  }
+  if (actions->actions().size() < 1) {

[battre, 2013/01/16 15:41:10]

dito

+    *error = "No actions";

[battre, 2013/01/16 15:41:10]

this path is never exercised -> remove

[Jeffrey Yasskin, 2013/01/16 23:11:14]

Done.

+    return false;
+  }
+  return true;
+}
+
+TEST(DeclarativeRuleTest, CheckConsistency) {
+  typedef DeclarativeRule<FulfillableCondition, SummingAction> Rule;
+  URLMatcher matcher;
+  std::string error;
+  linked_ptr<Rule::JsonRule> json_rule(new Rule::JsonRule);
+  const char kExtensionId[] = "ext1";
+
+  ASSERT_TRUE(Rule::JsonRule::Populate(
+      *ParseJson("{ \n"
+                 "  \"id\": \"rule1\", \n"
+                 "  \"conditions\": [ \n"
+                 "    {\"url_id\": 1, \"max\": 3}, \n"
+                 "    {\"url_id\": 2, \"max\": 5}, \n"
+                 "  ], \n"
+                 "  \"actions\": [ \n"
+                 "    { \n"
+                 "      \"value\": 2 \n"
+                 "    } \n"
+                 "  ], \n"
+                 "  \"priority\": 200 \n"
+                 "}"),
+      json_rule.get()));
+  scoped_ptr<Rule> rule(
+      Rule::Create(matcher.condition_factory(), kExtensionId, base::Time(),
+                   json_rule, &AtLeastOneConditionAndAction, &error));
+  EXPECT_TRUE(rule);
+  EXPECT_EQ("", error);
+
+  ASSERT_TRUE(Rule::JsonRule::Populate(
+      *ParseJson("{ \n"
+                 "  \"id\": \"rule1\", \n"
+                 "  \"conditions\": [ \n"
+                 "  ], \n"
+                 "  \"actions\": [ \n"
+                 "    { \n"
+                 "      \"value\": 2 \n"
+                 "    } \n"
+                 "  ], \n"
+                 "  \"priority\": 200 \n"
+                 "}"),
+      json_rule.get()));
+  rule = Rule::Create(matcher.condition_factory(), kExtensionId, base::Time(),
+                      json_rule, &AtLeastOneConditionAndAction, &error);
+  EXPECT_FALSE(rule);
+  EXPECT_EQ("No conditions", error);
+}
+
+}  // namespace extensions