Merge lp:~gerboland/miral/fix-windowmanager-tests into lp:miral
- fix-windowmanager-tests
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 321 |
Proposed branch: | lp:~gerboland/miral/fix-windowmanager-tests |
Merge into: | lp:miral |
Diff against target: |
301 lines (+67/-10) 1 file modified
miral-qt/tests/modules/WindowManager/windowmodel_test.cpp (+67/-10) |
To merge this branch: | bzr merge lp:~gerboland/miral/fix-windowmanager-tests |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alan Griffiths | Approve | ||
Review via email: mp+305283@code.launchpad.net |
Commit message
[miral-qt] Fix failing tests due to lack of event loop, and fix mocking the Window size so can enable its test
Description of the change
Gerry Boland (gerboland) wrote : | # |
Alan Griffiths (alan-griffiths) wrote : | # |
+// windowSpec.
useless
Alan Griffiths (alan-griffiths) wrote : | # |
> I could add functions to wrap each notifier.* method with flush() call, but
> wasn't sure the test readability would be enhanced. Second opinion desired.
Well, I'd be tempted by a bit more a restructuring of the tests:
All the tests start with:
WindowModel
WindowModel model(¬ifier, nullptr); // no need for controller in this testcase
I don't see why these can't be moved into the fixture.
Then the fixture can have:
void processEvents(
{
}
and the tests can have:
processEven
{
});
Alan Griffiths (alan-griffiths) wrote : | # |
If you want to land this and (optionally) rework the tests later then I'm fine with that.
Gerry Boland (gerboland) wrote : | # |
Rework more significant, so I'd prefer doing it separately
Preview Diff
1 | === modified file 'miral-qt/tests/modules/WindowManager/windowmodel_test.cpp' |
2 | --- miral-qt/tests/modules/WindowManager/windowmodel_test.cpp 2016-09-07 14:11:13 +0000 |
3 | +++ miral-qt/tests/modules/WindowManager/windowmodel_test.cpp 2016-09-08 23:24:56 +0000 |
4 | @@ -34,10 +34,19 @@ |
5 | |
6 | namespace ms = mir::scene; |
7 | namespace mg = mir::graphics; |
8 | -using StubSurface = mir::test::doubles::StubSurface; |
9 | using StubSession = mir::test::doubles::StubSession; |
10 | using namespace testing; |
11 | |
12 | +struct SizedStubSurface : public mir::test::doubles::StubSurface |
13 | +{ |
14 | + mir::geometry::Size size() const override { return toMirSize(m_size); } |
15 | + |
16 | + void setSize(QSize size) { m_size = size; } |
17 | + |
18 | +private: |
19 | + QSize m_size; |
20 | +}; |
21 | + |
22 | |
23 | class WindowModelTest : public ::testing::Test |
24 | { |
25 | @@ -51,16 +60,18 @@ |
26 | miral::WindowInfo createMirALWindowInfo(QPoint position = {160, 320}, QSize size = {100, 200}) |
27 | { |
28 | const miral::Application app{stubSession}; |
29 | + stubSurface->setSize(size); |
30 | const miral::Window window{app, stubSurface}; |
31 | |
32 | ms::SurfaceCreationParameters windowSpec; |
33 | - windowSpec.of_size(toMirSize(size)); |
34 | +// windowSpec.of_size(toMirSize(size)); // useless, Window/Surface has the size actually used |
35 | windowSpec.of_position(toMirPoint(position)); |
36 | return miral::WindowInfo{window, windowSpec}; |
37 | } |
38 | |
39 | MirSurface *getMirSurfaceFromModel(const WindowModel &model, int index) |
40 | { |
41 | + flushEvents(); |
42 | return model.data(model.index(index, 0), WindowModel::SurfaceRole).value<MirSurface*>(); |
43 | } |
44 | |
45 | @@ -69,8 +80,26 @@ |
46 | return getMirSurfaceFromModel(model, index)->window(); |
47 | } |
48 | |
49 | + void SetUp() override |
50 | + { |
51 | + int argc = 0; |
52 | + char* argv[0]; |
53 | + qtApp = new QCoreApplication(argc, argv); // needed for event loop |
54 | + } |
55 | + |
56 | + void TearDown() override |
57 | + { |
58 | + delete qtApp; |
59 | + } |
60 | + |
61 | + void flushEvents() |
62 | + { |
63 | + qtApp->sendPostedEvents(); |
64 | + } |
65 | + |
66 | const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()}; |
67 | - const std::shared_ptr<StubSurface> stubSurface{std::make_shared<StubSurface>()}; |
68 | + const std::shared_ptr<SizedStubSurface> stubSurface{std::make_shared<SizedStubSurface>()}; |
69 | + QCoreApplication *qtApp; |
70 | }; |
71 | |
72 | /* |
73 | @@ -85,6 +114,7 @@ |
74 | auto mirWindowInfo = createMirALWindowInfo(); |
75 | |
76 | notifier.addWindow(mirWindowInfo); |
77 | + flushEvents(); |
78 | |
79 | EXPECT_EQ(1, model.count()); |
80 | } |
81 | @@ -103,6 +133,7 @@ |
82 | QSignalSpy spyCountChanged(&model, SIGNAL(countChanged())); |
83 | |
84 | notifier.addWindow(mirWindowInfo); |
85 | + flushEvents(); |
86 | |
87 | EXPECT_EQ(1, spyCountChanged.count()); |
88 | } |
89 | @@ -119,6 +150,7 @@ |
90 | auto mirWindowInfo = createMirALWindowInfo(); |
91 | |
92 | notifier.addWindow(mirWindowInfo); |
93 | + flushEvents(); |
94 | |
95 | auto miralWindow = getMirALWindowFromModel(model, 0); |
96 | EXPECT_EQ(mirWindowInfo.window(), miralWindow); |
97 | @@ -138,6 +170,7 @@ |
98 | |
99 | // Test removing the window |
100 | notifier.removeWindow(mirWindowInfo); |
101 | + flushEvents(); |
102 | |
103 | EXPECT_EQ(0, model.count()); |
104 | } |
105 | @@ -153,11 +186,13 @@ |
106 | |
107 | auto mirWindowInfo = createMirALWindowInfo(); |
108 | notifier.addWindow(mirWindowInfo); |
109 | + flushEvents(); |
110 | |
111 | // Test removing the window |
112 | QSignalSpy spyCountChanged(&model, SIGNAL(countChanged())); |
113 | |
114 | notifier.removeWindow(mirWindowInfo); |
115 | + flushEvents(); |
116 | |
117 | EXPECT_EQ(1, spyCountChanged.count()); |
118 | } |
119 | @@ -176,6 +211,7 @@ |
120 | |
121 | notifier.addWindow(mirWindowInfo1); |
122 | notifier.addWindow(mirWindowInfo2); |
123 | + flushEvents(); |
124 | |
125 | ASSERT_EQ(2, model.count()); |
126 | auto miralWindow1 = getMirALWindowFromModel(model, 0); |
127 | @@ -200,6 +236,7 @@ |
128 | |
129 | // Remove second window |
130 | notifier.removeWindow(mirWindowInfo2); |
131 | + flushEvents(); |
132 | |
133 | ASSERT_EQ(1, model.count()); |
134 | auto miralWindow = getMirALWindowFromModel(model, 0); |
135 | @@ -222,6 +259,7 @@ |
136 | |
137 | // Remove first window |
138 | notifier.removeWindow(mirWindowInfo1); |
139 | + flushEvents(); |
140 | |
141 | ASSERT_EQ(1, model.count()); |
142 | auto miralWindow = getMirALWindowFromModel(model, 0); |
143 | @@ -245,6 +283,7 @@ |
144 | notifier.removeWindow(mirWindowInfo1); |
145 | |
146 | notifier.addWindow(mirWindowInfo3); |
147 | + flushEvents(); |
148 | |
149 | ASSERT_EQ(2, model.count()); |
150 | auto miralWindow2 = getMirALWindowFromModel(model, 0); |
151 | @@ -270,6 +309,7 @@ |
152 | notifier.addWindow(mirWindowInfo3); |
153 | |
154 | notifier.removeWindow(mirWindowInfo2); |
155 | + flushEvents(); |
156 | |
157 | ASSERT_EQ(2, model.count()); |
158 | auto miralWindow1 = getMirALWindowFromModel(model, 0); |
159 | @@ -291,6 +331,7 @@ |
160 | |
161 | // Raise first window |
162 | notifier.raiseWindows({mirWindowInfo1.window()}); |
163 | + flushEvents(); |
164 | |
165 | ASSERT_EQ(1, model.count()); |
166 | auto topWindow = getMirALWindowFromModel(model, 0); |
167 | @@ -312,6 +353,7 @@ |
168 | |
169 | // Raise second window (currently on top) |
170 | notifier.raiseWindows({mirWindowInfo2.window()}); |
171 | + flushEvents(); |
172 | |
173 | // Check second window still on top |
174 | ASSERT_EQ(2, model.count()); |
175 | @@ -334,6 +376,7 @@ |
176 | |
177 | // Raise first window (currently at bottom) |
178 | notifier.raiseWindows({mirWindowInfo1.window()}); |
179 | + flushEvents(); |
180 | |
181 | // Check first window now on top |
182 | ASSERT_EQ(2, model.count()); |
183 | @@ -368,6 +411,8 @@ |
184 | // 2: Window1 |
185 | // 1: Window2 |
186 | // 0: Window3 |
187 | + flushEvents(); |
188 | + |
189 | ASSERT_EQ(3, model.count()); |
190 | auto topWindow = getMirALWindowFromModel(model, 2); |
191 | EXPECT_EQ(mirWindowInfo1.window(), topWindow); |
192 | @@ -400,6 +445,8 @@ |
193 | // Model should now be like this: |
194 | // 1: Window1 |
195 | // 0: Window2 |
196 | + flushEvents(); |
197 | + |
198 | ASSERT_EQ(2, model.count()); |
199 | auto topWindow = getMirALWindowFromModel(model, 1); |
200 | EXPECT_EQ(mirWindowInfo1.window(), topWindow); |
201 | @@ -435,6 +482,8 @@ |
202 | // 2: Window2 |
203 | // 1: Window1 |
204 | // 0: Window3 |
205 | + flushEvents(); |
206 | + |
207 | ASSERT_EQ(3, model.count()); |
208 | auto topWindow = getMirALWindowFromModel(model, 2); |
209 | EXPECT_EQ(mirWindowInfo2.window(), topWindow); |
210 | @@ -456,6 +505,7 @@ |
211 | |
212 | auto mirWindowInfo = createMirALWindowInfo(position); |
213 | notifier.addWindow(mirWindowInfo); |
214 | + flushEvents(); |
215 | |
216 | auto surface = getMirSurfaceFromModel(model, 0); |
217 | EXPECT_EQ(position, surface->position()); |
218 | @@ -479,6 +529,7 @@ |
219 | |
220 | // Move window, check new position set |
221 | notifier.moveWindow(mirWindowInfo, toMirPoint(newPosition)); |
222 | + flushEvents(); |
223 | |
224 | EXPECT_EQ(newPosition, surface->position()); |
225 | } |
226 | @@ -503,6 +554,7 @@ |
227 | |
228 | // Move window, check new position set |
229 | notifier.moveWindow(mirWindowInfo1, toMirPoint(newPosition)); |
230 | + flushEvents(); |
231 | |
232 | EXPECT_EQ(newPosition, surface->position()); |
233 | } |
234 | @@ -526,6 +578,7 @@ |
235 | |
236 | // Move window, check new position set |
237 | notifier.moveWindow(mirWindowInfo1, toMirPoint(QPoint(350, 420))); |
238 | + flushEvents(); |
239 | |
240 | // Ensure other window untouched |
241 | EXPECT_EQ(fixedPosition, surface->position()); |
242 | @@ -543,6 +596,7 @@ |
243 | |
244 | auto mirWindowInfo1 = createMirALWindowInfo(QPoint(), size); |
245 | notifier.addWindow(mirWindowInfo1); |
246 | + flushEvents(); |
247 | |
248 | auto surface = getMirSurfaceFromModel(model, 0); |
249 | EXPECT_EQ(size, surface->size()); |
250 | @@ -563,8 +617,9 @@ |
251 | |
252 | auto surface = getMirSurfaceFromModel(model, 0); |
253 | |
254 | - // Move window, check new position set |
255 | + // Resize window, check new size set |
256 | notifier.resizeWindow(mirWindowInfo1, toMirSize(newSize)); |
257 | + flushEvents(); |
258 | |
259 | EXPECT_EQ(newSize, surface->size()); |
260 | } |
261 | @@ -586,8 +641,9 @@ |
262 | |
263 | auto surface = getMirSurfaceFromModel(model, 0); |
264 | |
265 | - // Move window, check new position set |
266 | + // Resize window, check new size set |
267 | notifier.resizeWindow(mirWindowInfo1, toMirSize(newSize)); |
268 | + flushEvents(); |
269 | |
270 | EXPECT_EQ(newSize, surface->size()); |
271 | } |
272 | @@ -595,23 +651,24 @@ |
273 | /* |
274 | * Test: with 2 windows, ensure window resize does not impact other MirSurfaces |
275 | */ |
276 | -TEST_F(WindowModelTest, DISABLED_WindowResizeDoesNotTouchOtherMirSurfaces) |
277 | +TEST_F(WindowModelTest, WindowResizeDoesNotTouchOtherMirSurfaces) |
278 | { |
279 | WindowModelNotifier notifier; |
280 | WindowModel model(¬ifier, nullptr); // no need for controller in this testcase |
281 | |
282 | - QSize fixedPosition(300, 400); |
283 | + QSize fixedSize(300, 400); |
284 | |
285 | auto mirWindowInfo1 = createMirALWindowInfo(QPoint(), QSize(100, 200)); |
286 | - auto mirWindowInfo2 = createMirALWindowInfo(QPoint(), fixedPosition); |
287 | + auto mirWindowInfo2 = createMirALWindowInfo(QPoint(), fixedSize); |
288 | notifier.addWindow(mirWindowInfo1); |
289 | notifier.addWindow(mirWindowInfo2); |
290 | |
291 | auto surface = getMirSurfaceFromModel(model, 1); |
292 | |
293 | - // Move window |
294 | + // Resize window |
295 | notifier.resizeWindow(mirWindowInfo1, toMirSize(QSize(150, 220))); |
296 | + flushEvents(); |
297 | |
298 | // Ensure other window untouched |
299 | - EXPECT_EQ(fixedPosition, surface->size()); |
300 | + EXPECT_EQ(fixedSize, surface->size()); |
301 | } |
I could add functions to wrap each notifier.* method with flush() call, but wasn't sure the test readability would be enhanced. Second opinion desired.