Issue 1107 - Support notifications

test/Notification.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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"
 
 using namespace AdblockPlus;
 
-/* This define enables NotificationMockWebRequestTest but to run it

[Felix Dahlke, 2015/01/23 13:25:55]

Shouldn't this comment be right above the define in question?

Anyway, I think this is not a bad solution for now, but I also think we should
create a follow-up issue for improving this. Only achieving full test coverage
when manually editing notification.js is not ideal. Maybe we should show a
pragma message if this isn't defined, too?

[sergei, 2015/01/23 14:48:19]

I guess it's better to have it at the beginning of the file than somewhere in
the code. I would like to leave it as is and wait for the change in the core. I
guess till that moment we will switch to some decent compiler and it will be
unconditional.

+// This define enables NotificationMockWebRequestTest but to run it
 // one need to set INITIAL_DELAY to about 2000 msec in notification.js.
-#include <chrono>
-#include <thread>
-#define NotificationMockWebRequestTest_ENABLED
-//*/

[Felix Dahlke, 2015/01/23 13:25:55]

Seems like this can be removed :)

[sergei, 2015/01/23 14:48:19]

Changed.
It's a part of comment toggling. It also included <chrono> which is not
available in msvs2010.

commented:
/*
some code
some code
some code
//*/

uncommented:
//*
some code
some code
some code
//*/

JIC, it's also possible to have "branching"
/*
case A
/*/
case B
//*/

+//#define NotificationMockWebRequestTest_ENABLED
 
 namespace
 {
   typedef std::tr1::shared_ptr<FilterEngine> FilterEnginePtr;
 
   class NotificationTest : public BaseJsTest
   {
   protected:
     FilterEnginePtr filterEngine;
     void SetUp() override
     {
       BaseJsTest::SetUp();
       jsEngine->SetFileSystem(FileSystemPtr(new LazyFileSystem()));

[Felix Dahlke, 2015/01/23 13:25:55]

Why use make_shared below for this but not here?

[sergei, 2015/01/23 14:48:19]

I guess, because I've copied it and during the testing I changed the next line
manually.

-      jsEngine->SetWebRequest(std::tr1::make_shared<LazyWebRequest>());

[Felix Dahlke, 2015/01/23 13:25:55]

Why use make_shared here? Strictly speaking, this expects a WebRequestPtr, which
may or may not be an std::tr1::shared_ptr.

But I get the impression we should probably have make_shared typedefs
corresponding to all these shared_ptr typedefs we have, huh? Stuff for a
follow-up issue.

[sergei, 2015/01/23 14:48:19]

I don't think it makes sense to have typedef for make_shared.

+      jsEngine->SetWebRequest(WebRequestPtr(new LazyWebRequest()));
       jsEngine->SetLogSystem(LogSystemPtr(new DefaultLogSystem()));
       filterEngine = std::tr1::make_shared<FilterEngine>(jsEngine);
     }
 
-    void AddNotificaiton(const std::string& notification)

[Felix Dahlke, 2015/01/23 13:25:55]

Should be "AddNotification", huh?

[sergei, 2015/01/23 14:48:19]

thanks )

+    void AddNotification(const std::string& notification)
     {
       jsEngine->Evaluate("(function()"
       "{"
         "require('notification').Notification.addNotification(" + notification + ");"
       "})();");
     }
   };
 
   class MockWebRequest : public WebRequest
   {
   public:
-    std::string m_responseText;

[Felix Dahlke, 2015/01/23 13:25:55]

Hungarian again! :D

+    std::string responseText;
     explicit MockWebRequest(const std::string& notification)
-      : m_responseText(notification)
+      : responseText(notification)
     {
     }
     ServerResponse GET(const std::string& url,
-      const HeaderList& requestHeaders) const override
+      const HeaderList& requestHeaders) const
     {
       if (url.find("/notification.json") == std::string::npos)
       {
         return ServerResponse();
       }
       ServerResponse serverResponse;
       serverResponse.status = NS_OK;
       serverResponse.responseStatus = 200;
-      serverResponse.responseText = m_responseText;
+      serverResponse.responseText = responseText;
       return serverResponse;
     }
   };
 
 #ifdef NotificationMockWebRequestTest_ENABLED
   class NotificationMockWebRequestTest : public BaseJsTest
   {
   protected:
     FilterEnginePtr filterEngine;
     void SetUp() override

[Felix Dahlke, 2015/01/23 13:25:55]

override is C++11, isn't it?

     {
       BaseJsTest::SetUp();
       jsEngine->SetFileSystem(std::tr1::make_shared<LazyFileSystem>());
       const char* responseJsonText = "{"
         "\"notifications\": [{"
           "\"id\": \"some id\","
           "\"type\": \"information\","
           "\"message\": {"
              "\"en-US\": \"message\""
           "},"
           "\"title\": \"Title\""
         "}]"
         "}";
       jsEngine->SetWebRequest(std::tr1::make_shared<MockWebRequest>(responseJsonText));
       jsEngine->SetLogSystem(LogSystemPtr(new DefaultLogSystem()));
       filterEngine = std::tr1::make_shared<FilterEngine>(jsEngine);
     }
   };
 #endif
 }
 
 TEST_F(NotificationTest, NoNotifications)
 {
   NotificationPtr notification = filterEngine->GetNextNotificationToShow();
-  EXPECT_EQ(nullptr, notification);
+  EXPECT_EQ(NULL, notification.get());
 }
 
 #ifdef NotificationMockWebRequestTest_ENABLED
 TEST_F(NotificationMockWebRequestTest, SingleNotification)
 {
-  std::this_thread::sleep_for(std::chrono::seconds(5)); // it's a hack

[Felix Dahlke, 2015/01/23 13:25:55]

Both std::thread and std::chrono are C++11. Can't we just use
AdblockPlus::Sleep() like all the other tests?

[sergei, 2015/01/23 14:48:19]

fixed

+  AdblockPlus::Sleep(5000/*msec*/); // it's a hack
   NotificationPtr notification = filterEngine->GetNextNotificationToShow();
   // try another one immediately to avoid queuing of the next notification by
   // the timer.
-  EXPECT_EQ(nullptr, filterEngine->GetNextNotificationToShow().get());

[Felix Dahlke, 2015/01/23 13:25:55]

nullptr is C++11, too.

[sergei, 2015/01/23 14:48:19]

fixed

-  ASSERT_NE(nullptr, notification.get());
+  EXPECT_EQ(NULL, filterEngine->GetNextNotificationToShow().get());
+  ASSERT_TRUE(notification);
   EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notification->GetType());
   EXPECT_EQ("Title", notification->GetTitle());
   EXPECT_EQ("message", notification->GetMessageString());
 }
 #endif
 
 TEST_F(NotificationTest, AddNotification)
 {
-  AddNotificaiton("{"
+  AddNotification("{"
       "type: 'critical',"
       "title: 'testTitle',"
       "message: 'testMessage',"
     "}");
   NotificationPtr notification = filterEngine->GetNextNotificationToShow();
-  ASSERT_NE(nullptr, notification.get());
+  ASSERT_TRUE(notification);
   EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_CRITICAL, notification->GetType());
   EXPECT_EQ("testTitle", notification->GetTitle());
   EXPECT_EQ("testMessage", notification->GetMessageString());
 }
 
-TEST_F(NotificationTest, FilterByUrl1)

[Felix Dahlke, 2015/01/23 13:25:55]

Don't see any FitlerByUrl2 :)

+TEST_F(NotificationTest, FilterByUrl)
 {
-  AddNotificaiton("{ id: 'no-filter', type: 'critical' }");
-  AddNotificaiton("{ id: 'www.com', type: 'information',"
+  AddNotification("{ id: 'no-filter', type: 'critical' }");
+  AddNotification("{ id: 'www.com', type: 'information',"
     "urlFilters:['http://www.com']"
   "}");
-  AddNotificaiton("{ id: 'www.de', type: 'question',"
+  AddNotification("{ id: 'www.de', type: 'question',"
     "urlFilters:['http://www.de']"
   "}");
 
   NotificationPtr notification = filterEngine->GetNextNotificationToShow();
-  ASSERT_NE(nullptr, notification.get());
+  ASSERT_TRUE(notification);
   EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_CRITICAL, notification->GetType());
 
   notification = filterEngine->GetNextNotificationToShow("http://www.de");
-  ASSERT_NE(nullptr, notification.get());
+  ASSERT_TRUE(notification);
   EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_QUESTION, notification->GetType());
 
   notification = filterEngine->GetNextNotificationToShow("http://www.com");
-  ASSERT_NE(nullptr, notification.get());
+  ASSERT_TRUE(notification);
   EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notification->GetType());
 }
 
 TEST_F(NotificationTest, MarkAsShown)
 {
-  AddNotificaiton("{ id: 'id', type: 'question' }");
+  AddNotification("{ id: 'id', type: 'question' }");
   NotificationPtr notification = filterEngine->GetNextNotificationToShow();
-  EXPECT_NE(nullptr, notification);

[Felix Dahlke, 2015/01/23 13:25:55]

Shouldn't you compare to notification.get() here like above?

[sergei, 2015/01/23 14:48:19]

no, if we use nullptr.

+  EXPECT_TRUE(notification);
   notification = filterEngine->GetNextNotificationToShow();
-  ASSERT_NE(nullptr, notification);
+  ASSERT_TRUE(notification);
   notification->MarkAsShown();
-  EXPECT_EQ(nullptr, filterEngine->GetNextNotificationToShow().get());
+  EXPECT_EQ(NULL, filterEngine->GetNextNotificationToShow().get());
 }