Merge lp:~saviq/unity/phablet.notification-interface-tests into lp:unity/phablet

Proposed by Michał Sawicz on 2013-03-28
Status: Rejected
Rejected by: Michał Sawicz on 2013-05-16
Proposed branch: lp:~saviq/unity/phablet.notification-interface-tests
Merge into: lp:unity/phablet
Diff against target: 215 lines (+199/-0) 2 files modified
To merge this branch: bzr merge lp:~saviq/unity/phablet.notification-interface-tests
Reviewer Review Type Date Requested Status
Michał Sawicz Disapprove on 2013-05-16
PS Jenkins bot continuous-integration Approve on 2013-04-22
Michi Henning 2013-03-28 Pending
Mirco Müller 2013-03-28 Pending
Jussi Pakkanen 2013-03-28 Pending
Thomas Voß architecture 2013-03-28 Pending
Michael Zanetti code 2013-03-28 Pending
Review via email: mp+155914@code.launchpad.net

Commit Message

add Unity.Notifications interface tests

Description of the Change

This is the first installment of our "let's drive the shell-facing interfaces with QML tests" approach.

This test is failing (hence skipped), but can be run by `make testNotificationInterface`, and is an acceptance factor for the Notifications backend.

Please look through to see if everything is clear and comment away.

To post a comment you must log in.
Mirco Müller (macslow) wrote :

While the hint "synchronous" is implicitly know to be true, if a notification of Type.Confirmation is passed, we should still test for this particular hint.

Michał Sawicz (saviq) wrote :

I disagree, there's no value in passing that hint to the UI, it won't care about it, and if we move away from libnotify at some point, we'd have to artificially set that hint just for the sake of it being there.

kevin gunn (kgunn72) wrote :

maybe i'm early & this would be expected, but will there be additional failing tests added to determine what's valid? for instance, mutually exclusive typing enums, e.g. can a snap decision ever not have a button? or not contain text?...i would think not...e.g. its a requirement.

Michał Sawicz (saviq) wrote :

All the additional tests would happen on the C++ side. This set is only supposed to verify that the QML-exposed parts are working as expected.

Mirco Müller (macslow) wrote :

test_5_actions() should either check for Type.Interactive or have at least two actions defined. Going back to our discussion about the way how to define an interactive notification (and pass in the callback), we said we'd use actions too, but only a single one. Snap-decisions are meant to have at least two actions defined.

Michał Sawicz (saviq) wrote :

We have to balance out here, I'm not against adding such tests here, but then they have to be written as C++ tests anyway.

The idea is to have the smallest possible test suite in QML to ensure API is what we expect it to be. What, BTW, should happen when an app sends a SnapDecision with just one action? I know that's against the guidelines, but we might silently convert it to an Interactive notification.

BTW, what's the official naming? Ephemeral or Asynchronous?

522. By Michał Sawicz on 2013-03-28

rework most of the tests to fail, not only warn on mismatched interfaces

Michał Sawicz (saviq) wrote :

I reworked the tests to be a little more structured and a lot more robust.

523. By Michał Sawicz on 2013-03-28

fix comment

524. By Michał Sawicz on 2013-03-28

reorder the calls to wait for the dataChanged signal first, then verify the number of notifications

Mirco Müller (macslow) wrote :

> We have to balance out here, I'm not against adding such tests here, but then
> they have to be written as C++ tests anyway.

I agree.

> The idea is to have the smallest possible test suite in QML to ensure API is
> what we expect it to be. What, BTW, should happen when an app sends a
> SnapDecision with just one action? I know that's against the guidelines, but
> we might silently convert it to an Interactive notification.

No, this could lead to odd side-effects in terms of behaviour. The request of a notification of a specific type should always be explicit. An interactive notification should always use the dedicated hint "x-canonical-switch-to-application". I agree that we can then allow the triggering application to pass the single action via the actions parameter. If an application issues a wrongly formed notification, the backend should simply reject it. We have 3rd-party developer documentation to educate people about such requirements. Of course these need to be updated once we're done with the new implementation to reflect all the additions and changes we made.

> BTW, what's the official naming? Ephemeral or Asynchronous?

"Ephemeral" is official. "Asynchronous" is the internal/legacy term.

525. By Michał Sawicz on 2013-04-03

make Notifications.Model a registered singleton instead of a context property

526. By Michał Sawicz on 2013-04-03

remove numbers from tests

527. By Michał Sawicz on 2013-04-03

add documentation for the tests

528. By Michał Sawicz on 2013-04-03

fix references to enumerations

529. By Michał Sawicz on 2013-04-04

fix two remaining test comments

530. By Michał Sawicz on 2013-04-04

two more comment fixes

531. By Michał Sawicz on 2013-04-05

fix enum references, verify there's no notifications on init

Michael Zanetti (mzanetti) wrote :

Ending test functions with _data() has a special meaning in QML. While this might still work, its very confusing to someone who knows the meaning of _data() functions for tests. I'd vote for either removing the _data() or change it in a way that it actually makes use of that mechanism.

532. By Michał Sawicz on 2013-04-11

rename _data tests to avoid confusion with data-driven tests

Michał Sawicz (saviq) wrote :

Indeed, fixed.

533. By Michał Sawicz on 2013-04-11

merge trunk

Daniel d'Andrada (dandrader) wrote :

coding style: some lines are too long. I think we should strive for a maximum of around 90 chars so that having windows with code side-by-side is possible.

Daniel d'Andrada (dandrader) wrote :

147 + compare(repeater.itemAt(1).data.type, Notifications.Ephemeral, "the second notification should be of Ephemeral type");

Was that a typo?
s/Notifications.Ephemeral/Notifications.Type.Ephemeral

534. By Michał Sawicz on 2013-04-15

missed a .Type

535. By Michał Sawicz on 2013-04-15

wrap the code to 90 lines

Michał Sawicz (saviq) wrote :

Fixed on both accounts.

536. By Michał Sawicz on 2013-04-15

merge trunk

537. By Michał Sawicz on 2013-04-15

add copyright header to tst_NotificationInterface.qml

538. By Michał Sawicz on 2013-04-15

add missing CMakeLists.txt

Michael Zanetti (mzanetti) wrote :

coding style: Wrapping lines at 90 chars makes half of my screen empty while making it clumsy to read being squeezed on the left side.

Jokes aside... I think all editors nowadays support automatically wrapping lines. But there is no editor that supports automatic removal of newlines even though there is plenty of space on the screen. Also we have screens with 2880x1800 pixels and 30 inches of space - as a developer, usually multiple of those side by side.

IMO we should not follow 1980's style guides and manually wrap lines.

Michi Henning (michihenning) wrote :

I don't think this is an issue of technology. It's more about ergonomics. Long lines are harder to read than short ones. But I wouldn't get too obsessive about it. Somewhere around 120-130 columns is probably where things start to go out of hand, and two lines of 65 characters each would be easier to read.

Somewhere around 180 columns, we are definitely crossing over into insanity…

539. By Michał Sawicz on 2013-04-19

merge trunk

540. By Michał Sawicz on 2013-04-22

merge trunk

541. By Michał Sawicz on 2013-04-22

relax column limit, don't expect Notification::completed to be available to QML, s/int/number/

Michał Sawicz (saviq) wrote :

This has now been "converted" to lp:~saviq/unity-api/add-shell-notifications-api for merging into the separate lp:unity-api project, and mock plugins implemented to make sure that these pass.

review: Disapprove

Unmerged revisions

541. By Michał Sawicz on 2013-04-22

relax column limit, don't expect Notification::completed to be available to QML, s/int/number/

540. By Michał Sawicz on 2013-04-22

merge trunk

539. By Michał Sawicz on 2013-04-19

merge trunk

538. By Michał Sawicz on 2013-04-15

add missing CMakeLists.txt

537. By Michał Sawicz on 2013-04-15

add copyright header to tst_NotificationInterface.qml

536. By Michał Sawicz on 2013-04-15

merge trunk

535. By Michał Sawicz on 2013-04-15

wrap the code to 90 lines

534. By Michał Sawicz on 2013-04-15

missed a .Type

533. By Michał Sawicz on 2013-04-11

merge trunk

532. By Michał Sawicz on 2013-04-11

rename _data tests to avoid confusion with data-driven tests

Preview Diff

1=== modified file 'tests/qmltests/CMakeLists.txt'
2--- tests/qmltests/CMakeLists.txt 2013-04-22 09:57:23 +0000
3+++ tests/qmltests/CMakeLists.txt 2013-04-22 12:17:29 +0000
4@@ -27,6 +27,7 @@
5 add_qml_test(Greeter Clock)
6 add_qml_test(Panel IndicatorItem)
7 add_qml_test(utils/Unity/Test UnityTest)
8+add_qml_test(interfaces NotificationInterface NO_ADD_TEST NO_TARGETS)
9
10 set(qmltest_DEFAULT_TARGETS qmluitests qmltests)
11 set(qmltest_DEFAULT_NO_ADD_TEST TRUE)
12
13=== added directory 'tests/qmltests/interfaces'
14=== added file 'tests/qmltests/interfaces/tst_NotificationInterface.qml'
15--- tests/qmltests/interfaces/tst_NotificationInterface.qml 1970-01-01 00:00:00 +0000
16+++ tests/qmltests/interfaces/tst_NotificationInterface.qml 2013-04-22 12:17:29 +0000
17@@ -0,0 +1,198 @@
18+/*
19+ * Copyright 2013 Canonical Ltd.
20+ *
21+ * This program is free software; you can redistribute it and/or modify
22+ * it under the terms of the GNU General Public License as published by
23+ * the Free Software Foundation; version 3.
24+ *
25+ * This program is distributed in the hope that it will be useful,
26+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
27+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
28+ * GNU General Public License for more details.
29+ *
30+ * You should have received a copy of the GNU General Public License
31+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
32+ */
33+
34+import QtQuick 2.0
35+import QtTest 1.0
36+import Unity.Notifications 0.1 as Notifications
37+import Unity.Notifications.Mocks 0.1 as NotificationMocks
38+
39+TestCase {
40+
41+ NotificationMocks.Source {
42+ id: source
43+ }
44+
45+ Repeater {
46+ id: repeater
47+ model: Notifications.Model
48+ delegate: Item {
49+ property var data: model
50+ property Repeater actionRepeater
51+
52+ actionRepeater: Repeater {
53+ model: parent.data.actions
54+ delegate: Item {
55+ property var data: model
56+ }
57+ }
58+ }
59+ }
60+
61+ SignalSpy {
62+ id: dataSpy
63+ target: Notifications.Model
64+ signalName: "dataChanged"
65+ }
66+
67+ function initTestCase() {
68+ Notifications.Model.source = source;
69+ }
70+
71+ function init() {
72+ Notifications.Model.confirmationPlaceholder = false;
73+ tryCompare(repeater, "count", "0", "There should be no notifications")
74+ }
75+
76+ function cleanup() {
77+ // dismiss all Notifications
78+ for (var i = 0; i < repeater.count; i++) {
79+ repeater.itemAt(i).data.notification.dismissed()
80+ }
81+ dataSpy.clear()
82+ }
83+
84+ /* make sure all the required types are registered */
85+ function test_types() {
86+ compare(typeof Notifications.Urgency.Low, "number",
87+ "there should be an Urgency::Low enum");
88+ compare(typeof Notifications.Urgency.Normal, "number",
89+ "there should be an Urgency::Normal enum");
90+ compare(typeof Notifications.Urgency.Critical, "number",
91+ "there should be an Urgency::Critical enum");
92+ compare(typeof Notifications.Type.Confirmation, "number",
93+ "there should be a Type::Confirmation enum");
94+ compare(typeof Notifications.Type.Ephemeral, "number",
95+ "there should be a Type::Ephemeral enum");
96+ compare(typeof Notifications.Type.Interactive, "number",
97+ "there should be a Type::Interactive enum");
98+ compare(typeof Notifications.Type.SnapDecision, "number",
99+ "there should be a Type::SnapDecision enum");
100+ compare(typeof Notifications.Type.Placeholder, "number",
101+ "there should be a Type::Placeholder enum");
102+ compare(typeof Notifications.Hint.ButtonTint, "number",
103+ "there should be a Hint::ButtonTint enum");
104+ compare(typeof Notifications.Hint.IconOnly, "number",
105+ "there should be a Hint::IconOnly enum");
106+ compare(typeof Notifications.Notification, "object",
107+ "Notification should be a registered type");
108+ compare(typeof Notifications.Action, "object",
109+ "Action should be a registered type");
110+ compare(typeof Notifications.Model, "object",
111+ "Notifications.Model should be a registered singleton");
112+ compare(typeof Notifications.Model.source, "object",
113+ "Notifications.Model should have a source property");
114+ compare(typeof Notifications.Model.confirmationPlaceholder, "boolean",
115+ "Notifications.Model should have a confirmationPlaceholder property");
116+ }
117+
118+ /* make sure all the required roles and methods of Notification are exposed */
119+ function test_notification_members() {
120+ source.send({
121+ "type": Notifications.Type.Interactive,
122+ });
123+ tryCompare(repeater, "count", 1, "there should be one notification");
124+
125+ var data = repeater.itemAt(0).data;
126+ compare(typeof data.type, "number",
127+ "NotificationModel should expose a \"type\" role of type Type");
128+ compare(typeof data.urgency, "number",
129+ "NotificationModel should expose a \"urgency\" role of type Urgency");
130+ compare(typeof data.id, "number",
131+ "NotificationModel should expose a \"id\" role of type int");
132+ compare(typeof data.summary, "string",
133+ "NotificationModel should expose a \"summary\" role of type QString");
134+ compare(typeof data.body, "string",
135+ "NotificationModel should expose a \"body\" role of type QString");
136+ compare(typeof data.value, "number",
137+ "NotificationModel should expose a \"value\" role of type int");
138+ compare(typeof data.icon, "object",
139+ "NotificationModel should expose an \"icon\" role of type QUrl");
140+ compare(typeof data.secondaryIcon, "object",
141+ "NotificationModel should expose a \"secondaryIcon\" role of type QUrl");
142+ compare(typeof data.actions, "object",
143+ "NotificationModel should expose an \"actions\" role of type QQmlListProperty");
144+ compare(typeof data.hints, "number",
145+ "NotificationModel should expose a \"hints\" role of type Hints");
146+ compare(typeof data.notification, "object",
147+ "NotificationModel should expose a \"notification\" role of type Notification");
148+
149+ var notification = data.notification;
150+ compare(typeof notification.dismissed, "function",
151+ "Notification::dismissed should be a signal");
152+ compare(typeof notification.onHovered, "function",
153+ "Notification::onHovered should be a slot");
154+ compare(typeof notification.onDisplayed, "function",
155+ "Notification::onDisplayed should be a slot");
156+ }
157+
158+ /* make sure all the required roles and methods of Action are exposed */
159+ function test_action_members() {
160+ source.send({
161+ "type": Notifications.Type.Interactive,
162+ "actions": [
163+ {"label": "test"}
164+ ]
165+ });
166+ tryCompare(repeater, "count", 1, "there should be one notification");
167+
168+ var data = repeater.itemAt(0).data;
169+ data = repeater.itemAt(0).actionRepeater.itemAt(0).data;
170+ compare(typeof data.label, "string",
171+ "Notification::actions should expose a \"label\" role of type QString");
172+ compare(typeof data.action, "object",
173+ "Notification::actions should expose an \"action\" role of type Action");
174+
175+ var action = data.action;
176+ compare(typeof action.invoke, "function", "Action::invoke should be a function");
177+ }
178+
179+ /* make sure the model is empty by default */
180+ function test_empty() {
181+ tryCompare(repeater, "count", 0, "there should be no notifications");
182+ }
183+
184+ /* make sure there is a placeholder item used as the first item when
185+ confirmationPlaceholder is true and that any additional notification is added
186+ after it */
187+ function test_placeholder() {
188+ Notifications.Model.confirmationPlaceholder = true;
189+ tryCompare(repeater, "count", 1, "there should be one notification");
190+ compare(repeater.itemAt(0).data.type, Notifications.Type.Placeholder,
191+ "the notification should be of Placeholder type");
192+ source.send({
193+ "type": Notifications.Type.Ephemeral
194+ })
195+ tryCompare(repeater, "count", 2, "second notification was added");
196+ compare(repeater.itemAt(0).data.type, Notifications.Type.Placeholder,
197+ "the first notification should be of Placeholder type");
198+ compare(repeater.itemAt(1).data.type, Notifications.Type.Ephemeral,
199+ "the second notification should be of Ephemeral type");
200+ }
201+
202+ /* make sure the placeholder item is updated with data incoming in a Confirmation
203+ notification */
204+ function test_confirmation() {
205+ Notifications.Model.confirmationPlaceholder = true;
206+ tryCompare(repeater, "count", 1, "there should be only one notification");
207+ source.send({
208+ "type": Notifications.Type.Confirmation
209+ });
210+ tryCompare(dataSpy, "count", 1, "notification data should have been updated");
211+ compare(repeater.count, 1, "there should be only one notification");
212+ compare(repeater.itemAt(0).type, Notifications.Type.Confirmation,
213+ "the first notification should be of Confirmation type");
214+ }
215+}

Subscribers

People subscribed via source and target branches