Issue 1376 - Add tests for SetFilterChangeCallback

test/FilterEngine.cpp

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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
@@ skipping 30 matching lines @@
       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));
     }
   };
 
   typedef FilterEngineTestGeneric<LazyFileSystem, AdblockPlus::DefaultLogSystem> FilterEngineTest;
   typedef FilterEngineTestGeneric<VeryLazyFileSystem, LazyLogSystem> FilterEngineTestNoData;
+
+  struct MockFilterChangeCallback
+  {
+    MockFilterChangeCallback(int& timesCalled) : timesCalled(timesCalled) {}

[Felix Dahlke, 2014/09/16 09:37:19]

It's somewhat unintuitive that the functor doesn't store timesCalled itself, but
that wouldn't work - we pass callbacks by value. See:
https://issues.adblockplus.org/ticket/1378

[Wladimir Palant, 2014/12/09 21:44:41]

Yes, that's a footgun. Should we at least have a comment that the parameter of
this class should always outlive it? Still, how about a global variable instead?

[Felix Dahlke, 2015/05/29 23:06:40]

It just occurred to me that a shared_ptr should be the safest alternative.
However, considering that this is just a mock, and considering that
UpdateAvailableCallback uses the same approach, I'd rather leave this as it is
for now, and tackle both together with #1378.

+
+    void operator()(const std::string&, const AdblockPlus::JsValuePtr)
+    {
+      timesCalled++;
+    }
+
+  private:
+    int& timesCalled;
+  };
 }
 
 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());
   AdblockPlus::FilterPtr filter3 = filterEngine->GetFilter("example.com##foo");
   ASSERT_EQ(AdblockPlus::Filter::TYPE_ELEMHIDE, filter3->GetType());
@@ skipping 243 matching lines @@
 
 TEST_F(FilterEngineTest, FirstRunFlag)
 {
   ASSERT_FALSE(filterEngine->IsFirstRun());
 }
 
 TEST_F(FilterEngineTestNoData, FirstRunFlag)
 {
   ASSERT_TRUE(filterEngine->IsFirstRun());
 }
+
+TEST_F(FilterEngineTest, SetRemoveFilterChangeCallback)
+{
+  int timesCalled = 0;
+  MockFilterChangeCallback mockFilterChangeCallback(timesCalled);
+
+  filterEngine->SetFilterChangeCallback(mockFilterChangeCallback);
+  filterEngine->GetSubscription("foo")->AddToList();

[Felix Dahlke, 2014/09/16 09:37:19]

I tried this with fitlerEngine->GetFilter("foo")->AddToList(), but it did not
trigger "filter.added" as I would have expected. Any idea why?

[Wladimir Palant, 2014/12/09 21:44:41]

Not really, no. If the filter was added it should trigger a notification.

[Felix Dahlke, 2015/05/29 23:06:40]

Debugged this a bit. filter.added is actually not called for the first custom
filter that's being added, which is the case here. A new subscription is added,
which triggers subscription.title and subscription.added, but not filter.added.
Went for GetFilter here anyway, since I think that's a more reasonable use case
than this bogus subscription.

+  ASSERT_EQ(1, timesCalled);
+
+  filterEngine->RemoveFilterChangeCallback();
+  filterEngine->GetSubscription("foo")->RemoveFromList();
+  ASSERT_EQ(1, timesCalled);
+}