Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Delta Between Two Patch Sets: test/Notification.cpp

Issue 29539858: Issue 5506 - Make the notification test work. (Closed) Base URL: https://hg.adblockplus.org/libadblockplus/
Left Patch Set: This version work using a fake "ManualTimer" Created Sept. 11, 2017, 10:01 p.m.
Right Patch Set: Removed dead code from previous patch Created Sept. 12, 2017, 3:50 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « no previous file | no next file » | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-present eyeo GmbH 3 * Copyright (C) 2006-present eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12 * GNU General Public License for more details. 12 * GNU General Public License for more details.
13 * 13 *
14 * You should have received a copy of the GNU General Public License 14 * You should have received a copy of the GNU General Public License
15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. 15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
16 */ 16 */
17 17
18 #include <iostream>
19 #include <AdblockPlus/DefaultLogSystem.h>
20 #include "BaseJsTest.h" 18 #include "BaseJsTest.h"
21 #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.
22 19
23 using namespace AdblockPlus; 20 using namespace AdblockPlus;
24 21
25 namespace 22 namespace
26 { 23 {
27 class NotificationTest : public BaseJsTest 24 class NotificationTest : public BaseJsTest
28 { 25 {
29 protected: 26 protected:
30 void SetUp() 27 void SetUp()
31 { 28 {
(...skipping 49 matching lines...) Expand 10 before | Expand all | Expand 10 after
81 serverResponse.responseText = responseText; 78 serverResponse.responseText = responseText;
82 getCallback(serverResponse); 79 getCallback(serverResponse);
83 } 80 }
84 }; 81 };
85 82
86 // To run this test one needs to set INITIAL_DELAY to about 2000 msec 83 // To run this test one needs to set INITIAL_DELAY to about 2000 msec
87 // in notification.js. 84 // in notification.js.
88 class NotificationMockWebRequestTest : public BaseJsTest 85 class NotificationMockWebRequestTest : public BaseJsTest
89 { 86 {
90 protected: 87 protected:
91 AdblockPlus::Sync sync; 88 bool isNotificationCallbackCalled;
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,
hub 2017/09/12 15:51:10 Done.
89 DelayedTimer::SharedTasks timerTasks;
92 void SetUp() 90 void SetUp()
93 { 91 {
92 isNotificationCallbackCalled = false;
94 const char* responseJsonText = "{" 93 const char* responseJsonText = "{"
95 "\"notifications\": [{" 94 "\"notifications\": [{"
96 "\"id\": \"some id\"," 95 "\"id\": \"some id\","
97 "\"type\": \"information\"," 96 "\"type\": \"information\","
98 "\"message\": {" 97 "\"message\": {"
99 "\"en-US\": \"message\"" 98 "\"en-US\": \"message\""
100 "}," 99 "},"
101 "\"title\": \"Title\"" 100 "\"title\": \"Title\""
102 "}]" 101 "}]"
103 "}"; 102 "}";
104 103
105 AdblockPlus::Platform::CreationParameters platformParams; 104 LazyFileSystem* fileSystem;
106 platformParams.logSystem.reset(new AdblockPlus::DefaultLogSystem()); 105 ThrowingPlatformCreationParameters platformParams;
sergei 2017/09/12 09:22:48 What about using of ThrowingLogSystem as earlier?
hub 2017/09/12 12:59:09 Done.
107 platformParams.timer.reset(new ManualTimer()); 106 platformParams.timer = DelayedTimer::New(timerTasks);
sergei 2017/09/12 09:22:48 I would recommend to use DelayedTimer and not crea
hub 2017/09/12 12:59:09 DelayedTimer doesn't do what I want. The idea of M
sergei 2017/09/12 13:34:09 I still think that DelayedTimer is better here bec
hub 2017/09/12 15:51:10 I see. Done.
108 platformParams.fileSystem.reset(new LazyFileSystem()); 107 platformParams.fileSystem.reset(fileSystem = new LazyFileSystem());
109 platformParams.webRequest.reset(new MockWebRequest(responseJsonText)); 108 platformParams.webRequest.reset(new MockWebRequest(responseJsonText));
110 platform.reset(new Platform(std::move(platformParams))); 109 platform.reset(new Platform(std::move(platformParams)));
111 110
111 CreateFilterEngine(*fileSystem, *platform);
112 auto& filterEngine = platform->GetFilterEngine(); 112 auto& filterEngine = platform->GetFilterEngine();
sergei 2017/09/12 09:22:48 I think here is a dead lock, it should be as above
hub 2017/09/12 12:59:08 Done.
113 filterEngine.SetShowNotificationCallback( 113 filterEngine.SetShowNotificationCallback(
114 [this](Notification&& notification) { 114 [this](Notification&& notification) {
115 sync.Set(); 115 isNotificationCallbackCalled = true;
116 EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notificatio n.GetType()); 116 EXPECT_EQ(NotificationType::NOTIFICATION_TYPE_INFORMATION, notificatio n.GetType());
117 EXPECT_EQ("Title", notification.GetTexts().title); 117 EXPECT_EQ("Title", notification.GetTexts().title);
118 EXPECT_EQ("message", notification.GetTexts().message); 118 EXPECT_EQ("message", notification.GetTexts().message);
119 notification.MarkAsShown(); 119 notification.MarkAsShown();
120 }); 120 });
121 } 121 }
122 }; 122 };
123 } 123 }
124 124
125 TEST_F(NotificationTest, NoNotifications) 125 TEST_F(NotificationTest, NoNotifications)
126 { 126 {
127 EXPECT_FALSE(PeekNotification()); 127 EXPECT_FALSE(PeekNotification());
128 } 128 }
129 129
130 TEST_F(NotificationMockWebRequestTest, SingleNotification) 130 TEST_F(NotificationMockWebRequestTest, SingleNotification)
131 { 131 {
132 auto& filterEngine = platform->GetFilterEngine(); 132 auto& filterEngine = platform->GetFilterEngine();
133 static_cast<ManualTimer&>(platform->GetTimer()).runOne(); 133 auto ii = timerTasks->begin();
sergei 2017/09/12 09:22:48 I still think that it would be more reliable to fi
hub 2017/09/12 12:59:08 I changed it to a runPending() which will run all
134 filterEngine.ShowNextNotification(); 134 while(!isNotificationCallbackCalled && ii != timerTasks->end()) {
135 EXPECT_TRUE(sync.WaitFor(std::chrono::seconds(4))); 135 ii->callback();
136 ii = timerTasks->erase(ii);
137 filterEngine.ShowNextNotification();
138 }
139 EXPECT_TRUE(isNotificationCallbackCalled);
136 } 140 }
137 141
138 TEST_F(NotificationTest, AddNotification) 142 TEST_F(NotificationTest, AddNotification)
139 { 143 {
140 AddNotification("{" 144 AddNotification("{"
141 "type: 'critical'," 145 "type: 'critical',"
142 "title: 'testTitle'," 146 "title: 'testTitle',"
143 "message: 'testMessage'," 147 "message: 'testMessage',"
144 "}"); 148 "}");
145 auto notification = PeekNotification(); 149 auto notification = PeekNotification();
(...skipping 47 matching lines...) Expand 10 before | Expand all | Expand 10 after
193 TEST_F(NotificationTest, Links) 197 TEST_F(NotificationTest, Links)
194 { 198 {
195 AddNotification("{ id: 'id', links: ['link1', 'link2'] }"); 199 AddNotification("{ id: 'id', links: ['link1', 'link2'] }");
196 auto notification = PeekNotification(); 200 auto notification = PeekNotification();
197 ASSERT_TRUE(notification); 201 ASSERT_TRUE(notification);
198 std::vector<std::string> notificationLinks = notification->GetLinks(); 202 std::vector<std::string> notificationLinks = notification->GetLinks();
199 ASSERT_EQ(2u, notificationLinks.size()); 203 ASSERT_EQ(2u, notificationLinks.size());
200 EXPECT_EQ("link1", notificationLinks[0]); 204 EXPECT_EQ("link1", notificationLinks[0]);
201 EXPECT_EQ("link2", notificationLinks[1]); 205 EXPECT_EQ("link2", notificationLinks[1]);
202 } 206 }
LEFTRIGHT
« no previous file | no next file » | Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Toggle Comments ('s')

Powered by Google App Engine
This is Rietveld