Issue 4756164013195264: Issue 1376 - Add tests for SetFilterChangeCallback (Closed)
|
Created:
Sept. 16, 2014, 9:21 a.m. by Felix Dahlke Modified:
Aug. 13, 2015, 5:23 p.m. Reviewers:
sergei CC:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 1376 - Add tests for SetFilterChangeCallback
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Rebased, use GetFilter() #
Total comments: 4
Patch Set 3 : Rebased, use EXPECT instead of ASSERT #MessagesTotal messages: 9
http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... File test/FilterEngine.cpp (right): http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... test/FilterEngine.cpp:54: MockFilterChangeCallback(int& timesCalled) : timesCalled(timesCalled) {} 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 http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... test/FilterEngine.cpp:334: filterEngine->GetSubscription("foo")->AddToList(); I tried this with fitlerEngine->GetFilter("foo")->AddToList(), but it did not trigger "filter.added" as I would have expected. Any idea why?
http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... File test/FilterEngine.cpp (right): http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... test/FilterEngine.cpp:54: MockFilterChangeCallback(int& timesCalled) : timesCalled(timesCalled) {} On 2014/09/16 09:37:19, Felix H. Dahlke wrote: > 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 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? http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... test/FilterEngine.cpp:334: filterEngine->GetSubscription("foo")->AddToList(); On 2014/09/16 09:37:19, Felix H. Dahlke wrote: > I tried this with fitlerEngine->GetFilter("foo")->AddToList(), but it did not > trigger "filter.added" as I would have expected. Any idea why? Not really, no. If the filter was added it should trigger a notification.
Didn't like this one bitrotting around :) New patch set is up. http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... File test/FilterEngine.cpp (right): http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... test/FilterEngine.cpp:54: MockFilterChangeCallback(int& timesCalled) : timesCalled(timesCalled) {} On 2014/12/09 21:44:41, Wladimir Palant wrote: > On 2014/09/16 09:37:19, Felix H. Dahlke wrote: > > 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 > > 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? 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. http://codereview.adblockplus.org/4756164013195264/diff/5649050225344512/test... test/FilterEngine.cpp:334: filterEngine->GetSubscription("foo")->AddToList(); On 2014/12/09 21:44:41, Wladimir Palant wrote: > On 2014/09/16 09:37:19, Felix H. Dahlke wrote: > > I tried this with fitlerEngine->GetFilter("foo")->AddToList(), but it did not > > trigger "filter.added" as I would have expected. Any idea why? > > Not really, no. If the filter was added it should trigger a notification. 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.
Starting to bit rot again, I've added Sergei as reviewer and moved Wladimir to CC (feel quite free to review anyway, of course).
A couple of NITs. https://codereview.adblockplus.org/4756164013195264/diff/5639274879778816/tes... File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/4756164013195264/diff/5639274879778816/tes... test/FilterEngine.cpp:384: ASSERT_EQ(2, timesCalled); Nit: it could be a good practice to use EXPECT_* when the test can continue the execution. > https://code.google.com/p/googletest/wiki/Primer > Usually EXPECT_* are preferred, as they allow more than one failures to be reported in a test. However, you should use ASSERT_* if it doesn't make sense to continue when the assertion in question fails. https://codereview.adblockplus.org/4756164013195264/diff/5639274879778816/tes... test/FilterEngine.cpp:384: ASSERT_EQ(2, timesCalled); It would be also good to test when timesCalled is equal to 1 to better understand what is happening, although it may be look a little bit implementation specific.
New patch set is up. https://codereview.adblockplus.org/4756164013195264/diff/5639274879778816/tes... File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/4756164013195264/diff/5639274879778816/tes... test/FilterEngine.cpp:384: ASSERT_EQ(2, timesCalled); On 2015/08/06 08:09:37, sergei wrote: > It would be also good to test when timesCalled is equal to 1 to better > understand what is happening, although it may be look a little bit > implementation specific. That's not really possible - it's called two times as part of `AddToList()`. https://codereview.adblockplus.org/4756164013195264/diff/5639274879778816/tes... test/FilterEngine.cpp:384: ASSERT_EQ(2, timesCalled); On 2015/08/06 08:09:37, sergei wrote: > Nit: it could be a good practice to use EXPECT_* when the test can continue the > execution. > > https://code.google.com/p/googletest/wiki/Primer > > Usually EXPECT_* are preferred, as they allow more than one failures to be > reported in a test. However, you should use ASSERT_* if it doesn't make sense to > continue when the assertion in question fails. Hm, you're right, I think I should use it more often. In this case, one could not only see whether timesCalled was invoked as often as required here, but also whether it was called after the callback has been removed below. Done. I guess we should at some point go over all the libadblockplus tests and use EXPECT where it makes sense? We use it rarely so far.
The last patch set is merely a new diff, not the whole change.
Anyway, LGTM, especially since it's already pushed.
On 2015/08/13 14:39:55, sergei wrote: > The last patch set is merely a new diff, not the whole change. Whoops, true, I've replaced it. Had to merge changes from master, however, those are in the diff now. On 2015/08/13 14:40:51, sergei wrote: > Anyway, LGTM, especially since it's already pushed. It's only in a pushed feature branch, not in master. But I'll push it to master then :) 
|
