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.

+// 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
//*/

+
+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->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 )

+    {
+      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

+    explicit MockWebRequest(const std::string& notification)
+      : m_responseText(notification)
+    {
+    }
+    ServerResponse GET(const std::string& url,
+      const HeaderList& requestHeaders) const override
+    {
+      if (url.find("/notification.json") == std::string::npos)
+      {
+        return ServerResponse();
+      }
+      ServerResponse serverResponse;
+      serverResponse.status = NS_OK;
+      serverResponse.responseStatus = 200;
+      serverResponse.responseText = m_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);
+}
+
+#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

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

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

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