Issue 4657 - Add Acceptable Ads API

test/FilterEngine.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-2016 Eyeo GmbH
+ * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include "BaseJsTest.h"
 #include <thread>
+#include <condition_variable>
 
 using namespace AdblockPlus;
 
+namespace AdblockPlus
+{
+  namespace Utils
+  {
+    inline bool BeginsWith(const std::string& str, const std::string& beginning)
+    {
+      return 0 == str.compare(0, beginning.size(), beginning);
+    }
+  }
+}
+
 namespace
 {
-  typedef std::shared_ptr<AdblockPlus::FilterEngine> FilterEnginePtr;
-
   class VeryLazyFileSystem : public LazyFileSystem
   {
   public:
     StatResult Stat(const std::string& path) const
     {
       return StatResult();
     }
   };
 
   template<class FileSystem, class LogSystem>
   class FilterEngineTestGeneric : public BaseJsTest
   {
   protected:
     FilterEnginePtr filterEngine;
 
     void SetUp()
     {
       BaseJsTest::SetUp();
       jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(new FileSystem));
       jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(new LazyWebRequest));
       jsEngine->SetLogSystem(AdblockPlus::LogSystemPtr(new LogSystem));
-      filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine));
+      filterEngine = AdblockPlus::FilterEngine::Create(jsEngine);
     }
   };
 
   typedef FilterEngineTestGeneric<LazyFileSystem, AdblockPlus::DefaultLogSystem> FilterEngineTest;
   typedef FilterEngineTestGeneric<VeryLazyFileSystem, LazyLogSystem> FilterEngineTestNoData;
 
   struct MockFilterChangeCallback
   {
     MockFilterChangeCallback(int& timesCalled) : timesCalled(timesCalled) {}
 
@@ skipping 26 matching lines @@
 
     void SetUp()
     {
       AdblockPlus::AppInfo appInfo;
       appInfo.name = "test";
       appInfo.version = "1.0.1";
       AdblockPlus::JsEnginePtr jsEngine = CreateJsEngine(appInfo);
       jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(new LazyFileSystem));
       mockWebRequest = new MockWebRequest;
       jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(mockWebRequest));
-      filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine));
+      filterEngine = AdblockPlus::FilterEngine::Create(jsEngine);
     }
   };
-
-  struct MockUpdateAvailableCallback
-  {
-    MockUpdateAvailableCallback(int& timesCalled) : timesCalled(timesCalled) {}
-
-    void operator()(const std::string&)
-    {
-      timesCalled++;
-    }
-
-  private:
-    // We currently cannot store timesCalled in the functor, see:
-    // https://issues.adblockplus.org/ticket/1378.
-    int& timesCalled;
-  };
-
-  // Workaround for https://issues.adblockplus.org/ticket/1397.
-  void NoOpUpdaterCallback(const std::string&) {}
 
   class FilterEngineWithFreshFolder : public ::testing::Test
   {
   protected:
     FileSystemPtr fileSystem;
     std::weak_ptr<JsEngine> weakJsEngine;
 
     void SetUp() override
     {
       fileSystem.reset(new DefaultFileSystem());
@@ skipping 30 matching lines @@
           return true;
         }
         catch (...)
         {
           return false;
         }
       };
       int i = 5;
       while ((i-- > 0 && weakJsEngine.lock()) || !safeRemove())
         std::this_thread::sleep_for(std::chrono::seconds(2));
+    }
+  };
+
+  class FilterEngineIsAllowedConnectionTest : public BaseJsTest
+  {
+    class MockWebRequest : public LazyWebRequest
+    {
+    public:
+      std::map</*beginning of url*/std::string, AdblockPlus::ServerResponse> responses;
+
+      AdblockPlus::ServerResponse GET(const std::string& url,
+        const AdblockPlus::HeaderList& requestHeaders) const
+      {
+        for (const auto& response : responses)
+        {
+          if (Utils::BeginsWith(url, response.first))
+          {
+            return response.second;
+          }
+        }
+        return LazyWebRequest::GET(url, requestHeaders);
+      }
+    };
+    class SyncStrings
+    {
+    public:
+      void Add(const std::string* value)
+      {
+        std::lock_guard<std::mutex> lock(mutex);
+        strings.emplace_back(!!value, value ? *value : "");
+      }
+      std::vector<std::pair<bool, std::string>> GetStrings() const
+      {
+        std::lock_guard<std::mutex> lock(mutex);
+        return strings;
+      }
+      void Clear()
+      {
+        std::lock_guard<std::mutex> lock(mutex);
+        strings.clear();
+      }
+    private:
+      mutable std::mutex mutex;
+      std::vector<std::pair<bool, std::string>> strings;
+    };
+  protected:
+    MockWebRequest* webRequest;
+    std::string subscriptionUrlPrefix;
+    FilterEngine::CreationParameters createParams;
+    SyncStrings capturedConnectionTypes;
+    bool isConnectionAllowed;
+    FilterEnginePtr filterEngine;
+
+    struct
+    {
+      std::string url;
+      std::mutex mutex;
+      std::condition_variable cv;
+    } downloadStatusChanged;
+
+    void SetUp()
+    {
+      BaseJsTest::SetUp();
+      jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(new LazyFileSystem()));
+      jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(webRequest = new MockWebRequest()));
+      jsEngine->SetLogSystem(AdblockPlus::LogSystemPtr(new LazyLogSystem()));
+
+      subscriptionUrlPrefix = "http://example";
+      ServerResponse exampleSubscriptionResponse;
+      exampleSubscriptionResponse.responseStatus = 200;
+      exampleSubscriptionResponse.status = WebRequest::NS_OK;
+      exampleSubscriptionResponse.responseText = "[Adblock Plus 2.0]\n||example.com";
+      webRequest->responses.emplace(subscriptionUrlPrefix, exampleSubscriptionResponse);
+      createParams.preconfiguredPrefs["first_run_subscription_auto_select"] = jsEngine->NewValue(false);
+      isConnectionAllowed = true;
+      createParams.isConnectionAllowedCallback = [this](const std::string* allowedConnectionType)->bool{
+        capturedConnectionTypes.Add(allowedConnectionType);
+        return isConnectionAllowed;
+      };
+      jsEngine->SetEventCallback("filterChange", [this](const JsValueList& params/*action, item*/)
+      {
+        ASSERT_EQ(2u, params.size());
+        if (params[0]->AsString() == "subscription.downloadStatus")
+        {
+          {
+            std::lock_guard<std::mutex> lock(downloadStatusChanged.mutex);
+            downloadStatusChanged.url = params[1]->GetProperty("url")->AsString();
+          }
+          downloadStatusChanged.cv.notify_one();
+        }
+      });
+    }
+
+    SubscriptionPtr EnsureExampleSubscriptionAndForceUpdate(const std::string& apppendToUrl = std::string())
+    {
+      if (!filterEngine)
+        filterEngine = FilterEngine::Create(jsEngine, createParams);
+      auto subscriptionUrl = subscriptionUrlPrefix + apppendToUrl;
+      auto subscription = filterEngine->GetSubscription(subscriptionUrl);
+      EXPECT_EQ(0u, subscription->GetProperty("filters")->AsList().size()) << subscriptionUrl;
+      EXPECT_TRUE(subscription->GetProperty("downloadStatus")->IsNull()) << subscriptionUrl;
+      subscription->UpdateFilters();
+      {
+        std::unique_lock<std::mutex> lock(downloadStatusChanged.mutex);
+        downloadStatusChanged.cv.wait_for(lock,
+          /*don't block tests forever*/std::chrono::seconds(5),
+          [this, subscriptionUrl]()->bool
+        {
+          return subscriptionUrl == downloadStatusChanged.url;
+        });
+        // Basically it's enough to wait only for downloadStatus although there
+        // is still some JS code being executed. Any following attempt to work
+        // with subscription object will result in execution of JS, which will
+        // be blocked until finishing of currently running code.
+      }
+      return subscription;
     }
   };
 }
 
 TEST_F(FilterEngineTest, FilterCreation)
 {
   AdblockPlus::FilterPtr filter1 = filterEngine->GetFilter("foo");
   ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, filter1->GetType());
   AdblockPlus::FilterPtr filter2 = filterEngine->GetFilter("@@foo");
   ASSERT_EQ(AdblockPlus::Filter::TYPE_EXCEPTION, filter2->GetType());
@@ skipping 312 matching lines @@
 
 TEST_F(FilterEngineTestNoData, FirstRunFlag)
 {
   ASSERT_TRUE(filterEngine->IsFirstRun());
 }
 
 TEST_F(FilterEngineTest, SetRemoveFilterChangeCallback)
 {
   int timesCalled = 0;
   MockFilterChangeCallback mockFilterChangeCallback(timesCalled);
-
+  std::this_thread::sleep_for(std::chrono::milliseconds(200));
   filterEngine->SetFilterChangeCallback(mockFilterChangeCallback);
   filterEngine->GetFilter("foo")->AddToList();
   EXPECT_EQ(1, timesCalled);
 
   filterEngine->RemoveFilterChangeCallback();
   filterEngine->GetFilter("foo")->RemoveFromList();
   EXPECT_EQ(1, timesCalled);
 }
 
 TEST_F(UpdaterTest, SetRemoveUpdateAvailableCallback)
 {
   mockWebRequest->response.status = 0;
   mockWebRequest->response.responseStatus = 200;
   mockWebRequest->response.responseText = "\
 {\
   \"test\": {\
     \"version\": \"1.0.2\",\
     \"url\": \"https://downloads.adblockplus.org/test-1.0.2.tar.gz?update\"\
   }\
 }";
 
   int timesCalled = 0;
-  MockUpdateAvailableCallback mockUpdateAvailableCallback(timesCalled);
-
-  filterEngine->SetUpdateAvailableCallback(mockUpdateAvailableCallback);
-  filterEngine->ForceUpdateCheck(&NoOpUpdaterCallback);
+  filterEngine->SetUpdateAvailableCallback([&timesCalled](const std::string&)->void
+  {
+    ++timesCalled;
+  });
+  filterEngine->ForceUpdateCheck();
   AdblockPlus::Sleep(100);
-  ASSERT_EQ(1, timesCalled);
+  EXPECT_EQ(1, timesCalled);
 
   filterEngine->RemoveUpdateAvailableCallback();
-  filterEngine->ForceUpdateCheck(&NoOpUpdaterCallback);
+  filterEngine->ForceUpdateCheck();
   AdblockPlus::Sleep(100);
-  ASSERT_EQ(1, timesCalled);
+  EXPECT_EQ(1, timesCalled);
+}
+
+TEST_F(UpdaterTest, ForceUpdateCheck)
+{
+  mockWebRequest->response.status = 0;
+  mockWebRequest->response.responseStatus = 200;
+  mockWebRequest->response.responseText = "\
+{\
+  \"test\": {\
+    \"version\": \"1.0.2\",\
+    \"url\": \"https://downloads.adblockplus.org/test-1.0.2.tar.gz?update\"\
+  }\
+}";
+
+  int called = 0; // 0 - not  called; 1 - once, no error; 2 - error
+  filterEngine->ForceUpdateCheck([&called](const std::string& error)->void
+  {
+    called = error.empty() ? 1 : 2;
+  });
+  AdblockPlus::Sleep(100);
+  EXPECT_EQ(1, called);
 }
 
 TEST_F(FilterEngineTest, DocumentWhitelisting)
 {
   filterEngine->GetFilter("@@||example.org^$document")->AddToList();
   filterEngine->GetFilter("@@||example.com^$document,domain=example.de")->AddToList();
 
   ASSERT_TRUE(filterEngine->IsDocumentWhitelisted(
       "http://example.org",
       std::vector<std::string>()));
@@ skipping 46 matching lines @@
       "http://example.co.uk",
       documentUrls1));
 }
 
 TEST_F(FilterEngineWithFreshFolder, LangAndAASubscriptionsAreChosenOnFirstRun)
 {
   AppInfo appInfo;
   appInfo.locale = "zh";
   const std::string langSubscriptionUrl = "https://easylist-downloads.adblockplus.org/easylistchina+easylist.txt";
   auto jsEngine = createJsEngine(appInfo);
-  auto filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine));
+  auto filterEngine = AdblockPlus::FilterEngine::Create(jsEngine);
   const auto subscriptions = filterEngine->GetListedSubscriptions();
   ASSERT_EQ(2u, subscriptions.size());
   SubscriptionPtr aaSubscription;
   SubscriptionPtr langSubscription;
   if (subscriptions[0]->IsAA())
   {
     aaSubscription = subscriptions[0];
     langSubscription = subscriptions[1];
-  } else if (subscriptions[1]->IsAA())
+  }
+  else if (subscriptions[1]->IsAA())
   {
     aaSubscription = subscriptions[1];
     langSubscription = subscriptions[0];
   }
   ASSERT_NE(nullptr, aaSubscription);
   ASSERT_NE(nullptr, langSubscription);
   EXPECT_EQ(langSubscriptionUrl, langSubscription->GetProperty("url")->AsString());
   EXPECT_TRUE(filterEngine->IsAAEnabled());
 }
 
 TEST_F(FilterEngineWithFreshFolder, DisableSubscriptionsAutoSelectOnFirstRun)
 {
   auto jsEngine = createJsEngine();
-  FilterEngine::Prefs preSettings;
-  preSettings["first_run_subscription_auto_select"] = jsEngine->NewValue(false);
-  auto filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine, preSettings));
+  FilterEngine::CreationParameters createParams;
+  createParams.preconfiguredPrefs["first_run_subscription_auto_select"] = jsEngine->NewValue(false);
+  auto filterEngine = AdblockPlus::FilterEngine::Create(jsEngine, createParams);
   const auto subscriptions = filterEngine->GetListedSubscriptions();
   EXPECT_EQ(0u, subscriptions.size());
   EXPECT_FALSE(filterEngine->IsAAEnabled());
 }
 
-TEST_F(FilterEngineTest, IsAAEnabled)
-{
-  // no subscription (because of preconfigured prefs.json and patterns.ini)
-  EXPECT_FALSE(filterEngine->IsAAEnabled());
-  // add subscription and enable
-  filterEngine->SetAAEnabled(true);
-  EXPECT_TRUE(filterEngine->IsAAEnabled());
-  // disable
-  filterEngine->SetAAEnabled(false);
-  EXPECT_FALSE(filterEngine->IsAAEnabled());
-  // only enable already existing disabled
-  filterEngine->SetAAEnabled(true);
-  EXPECT_TRUE(filterEngine->IsAAEnabled());
-  // remove
-  filterEngine->GetSubscription(filterEngine->GetAAURL())->RemoveFromList();
-  EXPECT_FALSE(filterEngine->IsAAEnabled());
-  // no subscription
-  EXPECT_FALSE(filterEngine->IsAAEnabled());
+namespace AA_ApiTest
+{
+  const std::string kOtherSubscriptionUrl = "https://non-existing-subscription.txt";
+  enum class AAStatus
+  {
+    absent,
+    enabled,
+    disabled_present
+  };
+
+  ::std::ostream& operator<<(std::ostream& os, AAStatus aaStatus)
+  {
+    switch (aaStatus)
+    {
+    case AAStatus::absent:
+      os << "absent";
+      break;
+    case AAStatus::enabled:
+      os << "enabled";
+      break;
+    case AAStatus::disabled_present:
+      os << "disabled_present";
+      break;
+    default:
+      ;
+    }
+    return os;
+  }
+
+  enum class Action
+  {
+    disable, enable, remove
+  };
+
+  ::std::ostream& operator<<(std::ostream& os, Action action)
+  {
+    switch (action)
+    {
+    case Action::disable:
+      os << "disable";
+      break;
+    case Action::enable:
+      os << "enable";
+      break;
+    case Action::remove:
+      os << "remove";
+      break;
+    default:
+      ;
+    }
+    return os;
+  }
+
+  struct Parameters
+  {
+    AAStatus initialAAStatus;
+    Action action;
+    AAStatus expectedAAStatus;
+    Parameters(AAStatus aaStatus, Action action)
+    {
+      // if `expect` is not called then no effect is expected, initialize it now
+      initialAAStatus = expectedAAStatus = aaStatus;
+      this->action = action;
+    }
+    Parameters& expect(AAStatus aaStatus)
+    {
+      expectedAAStatus = aaStatus;
+      return *this;
+    }
+    // it's merely to satisfy compiler (std::tuple requires default ctr) and
+    // testing internals even calls it.
+    Parameters()
+    {
+    }
+  };
+
+  // human readable printing for failed tests
+  ::std::ostream& operator<<(::std::ostream& os, const Parameters& params)
+  {
+    os << "initial AA: " << params.initialAAStatus
+       << " action: " << params.action
+       << " expected AA: " << params.expectedAAStatus;
+    return os;
+  }
+  class Test : public FilterEngineTest
+             , public ::testing::WithParamInterface<::testing::tuple<Parameters, /*number of other subscriptions*/uint8_t>>
+  {
+  public:
+    static std::vector<Parameters> VaryPossibleCases()
+    {
+      // AA API test matrix
+      // each column but other-subs is about AA subscription
+      // enabled exists other-subs action  => expected
+      //                                   => everywhere no effect on other subs
+      // 1.
+      // false   false  false      disable => no effect
+      // false   false  false      enable  => add and enable
+      //
+      // false   false  true       disable => no effect
+      // false   false  true       enable  => add and enable
+      // 2.
+      // false   true   false      disable => no effect
+      // false   true   false      enable  => enable
+      //
+      // false   true   true       disable => no effect
+      // false   true   true       enable  => enable
+      // 3.
+      // true    true   false      disable => disable
+      // ture    true   false      enable  => no effect
+      //
+      // true    true   true       disable => disable
+      // ture    true   true       enable  => no effect
+      // 4.
+      // false   true   false      remove  => remove
+      // false   true   true       remove  => remove
+      // ture    true   false      remove  => remove
+      // ture    true   true       remove  => remove
+      std::vector<Parameters> retValue;
+      // 1.
+      retValue.emplace_back(Parameters(AAStatus::absent, Action::disable));
+      retValue.emplace_back(Parameters(AAStatus::absent, Action::enable).expect(AAStatus::enabled));
+      // 2.
+      retValue.emplace_back(Parameters(AAStatus::disabled_present, Action::disable));
+      retValue.emplace_back(Parameters(AAStatus::disabled_present, Action::enable).expect(AAStatus::enabled));
+      // 3.
+      retValue.emplace_back(Parameters(AAStatus::enabled, Action::disable).expect(AAStatus::disabled_present));
+      retValue.emplace_back(Parameters(AAStatus::enabled, Action::enable));
+      // 4.
+      retValue.emplace_back(Parameters(AAStatus::disabled_present, Action::remove).expect(AAStatus::absent));
+      retValue.emplace_back(Parameters(AAStatus::enabled, Action::remove).expect(AAStatus::absent));
+      // since AA should not affect other subscriptions, the number of other
+      // subscriptions is not specified here, it goes as another item in test parameters tuple.
+      return retValue;
+    }
+  protected:
+    void init(AAStatus aaStatus, uint8_t otherSubscriptionsNumber)
+    {
+      // for the sake of simplicity test only with one suplimentary subscription
+      ASSERT_TRUE(otherSubscriptionsNumber == 0u || otherSubscriptionsNumber == 1u);
+
+      // this method also tests the result of intermediate steps.
+
+      {
+        // no subscription (because of preconfigured prefs.json and patterns.ini),
+        // though it should be enabled by default in a non-test environment, it's tested in
+        // corresponding tests.
+        const auto subscriptions = filterEngine->GetListedSubscriptions();
+        EXPECT_EQ(0u, subscriptions.size()); // no any subscription including AA
+        EXPECT_FALSE(filterEngine->IsAAEnabled());
+      }
+      if (otherSubscriptionsNumber == 1u)
+      {
+        auto subscription = filterEngine->GetSubscription(kOtherSubscriptionUrl);
+        ASSERT_TRUE(subscription);
+        subscription->AddToList();
+        const auto subscriptions = filterEngine->GetListedSubscriptions();
+        ASSERT_EQ(1u, subscriptions.size());
+        EXPECT_FALSE(subscriptions[0]->IsAA());
+        EXPECT_EQ(kOtherSubscriptionUrl, subscriptions[0]->GetProperty("url")->AsString());
+      }
+      if (isAASatusPresent(aaStatus))
+      {
+        filterEngine->SetAAEnabled(true); // add AA by enabling it
+        if (aaStatus == AAStatus::disabled_present)
+        {
+          filterEngine->SetAAEnabled(false);
+        }
+        testSubscriptionState(aaStatus, otherSubscriptionsNumber);
+      }
+    }
+    bool isAASatusPresent(AAStatus aaStatus)
+    {
+      return aaStatus != AAStatus::absent;
+    }
+    void testSubscriptionState(AAStatus aaStatus, int otherSubscriptionsNumber)
+    {
+      if (aaStatus == AAStatus::enabled)
+        EXPECT_TRUE(filterEngine->IsAAEnabled());
+      else
+        EXPECT_FALSE(filterEngine->IsAAEnabled());
+
+      SubscriptionPtr aaSubscription;
+      SubscriptionPtr otherSubscription;
+      const auto subscriptions = filterEngine->GetListedSubscriptions();
+      for (const auto& subscription : subscriptions)
+      {
+        (subscription->IsAA() ? aaSubscription : otherSubscription) = subscription;
+      }
+      if (otherSubscriptionsNumber == 1u)
+      {
+        if (isAASatusPresent(aaStatus))
+        {
+          EXPECT_EQ(2u, subscriptions.size());
+          EXPECT_TRUE(aaSubscription);
+          EXPECT_TRUE(otherSubscription);
+        }
+        else
+        {
+          EXPECT_EQ(1u, subscriptions.size());
+          EXPECT_FALSE(aaSubscription);
+          EXPECT_TRUE(otherSubscription);
+        }
+      }
+      else if (otherSubscriptionsNumber == 0u)
+      {
+        if (isAASatusPresent(aaStatus))
+        {
+          EXPECT_EQ(1u, subscriptions.size());
+          EXPECT_TRUE(aaSubscription);
+          EXPECT_FALSE(otherSubscription);
+        }
+        else
+        {
+          EXPECT_EQ(0u, subscriptions.size());
+        }
+      }
+    }
+  };
+
+  INSTANTIATE_TEST_CASE_P(AA_ApiTests, Test,
+    ::testing::Combine(::testing::ValuesIn(Test::VaryPossibleCases()), ::testing::Values<uint8_t>(0, 1)));
+
+  TEST_P(Test, VaryPossibleCases) {
+    const auto parameter = ::testing::get<0>(GetParam());
+    uint8_t otherSubscriptionsNumber = ::testing::get<1>(GetParam());
+    init(parameter.initialAAStatus, otherSubscriptionsNumber);
+
+    if (parameter.action == Action::enable)
+      filterEngine->SetAAEnabled(true);
+    else if (parameter.action == Action::disable)
+      filterEngine->SetAAEnabled(false);
+    else if (parameter.action == Action::remove)
+    {
+      SubscriptionPtr aaSubscription;
+      for (const auto& subscription : filterEngine->GetListedSubscriptions())
+      {
+        if (subscription->IsAA())
+        {
+          aaSubscription = subscription;
+          break;
+        }
+      }
+      ASSERT_TRUE(aaSubscription);
+      aaSubscription->RemoveFromList();
+    }
+
+    testSubscriptionState(parameter.expectedAAStatus, otherSubscriptionsNumber);
+  }
+}
+
+TEST_F(FilterEngineIsAllowedConnectionTest, AbsentCallbackAllowsUpdating)
+{
+  createParams.isConnectionAllowedCallback = FilterEngine::IsConnectionAllowedCallback();
+  auto subscription = EnsureExampleSubscriptionAndForceUpdate();
+  EXPECT_EQ("synchronize_ok", subscription->GetProperty("downloadStatus")->AsString());
+  EXPECT_EQ(1u, subscription->GetProperty("filters")->AsList().size());
+}
+
+TEST_F(FilterEngineIsAllowedConnectionTest, AllowingCallbackAllowsUpdating)
+{
+  // no stored allowed_connection_type preference
+  auto subscription = EnsureExampleSubscriptionAndForceUpdate();
+  EXPECT_EQ("synchronize_ok", subscription->GetProperty("downloadStatus")->AsString());
+  EXPECT_EQ(1u, subscription->GetProperty("filters")->AsList().size());
+  auto capturedConnectionTypes = this->capturedConnectionTypes.GetStrings();
+  ASSERT_EQ(1u, capturedConnectionTypes.size());
+  EXPECT_FALSE(capturedConnectionTypes[0].first);
+}
+
+TEST_F(FilterEngineIsAllowedConnectionTest, NotAllowingCallbackDoesNotAllowUpdating)
+{
+  isConnectionAllowed = false;
+  // no stored allowed_connection_type preference
+  auto subscription = EnsureExampleSubscriptionAndForceUpdate();
+  EXPECT_EQ("synchronize_connection_error", subscription->GetProperty("downloadStatus")->AsString());
+  EXPECT_EQ(0u, subscription->GetProperty("filters")->AsList().size());
+  auto capturedConnectionTypes = this->capturedConnectionTypes.GetStrings();
+  EXPECT_EQ(1u, capturedConnectionTypes.size());
+}
+
+TEST_F(FilterEngineIsAllowedConnectionTest, PredefinedAllowedConnectionTypeIsPassedToCallback)
+{
+  std::string predefinedAllowedConnectionType = "non-metered";
+  createParams.preconfiguredPrefs["allowed_connection_type"] = jsEngine->NewValue(predefinedAllowedConnectionType);
+  auto subscription = EnsureExampleSubscriptionAndForceUpdate();
+  EXPECT_EQ("synchronize_ok", subscription->GetProperty("downloadStatus")->AsString());
+  EXPECT_EQ(1u, subscription->GetProperty("filters")->AsList().size());
+  auto capturedConnectionTypes = this->capturedConnectionTypes.GetStrings();
+  ASSERT_EQ(1u, capturedConnectionTypes.size());
+  EXPECT_TRUE(capturedConnectionTypes[0].first);
+  EXPECT_EQ(predefinedAllowedConnectionType, capturedConnectionTypes[0].second);
+}
+
+TEST_F(FilterEngineIsAllowedConnectionTest, ConfiguredConnectionTypeIsPassedToCallback)
+{
+  // FilterEngine->RemoveSubscription is not usable here because subscriptions
+  // are cached internally by URL. So, different URLs are used in diffirent
+  // checks.
+  {
+    std::string predefinedAllowedConnectionType = "non-metered";
+    createParams.preconfiguredPrefs["allowed_connection_type"] = jsEngine->NewValue(predefinedAllowedConnectionType);
+    auto subscription = EnsureExampleSubscriptionAndForceUpdate();
+    EXPECT_EQ("synchronize_ok", subscription->GetProperty("downloadStatus")->AsString());
+    EXPECT_EQ(1u, subscription->GetProperty("filters")->AsList().size());
+    auto capturedConnectionTypes = this->capturedConnectionTypes.GetStrings();
+    ASSERT_EQ(1u, capturedConnectionTypes.size());
+    EXPECT_TRUE(capturedConnectionTypes[0].first);
+    EXPECT_EQ(predefinedAllowedConnectionType, capturedConnectionTypes[0].second);
+  }
+  capturedConnectionTypes.Clear();
+  {
+    // set no value
+    filterEngine->SetAllowedConnectionType(nullptr);
+    auto subscription = EnsureExampleSubscriptionAndForceUpdate("subA");
+    EXPECT_EQ("synchronize_ok", subscription->GetProperty("downloadStatus")->AsString());
+    EXPECT_EQ(1u, subscription->GetProperty("filters")->AsList().size());
+    auto capturedConnectionTypes = this->capturedConnectionTypes.GetStrings();
+    ASSERT_EQ(1u, capturedConnectionTypes.size());
+    EXPECT_FALSE(capturedConnectionTypes[0].first);
+    subscription->RemoveFromList();
+    this->capturedConnectionTypes.Clear();
+  }
+  capturedConnectionTypes.Clear();
+  {
+    // set some value
+    std::string testConnection = "test connection";
+    filterEngine->SetAllowedConnectionType(&testConnection);
+    auto subscription = EnsureExampleSubscriptionAndForceUpdate("subB");
+    EXPECT_EQ("synchronize_ok", subscription->GetProperty("downloadStatus")->AsString());
+    EXPECT_EQ(1u, subscription->GetProperty("filters")->AsList().size());
+    auto capturedConnectionTypes = this->capturedConnectionTypes.GetStrings();
+    ASSERT_EQ(1u, capturedConnectionTypes.size());
+    EXPECT_TRUE(capturedConnectionTypes[0].first);
+    EXPECT_EQ(testConnection, capturedConnectionTypes[0].second);
+  }
 }

[Eric, 2016/12/05 14:40:58]

This is a bunch of unit tests all combined into one. Tests are much more useful
if they test individual behaviors. Written this way, when there are failures
it's typical that some will fail and some will pass, and it's much quicker to
locate errors.

Not tested:
-- enable when already enabled
-- disable when already disabled
-- disable after initialization
-- enable after removing AA from subscriptions

Line 646 looks redundant.

[sergei, 2017/03/17 15:55:25]

Added more tests and they run in each separate test.