Issue 1376 - Add tests for SetFilterChangeCallback

test/FilterEngine.cpp

Index: test/FilterEngine.cpp
===================================================================
--- a/test/FilterEngine.cpp
+++ b/test/FilterEngine.cpp
@@ -48,6 +48,19 @@
 
   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)
@@ -311,3 +324,17 @@
 {
   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);
+}