Merge lp:~paulliu/unity/phablet_add-qmluitest1 into lp:unity/phablet

Proposed by Ying-Chun Liu
Status: Merged
Approved by: Michael Zanetti
Approved revision: no longer in the source branch.
Merged at revision: 638
Proposed branch: lp:~paulliu/unity/phablet_add-qmluitest1
Merge into: lp:unity/phablet
Diff against target: 105 lines (+91/-0)
2 files modified
tests/qmltests/CMakeLists.txt (+1/-0)
tests/qmltests/Panel/Menus/tst_IndicatorMenuWindow.qml (+90/-0)
To merge this branch: bzr merge lp:~paulliu/unity/phablet_add-qmluitest1
Reviewer Review Type Date Requested Status
Daniel d'Andrada (community) Needs Information
Michael Zanetti (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+158362@code.launchpad.net

Commit message

Add test for Panel/Menus/IndicatorMenuWindow.

Description of the change

Add test for Panel/Menus/IndicatorMenuWindow.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

60 + compare(indicatorMenuWindow.headingText, indicatorMenuWindow.name, "IndicatorMenuWindow name != headingText")
61 + compare(indicatorMenuWindow.contentHeight, testIndicatorMenuWindowItem.height, "IndicatorMenuWindow contentHeight != height")

Please avoid such long lines. Avoid having anything longer than 90 chars. Long lines makes having split windows (side-by-side code windows) a pain.

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Looking at IndicatorMenuWindow.qml, I don't think it makes sense to test it at all. There's virtually no logic in there. Nothing worth testing.

This test is also at a 1:1 mapping of the code being tested. It doesn't make much sense.

e.g.:

code under test:
   opacity: shown ? 1.0 : 0.0
test code:
    compare(indicatorMenuWindow.opacity, 0.0, "Init opacity is not zero")
    indicatorMenuWindow.shown=true
    tryCompare(indicatorMenuWindow, "opacity", 1.0)
    indicatorMenuWindow.shown=false
    tryCompare(indicatorMenuWindow, "opacity", 0.0)

I fail to see the value of such tests.

review: Disapprove
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

> Looking at IndicatorMenuWindow.qml, I don't think it makes sense to test it at
> all. There's virtually no logic in there. Nothing worth testing.
>
> This test is also at a 1:1 mapping of the code being tested. It doesn't make
> much sense.
>
> e.g.:
>
> code under test:
> opacity: shown ? 1.0 : 0.0
> test code:
> compare(indicatorMenuWindow.opacity, 0.0, "Init opacity is not zero")
> indicatorMenuWindow.shown=true
> tryCompare(indicatorMenuWindow, "opacity", 1.0)
> indicatorMenuWindow.shown=false
> tryCompare(indicatorMenuWindow, "opacity", 0.0)
>
> I fail to see the value of such tests.

I mean, this is like testing if a QML expression is doing what it should do. It's like testing QML itself.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Zanetti (mzanetti) wrote :

I agree with Daniel here. I think what could be tested is that the Window in fact does not let any mouse clicks pass through (which is the actual purpose of it).

In a nutshell:
- Open the Window, make it invisible
- Click somewhere in the window, make sure the click passes through
- Make it visible
- Click again in the window, make sure the click does NOT pass through it

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ying-Chun Liu (paulliu) wrote :

Need review again.

Revision history for this message
Andrea Cimitan (cimi) wrote :

Indentation needs fixes and extra lines to be removed

Revision history for this message
Michael Zanetti (mzanetti) wrote :

12 === added file 'tests/qmltests/Panel/Menus/CMakeLists.txt'
16 +add_qml_test(IndicatorMenuWindow)

This CMakeLists.txt file is not needed any more. Delete it.

review: Needs Fixing
Revision history for this message
Michael Zanetti (mzanetti) wrote :

94 + function test_mouseEvent() {
95 + indicatorMenuWindow.shown=true
96 + tryCompare(indicatorMenuWindow, "opacity", 1.0)
97 +
98 + clickedSpy.clear()
99 + mouseClick(indicatorMenuWindow, indicatorMenuWindow.width / 2,
100 + indicatorMenuWindow.height / 2)
101 + compare(clickedSpy.count, 0,
102 + "Mouse event should be eaten by indicatorMenuWindow")
103 +
104 + indicatorMenuWindow.shown=false
105 + tryCompare(indicatorMenuWindow, "opacity", 0.0)
106 +
107 + clickedSpy.clear()
108 + mouseClick(indicatorMenuWindow, indicatorMenuWindow.width / 2,
109 + indicatorMenuWindow.height / 2)
110 + compare(clickedSpy.count, 1,
111 + "Mouse event should not be eaten by indicatorMenuWindow")
112 + }

I think this would be a perfect candidate for using the _data() mechanism.

Example:

        function test_mouseEvent_data() {
            return [
                {tag: "invisible", shown: true, opacity: 1.0, mouseClicks: 0},
                {tag: "visible", shown: false, opacity: 0.0, mouseClicks: 1},
            ]
        }

        function test_mouseEvent(data) {
            indicatorMenuWindow.shown = data.shown
            tryCompare(indicatorMenuWindow, "opacity", data.opacity)

            clickedSpy.clear()
            mouseClick(indicatorMenuWindow, indicatorMenuWindow.width / 2,
                       indicatorMenuWindow.height / 2)
            compare(clickedSpy.count, data.mouseClicks,
                    "Check for Mouse event eating by indicatorMenuWindow failed")
        }

This way you don't have to copy/paste the test code and the debug output gives better feedback when a test fails.

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michael Zanetti (mzanetti) wrote :

thanks.

review: Approve
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

I would use more meaningful names for those rectangles, like backgroundRect and foregroundRect, instead of simply rectangle1 and rectangle2.

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

> I would use more meaningful names for those rectangles, like backgroundRect
> and foregroundRect, instead of simply rectangle1 and rectangle2.

Or rootRectangle for rectangle1

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

89 + {tag: "invisible", shown: true, opacity: 1.0, mouseClicks: 0},
90 + {tag: "visible", shown: false, opacity: 0.0, mouseClicks: 1},

Looks like you've swapped the "visible" and "invisible" tags...

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

By the way, what's the purpose of this red rectangle (id: rectangle2)? It doesn't seem to be used in the tests.
Looks like you can just remove it.

review: Needs Information
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Also rectangle1 could be the root item itself. Or maybe even that MouseArea.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'tests/qmltests/CMakeLists.txt'
--- tests/qmltests/CMakeLists.txt 2013-04-26 10:02:29 +0000
+++ tests/qmltests/CMakeLists.txt 2013-04-29 16:27:26 +0000
@@ -56,4 +56,5 @@
56add_qml_test(Panel Overview)56add_qml_test(Panel Overview)
57add_qml_test(Panel Panel)57add_qml_test(Panel Panel)
58add_qml_test(Panel SearchIndicator)58add_qml_test(Panel SearchIndicator)
59add_qml_test(Panel/Menus IndicatorMenuWindow IMPORT_PATHS ${qmltest_DEFAULT_IMPORT_PATHS})
59add_qml_test(SideStage SideStage IMPORT_PATHS ${qmltest_DEFAULT_IMPORT_PATHS} ${CMAKE_BINARY_DIR}/tests/mocks)60add_qml_test(SideStage SideStage IMPORT_PATHS ${qmltest_DEFAULT_IMPORT_PATHS} ${CMAKE_BINARY_DIR}/tests/mocks)
6061
=== added directory 'tests/qmltests/Panel/Menus'
=== added file 'tests/qmltests/Panel/Menus/tst_IndicatorMenuWindow.qml'
--- tests/qmltests/Panel/Menus/tst_IndicatorMenuWindow.qml 1970-01-01 00:00:00 +0000
+++ tests/qmltests/Panel/Menus/tst_IndicatorMenuWindow.qml 2013-04-29 16:27:26 +0000
@@ -0,0 +1,90 @@
1/*
2 * Copyright 2013 Canonical Ltd.
3 *
4 * This program is free software; you can redistribute it and/or modify
5 * it under the terms of the GNU General Public License as published by
6 * the Free Software Foundation; version 3.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 */
16
17import QtQuick 2.0
18import QtTest 1.0
19import "../.."
20import "../../../../Panel/Menus"
21import Ubuntu.Components 0.1
22import Unity.Test 0.1 as UT
23
24Item {
25 id: testIndicatorMenuWindowItem
26 width: units.gu(9)
27 height: units.gu(3)
28
29 Rectangle {
30 id: rectangle1
31 anchors.fill: parent
32 signal clicked
33
34 MouseArea {
35 anchors.fill: parent
36 onClicked: rectangle1.clicked()
37 }
38
39 Rectangle {
40 anchors.right: rectangle1.right
41 anchors.left: rectangle1.horizontalCenter
42 anchors.top: rectangle1.verticalCenter
43 anchors.bottom: rectangle1.bottom
44 color: "green"
45 IndicatorMenuWindow {
46 id: indicatorMenuWindow
47 name: "TestIndicatorMenuWindow"
48 anchors.fill: parent
49 }
50 }
51
52 Rectangle {
53 id: rectangle2
54 color: "red"
55 anchors.right: rectangle1.horizontalCenter
56 anchors.left: rectangle1.left
57 anchors.top: rectangle1.verticalCenter
58 anchors.bottom: rectangle1.bottom
59 }
60
61 SignalSpy {
62 id: clickedSpy
63 target: rectangle1
64 signalName: "clicked"
65 }
66 }
67
68 UT.UnityTestCase {
69 name: "IndicatorMenuWindow"
70 when: windowShown
71
72 function test_mouseEvent_data() {
73 return [
74 {tag: "invisible", shown: true, opacity: 1.0, mouseClicks: 0},
75 {tag: "visible", shown: false, opacity: 0.0, mouseClicks: 1},
76 ]
77 }
78
79 function test_mouseEvent(data) {
80 indicatorMenuWindow.shown = data.shown
81 tryCompare(indicatorMenuWindow, "opacity", data.opacity)
82
83 clickedSpy.clear()
84 mouseClick(indicatorMenuWindow, indicatorMenuWindow.width / 2,
85 indicatorMenuWindow.height / 2)
86 compare(clickedSpy.count, data.mouseClicks,
87 "Check for Mouse event eating by indicatorMenuWindow failed")
88 }
89 }
90}

Subscribers

People subscribed via source and target branches