Issue 5506 - Make the notification test work.

test/Notification.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 <iostream>
-#include <AdblockPlus/DefaultLogSystem.h>
 #include "BaseJsTest.h"
-#include "../src/DefaultTimer.h"

[sergei, 2017/09/12 09:22:48]

It seems these headers are not required.

[hub, 2017/09/12 12:59:09]

Done.

 
 using namespace AdblockPlus;
 
 namespace
 {
   class NotificationTest : public BaseJsTest
   {
   protected:
     void SetUp()
     {
@@ skipping 49 matching lines @@
       serverResponse.responseText = responseText;
       getCallback(serverResponse);
     }
   };
 
   // To run this test one needs to set INITIAL_DELAY to about 2000 msec
   // in notification.js.
   class NotificationMockWebRequestTest : public BaseJsTest
   {
   protected:
-    AdblockPlus::Sync sync;

[sergei, 2017/09/12 09:22:48]

It seems sync is not needed.

[hub, 2017/09/12 12:59:09]

yes it is. Line 112 and line 135

[sergei, 2017/09/12 13:34:09]

There are no other threads, so sync is not needed, it can be `bool
isNotificationCallbackCalled`. BTW, it's removed in another codereview. In case
we need such functionality we should rather use std::promise/std::future.

[hub, 2017/09/12 15:51:10]

Done.

+    bool isNotificationCallbackCalled;
+    DelayedTimer::SharedTasks timerTasks;
     void SetUp()
     {
+      isNotificationCallbackCalled = false;
       const char* responseJsonText = "{"
         "\"notifications\": [{"
           "\"id\": \"some id\","
           "\"type\": \"information\","
           "\"message\": {"
              "\"en-US\": \"message\""
           "},"
           "\"title\": \"Title\""
         "}]"
         "}";
 
-      AdblockPlus::Platform::CreationParameters platformParams;
-      platformParams.logSystem.reset(new AdblockPlus::DefaultLogSystem());

[sergei, 2017/09/12 09:22:48]

What about using of ThrowingLogSystem as earlier?

[hub, 2017/09/12 12:59:09]

Done.

-      platformParams.timer.reset(new ManualTimer());

[sergei, 2017/09/12 09:22:48]

I would recommend to use DelayedTimer and not create ManualTimer.

[hub, 2017/09/12 12:59:09]

DelayedTimer doesn't do what I want. The idea of ManualTimer is to ignore the
delay and just run them.

[sergei, 2017/09/12 13:34:09]

I still think that DelayedTimer is better here because it basically already has
the functionality of the pending tasks (should be a member of the fixture) and
we should run tasks until our callback is executed (isNotificationCallbackCalled
is set). 
So, the test body would be (NOT TESTED):
auto& filterEngine = platform->GetFilterEngine();
auto ii = timerTasks->begin();
while(!isNotificationCallbackCalled && ii != timerTasks->end())
{
  ii->callback();
  ii = timerTasks->erase(ii);
  filterEngine.ShowNextNotification();
}
EXPECT_TRUE(isNotificationCallbackCalled);

[hub, 2017/09/12 15:51:10]

I see.

Done.

-      platformParams.fileSystem.reset(new LazyFileSystem());
+      LazyFileSystem* fileSystem;
+      ThrowingPlatformCreationParameters platformParams;
+      platformParams.timer = DelayedTimer::New(timerTasks);
+      platformParams.fileSystem.reset(fileSystem = new LazyFileSystem());
       platformParams.webRequest.reset(new MockWebRequest(responseJsonText));
       platform.reset(new Platform(std::move(platformParams)));
 
+      CreateFilterEngine(*fileSystem, *platform);
       auto& filterEngine = platform->GetFilterEngine();

[sergei, 2017/09/12 09:22:48]

I think here is a dead lock, it should be as above
CreateFilterEngine(*fileSystem, *platform);

[hub, 2017/09/12 12:59:08]

Done.

       filterEngine.SetShowNotificationCallback(
         [this](Notification&& notification) {
-          sync.Set();
+          isNotificationCallbackCalled = true;
           EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notification.GetType());
           EXPECT_EQ("Title", notification.GetTexts().title);
           EXPECT_EQ("message", notification.GetTexts().message);
           notification.MarkAsShown();
         });
     }
   };
 }
 
 TEST_F(NotificationTest, NoNotifications)
 {
   EXPECT_FALSE(PeekNotification());
 }
 
 TEST_F(NotificationMockWebRequestTest, SingleNotification)
 {
   auto& filterEngine = platform->GetFilterEngine();
-  static_cast<ManualTimer&>(platform->GetTimer()).runOne();

[sergei, 2017/09/12 09:22:48]

I still think that it would be more reliable to fire timers with the 1 minute
timeout instead of a first one.

[hub, 2017/09/12 12:59:08]

I changed it to a runPending() which will run all pending timers.

-  filterEngine.ShowNextNotification();
-  EXPECT_TRUE(sync.WaitFor(std::chrono::seconds(4)));
+  auto ii = timerTasks->begin();
+  while(!isNotificationCallbackCalled && ii != timerTasks->end()) {
+    ii->callback();
+    ii = timerTasks->erase(ii);
+    filterEngine.ShowNextNotification();
+  }
+  EXPECT_TRUE(isNotificationCallbackCalled);
 }
 
 TEST_F(NotificationTest, AddNotification)
 {
   AddNotification("{"
       "type: 'critical',"
       "title: 'testTitle',"
       "message: 'testMessage',"
     "}");
   auto notification = PeekNotification();
@@ skipping 47 matching lines @@
 TEST_F(NotificationTest, Links)
 {
   AddNotification("{ id: 'id', links: ['link1', 'link2'] }");
   auto notification = PeekNotification();
   ASSERT_TRUE(notification);
   std::vector<std::string> notificationLinks = notification->GetLinks();
   ASSERT_EQ(2u, notificationLinks.size());
   EXPECT_EQ("link1", notificationLinks[0]);
   EXPECT_EQ("link2", notificationLinks[1]);
 }