Merge lp:~nik90/ubuntu-clock-app/add-world-qml-tests into lp:ubuntu-clock-app

[Nekhelesh Ramananthan]

Added world clock feature qml tests.

Added world clock feature qml tests.

The QML Test Suite now comprises of Feature, Unit and Integration tests and collectively has a far higher test coverage of the clock app than AP test have. A few more MPs later, I might propose removing AP tests.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:159
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/560/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic-python3/924
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic-python3/924/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/560/rebuild

review: Approve (continuous-integration)

[Leo Arias (elopio)]

You have a couple of ifs there that are really hard to understand.
You can refactor them like this: http://paste.ubuntu.com/8556979/
(I haven't run the tests with the refactor, so please double check the code, I could make typos).

Also, please remember that on python, we should name the variables with all lower case separating words with spaces. And that we should prefer single quotes instead of double quotes. Looking at the emulators file, I see some style errors. They can be fixed in a separate branch, unless you are planning to kill that file soon.

126 +/*
127 + This file is meant to create a fake but fully fleshed clock app with its
128 + own database and settings. This will avoid messing with the user data while
129 + running the tests.
130 +*/

Interesting, I like it. You just need to take a lot of care that this test double is as close to the real as possible. Ideally without duplicating any code, because otherwise they will be hard to maintain. Usually, devs add things to the real one and forget to update the double, so they end up testing a different thing.

Could you make a QML file that takes the app name and any other things you want to change on the mock as parameters, and then make both app/ubuntu-clock-app.qml and tests/unit/MockClockApp.qml extend that file?

250 + function _findWorldCity(cityList, type, cityName, countryName) {

It's slightly clearer when you make two methods instead of passing a type argument:
findUserWorldCity and findDefaultWorldCity.
Those are the methods you call from a test. Then, you make them call an internal _findWorldCity where you pass the right prefix. But you are encapsulating the prefix in the internal methods, your tests don't know about the exact prefix. So by changing the tests to call a method instead of passing a string, it will be easier for you if you ever have to change the prefix. Instead of changing it in all the tests that call findWorldCity, you will just have to change it on the non-internal methods and the name can remain the same.

271 + function _confirmWorldCityAddition(cityName, countryName) {

Here again, I prefer assert. confirm sounds like a step of the test, like confirm a dialog or something like that.

295 + tryCompareFunction(function() { return cityList.count > 0}, true)

And here again, this shouldn't be needed if you are already asserting the successful creation during a test.

327 + // Wait for the list to be populated with results
328 + tryCompareFunction(function() { return cityList.count > 0}, true)

Now this is a valid usage of an assertion on the middle of the steps. But it's just because this framework doesn't have a proper wait. On autopilot we would do it with a wait_for or something similar, that's a little different than an assertion.

373 + function test_addCityBySearchingOnline() {

You have to be careful here. The unit tests many times are run in a confined machine without internet access. As the tests are passing, I'm guessing that's not the case for the jenkins runners. But ideally, you would replace the online model with a hard-coded one for a unit test. And then do an integration test for the service that provides you with cities.

This is great, ...

[lp:~nik90/ubuntu-clock-app/add-world-qml-tests]

160. By Nekhelesh Ramananthan on 2014-10-14

merged trunk

161. By Nekhelesh Ramananthan on 2014-10-14

Migrated tst_worldClock over to ClockTestCase

162. By Nekhelesh Ramananthan on 2014-10-14

renamed confirmWorldCityAddition() to assertWorldCityAddition()

163. By Nekhelesh Ramananthan on 2014-10-14

Removed unnecessary confirm statement iat the start of a test

164. By Nekhelesh Ramananthan on 2014-10-14

Added TODO comment for mocking the searchOnline function

165. By Nekhelesh Ramananthan on 2014-10-14

Improved if logic in emulators.py

166. By Nekhelesh Ramananthan on 2014-10-14

Simplified code a bit

[Nekhelesh Ramananthan (nik90)]

> You have a couple of ifs there that are really hard to understand.
> You can refactor them like this: http://paste.ubuntu.com/8556979/
> (I haven't run the tests with the refactor, so please double check the code, I
> could make typos).
>
> Also, please remember that on python, we should name the variables with all
> lower case separating words with spaces. And that we should prefer single
> quotes instead of double quotes. Looking at the emulators file, I see some
> style errors. They can be fixed in a separate branch, unless you are planning
> to kill that file soon.

Thnx I fixed that as well. I was only planning to make minimal changes to emulators.py in fear of breaking the AP test ;). But now the if logic looks much simpler despite taking me longer than usual to fix that (hence my uneasiness in touch python code)

> 250 + function _findWorldCity(cityList, type, cityName, countryName) {
>
> It's slightly clearer when you make two methods instead of passing a type
> argument:
> findUserWorldCity and findDefaultWorldCity.
> Those are the methods you call from a test. Then, you make them call an
> internal _findWorldCity where you pass the right prefix. But you are
> encapsulating the prefix in the internal methods, your tests don't know about
> the exact prefix. So by changing the tests to call a method instead of passing
> a string, it will be easier for you if you ever have to change the prefix.
> Instead of changing it in all the tests that call findWorldCity, you will just
> have to change it on the non-internal methods and the name can remain the
> same.
>

Done. I hope what I did is what you had in mind as well.

> 271 + function _confirmWorldCityAddition(cityName, countryName) {
>
> Here again, I prefer assert. confirm sounds like a step of the test, like
> confirm a dialog or something like that.

Trivial fix. Done. I will use assert from now on.

>
> 295 + tryCompareFunction(function() { return cityList.count > 0}, true)
>
> And here again, this shouldn't be needed if you are already asserting the
> successful creation during a test.
>

Done

> 327 + // Wait for the list to be populated with results
> 328 + tryCompareFunction(function() { return cityList.count > 0}, true)
>
> Now this is a valid usage of an assertion on the middle of the steps. But it's
> just because this framework doesn't have a proper wait. On autopilot we would
> do it with a wait_for or something similar, that's a little different than an
> assertion.
>

Done

> 373 + function test_addCityBySearchingOnline() {
>
> You have to be careful here. The unit tests many times are run in a confined
> machine without internet access. As the tests are passing, I'm guessing that's
> not the case for the jenkins runners. But ideally, you would replace the
> online model with a hard-coded one for a unit test. And then do an integration
> test for the service that provides you with cities.
>

This is an easy one. I have added a TODO comment about mocking that. I am not sure *yet* how to do this without touching the clock app code which at the moment (for RTM) I don't want to do.

> This is great, as always. Ping me on IRC if you want to ...

[Nekhelesh Ramananthan (nik90)]

>
> 126 +/*
> 127 + This file is meant to create a fake but fully fleshed clock app with
> its
> 128 + own database and settings. This will avoid messing with the user
> data while
> 129 + running the tests.
> 130 +*/
>
> Interesting, I like it.

This is my first attempt at mocking the clock app. And I was surprised at how easy it was to do this, something which wouldn't be possible with AP without touching the actual app config files.

> You just need to take a lot of care that this test
> double is as close to the real as possible. Ideally without duplicating any
> code, because otherwise they will be hard to maintain. Usually, devs add
> things to the real one and forget to update the double, so they end up testing
> a different thing.

Yup. I agree this can be a potential issue. However this late in the cycle (close to RTM) there wouldn't be any more changes to ubuntu-clock-app.qml. And even if it does, it will be really minor and I will make sure to sync it up to MockClockApp.qml.

>
> Could you make a QML file that takes the app name and any other things you
> want to change on the mock as parameters, and then make both app/ubuntu-clock-
> app.qml and tests/unit/MockClockApp.qml extend that file?
>

At this point, I don't want to avoid touching the ubuntu-clock-app.qml if I can avoid it. I hope that is okay. But I think I have an idea of how you want to proceed with this. I will have another MP which plays with that idea.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:166
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/562/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic-python3/931
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic-python3/931/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/562/rebuild

review: Approve (continuous-integration)

[lp:~nik90/ubuntu-clock-app/add-world-qml-tests]

167. By Nekhelesh Ramananthan on 2014-10-14

Merged prerequisite branch

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:167
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/563/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic-python3/933
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic-python3/933/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/563/rebuild

review: Approve (continuous-integration)

[Leo Arias (elopio)]

review: Approve

[lp:~nik90/ubuntu-clock-app/add-world-qml-tests]

168. By Nekhelesh Ramananthan on 2014-10-14

Merged prerequisite branch

=== modified file 'app/worldclock/WorldCityList.qml'
--- app/worldclock/WorldCityList.qml	2014-10-11 18:39:12 +0000
+++ app/worldclock/WorldCityList.qml	2014-10-14 16:02:15 +0000
@@ -302,7 +302,7 @@
 
         delegate: ListItem.Base {
             showDivider: false
-            objectName: "worldCityItem" + index
+            objectName: "defaultWorldCityItem" + index
 
             Column {
                 id: worldCityDelegateColumn
@@ -316,7 +316,7 @@
 
                 Label {
                     text: city
-                    objectName: "cityNameText"
+                    objectName: "defaultCityNameText"
                     width: parent.width
                     elide: Text.ElideRight
                     color: UbuntuColors.midAubergine
@@ -324,7 +324,7 @@
 
                 Label {
                     text: country
-                    objectName: "countryNameText"
+                    objectName: "defaultCountryNameText"
                     fontSize: "xx-small"
                     width: parent.width
                     elide: Text.ElideRight
 
=== modified file 'debian/changelog'
--- debian/changelog	2014-10-14 16:02:15 +0000
+++ debian/changelog	2014-10-14 16:02:15 +0000
@@ -14,6 +14,7 @@
   * Fixed alarm page header width warning and corrected icon spacing.
   * Added edit alarm qml tests and added a library helper which contains commonly
     used test functions to avoid code duplication in future tests.
+  * Added world clock feature qml tests
   * Delayed loading of xmltimezone model until add world city page is loaded and
     dynamically load/unload the jsontimezone model only when necessary.
 
 
=== modified file 'tests/autopilot/ubuntu_clock_app/emulators.py'
--- tests/autopilot/ubuntu_clock_app/emulators.py	2014-10-10 22:12:50 +0000
+++ tests/autopilot/ubuntu_clock_app/emulators.py	2014-10-14 16:02:15 +0000
@@ -133,16 +133,15 @@
         old_cities_count = self.get_num_of_saved_cities()
         index = 0
         for index in range(old_cities_count):
-            if self.wait_select_single(
-                objectName='userWorldCityItem{}'.format(index)).\
-                wait_select_single("Label", objectName="userCityNameText").\
-                    text == city_Name:
-                if self.wait_select_single(
-                    objectName='userWorldCityItem{}'.format(index)).\
-                    wait_select_single(
-                    "Label", objectName="userCountryNameText").\
-                        text == country_Name:
-                    self._delete_userWorldCityItem(index)
+            world_city_item = self.wait_select_single(
+                objectName='userWorldCityItem{}'.format(index))
+            city_name_label = world_city_item.wait_select_single(
+                'Label', objectName='userCityNameText')
+            country_name_label = world_city_item.wait_select_single(
+                'Label', objectName='userCountryNameText')
+            if (city_name_label.text == city_Name and
+                    country_name_label.text == country_Name):
+                self._delete_userWorldCityItem(index)
 
     # FIXME -----------------------------------------------------------------
     # Commenting the following lines as deleting a world city when there is
@@ -266,16 +265,17 @@
         cityList.count.wait_for(GreaterThan(0))
 
         for index in range(int(cityList.count)):
-            if cityList.wait_select_single(
-                objectName="worldCityItem{}".format(index)).wait_select_single(
-                    "Label", objectName="cityNameText").text == city_Name:
-                if cityList.wait_select_single(
-                    objectName="worldCityItem{}".format(index)).\
-                    wait_select_single("Label", objectName="countryNameText").\
-                        text == country_Name:
-                    cityList.click_element(
-                        "worldCityItem{}".format(index), direction=None)
-                    break
+            world_city_item = self.wait_select_single(
+                objectName='defaultWorldCityItem{}'.format(index))
+            city_name_label = world_city_item.wait_select_single(
+                'Label', objectName='defaultCityNameText')
+            country_name_label = world_city_item.wait_select_single(
+                'Label', objectName='defaultCountryNameText')
+            if (city_name_label.text == city_Name and
+                    country_name_label.text == country_Name):
+                cityList.click_element(
+                    'defaultWorldCityItem{}'.format(index), direction=None)
+                break
 
     @autopilot_logging.log_action(logger.info)
     def search_world_city_(self, city_Name, country_Name):
 
=== modified file 'tests/unit/CMakeLists.txt'
--- tests/unit/CMakeLists.txt	2014-10-14 16:02:15 +0000
+++ tests/unit/CMakeLists.txt	2014-10-14 16:02:15 +0000
@@ -26,6 +26,7 @@
     declare_qml_test("Alarm" tst_alarm.qml)
     declare_qml_test("AlarmSound" tst_alarmSound.qml)
     declare_qml_test("AlarmUtils" tst_alarmUtils.qml)
+    declare_qml_test("WorldClock" tst_worldClock.qml)
 else()
     if (NOT QMLTESTRUNNER_BIN)
         message(WARNING "Qml tests disabled: qmltestrunner not found")
@@ -40,10 +41,12 @@
     tst_alarm.qml
     tst_alarmSound.qml
     tst_alarmUtils.qml
+    tst_worldClock.qml
 )
 add_custom_target(tst_QmlFiles ALL SOURCES ${QML_TST_FILES})
 
 set(QML_TST_UTILS
+    MockClockApp.qml
     ClockTestCase.qml
 )
 
 
=== added file 'tests/unit/MockClockApp.qml'
--- tests/unit/MockClockApp.qml	1970-01-01 00:00:00 +0000
+++ tests/unit/MockClockApp.qml	2014-10-14 16:02:15 +0000
@@ -0,0 +1,88 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd
+ *
+ * This file is part of Ubuntu Clock App
+ *
+ * Ubuntu Clock App 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.
+ *
+ * Ubuntu Clock App 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 this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import QtQuick 2.3
+import DateTime 1.0
+import U1db 1.0 as U1db
+import Ubuntu.Components 1.1
+import "../../app/clock"
+
+/*
+ This file is meant to create a fake but fully fleshed clock app with its
+ own database and settings. This will avoid messing with the user data while
+ running the tests.
+*/
+
+MainView {
+    id: clockApp
+
+    // Property to store the state of an application (active or suspended)
+    property bool applicationState: Qt.application.active
+
+    width: units.gu(40)
+    height: units.gu(70)
+    useDeprecatedToolbar: false
+    applicationName: "com.ubuntu.fakeclock.test"
+
+    U1db.Database {
+        id: clockDB
+        path: "user-preferences"
+    }
+
+    U1db.Document {
+        id: clockModeDocument
+        create: true
+        database: clockDB
+        docId: "clockModeDocument"
+        defaults: { "digitalMode": false }
+    }
+
+    DateTime {
+        id: localTimeSource
+        updateInterval: 1000
+    }
+
+    PageStack {
+        id: mainStack
+        objectName: "pageStack"
+
+        Component.onCompleted: push(clockPage)
+
+        ClockPage {
+            id: clockPage
+
+            Loader {
+                id: alarmModelLoader
+                asynchronous: false
+            }
+
+            alarmModel: alarmModelLoader.item
+            bottomEdgeEnabled: alarmModelLoader.status === Loader.Ready
+            clockTime: new Date
+                       (
+                           localTimeSource.localDateString.split(":")[0],
+                           localTimeSource.localDateString.split(":")[1]-1,
+                           localTimeSource.localDateString.split(":")[2],
+                           localTimeSource.localTimeString.split(":")[0],
+                           localTimeSource.localTimeString.split(":")[1],
+                           localTimeSource.localTimeString.split(":")[2],
+                           localTimeSource.localTimeString.split(":")[3]
+                       )
+        }
+    }
+}
 
=== added file 'tests/unit/tst_worldClock.qml'
--- tests/unit/tst_worldClock.qml	1970-01-01 00:00:00 +0000
+++ tests/unit/tst_worldClock.qml	2014-10-14 16:02:15 +0000
@@ -0,0 +1,194 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd
+ *
+ * This file is part of Ubuntu Clock App
+ *
+ * Ubuntu Clock App 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.
+ *
+ * Ubuntu Clock App 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 this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import QtQuick 2.0
+import QtTest 1.0
+import Ubuntu.Test 1.0
+import Ubuntu.Components 1.1
+
+MockClockApp {
+    id: clockApp
+
+    ClockTestCase {
+        id: worldClockFeatureTest
+        name: "WorldClockFeatureTest"
+
+        when: windowShown
+
+        property var header
+        property var backButton
+        property var clockPage
+        property var pageStack
+
+        function initTestCase() {
+            header = findChild(clockApp, "MainView_Header")
+            backButton = findChild(header, "customBackButton")
+            pageStack = findChild(clockApp, "pageStack")
+            clockPage = findChild(clockApp, "clockPage")
+        }
+
+        // *********** Helper Functions ************
+
+        function pressAddWorldCityButton() {
+            var addWorldCityButton = findChild(clockApp, "addWorldCityButton")
+            pressButton(addWorldCityButton)
+        }
+
+        function _findWorldCity(cityList, type, cityName, countryName) {
+            /*
+             The list view for the user world city list and the available world
+             city list have the same structure with some minor object name
+             changes. The 'objectPrefix' varible is used to handle that.
+            */
+            var objectPrefix = type === "user" ? "user" : "default"
+
+            for(var i=0; i<cityList.count; i++) {
+                var cityListItem = findChild(clockApp, objectPrefix+"WorldCityItem"+i)
+                var city = findChild(cityListItem, objectPrefix+"CityNameText")
+                var country = findChild(cityListItem, objectPrefix+"CountryNameText")
+
+                if (city.text === cityName && country.text === countryName) {
+                    return i
+                }
+            }
+
+            return -1;
+        }
+
+        function findUserWorldCity(cityList, cityName, countryName) {
+            return _findWorldCity(cityList, "user", cityName, countryName)
+        }
+
+        function findDefaultWorldCity(cityList, cityName, countryName) {
+            return _findWorldCity(cityList, "default", cityName, countryName)
+        }
+
+        function assertWorldCityAddition(cityName, countryName) {
+            var cityList = findChild(clockApp, "userWorldCityRepeater")
+
+            // Wait for the user list to be populated with results
+            tryCompareFunction(function() { return cityList.count > 0}, true)
+
+            var cityIndex = findUserWorldCity(cityList, cityName, countryName)
+
+            if (cityIndex === -1) {
+                // If city couldn't be found in the saved city list, fail the test
+                fail("City added during the test cannot be found in the user world city list!")
+            }
+        }
+
+        function deleteWorldCity(cityName, countryName) {
+            var cityList = findChild(clockApp, "userWorldCityRepeater")
+
+            var oldCount = cityList.count
+            var cityIndex = findUserWorldCity(cityList, cityName, countryName)
+
+            if (cityIndex === -1) {
+                fail("City added during the test cannot be found in the user world city list!")
+            }
+            else {
+                var cityListItem = findChild(clockApp, "userWorldCityItem"+cityIndex)
+                swipeToDeleteItem(cityListItem)
+            }
+
+            /*
+             #FIXME: Commented out the following line as deleting a world city
+             when there is only one world city does not decrease the count to 0
+             but leaves it as 1 causing the test to fail. This has been reported
+             in bug #1368393. (Also fails in Autopilot)
+
+             tryCompare(cityList, "count", oldCount-1, 5000, "city list count did not decrease")
+
+             The wait() call below is to ensure that the world city is deleted properly
+             which wouldn't be required if could do the count decrease check mentioned above.
+            */
+
+            wait(1000)
+        }
+
+        function addCityFromList(cityName, countryName) {
+            var worldCityPage = getPage(pageStack, "worldCityList")
+            var cityList = findChild(worldCityPage, "cityList")
+
+            // Wait for the list to be populated with results
+            tryCompareFunction(function() { return cityList.count > 0}, true)
+
+            var cityIndex = findDefaultWorldCity(cityList, cityName, countryName)
+
+            if (cityIndex === -1) {
+                fail("City cannot be found in the local world city list")
+            }
+
+            var cityListItem = findChild(cityList, "defaultWorldCityItem"+cityIndex)
+            mouseClick(cityListItem, centerOf(cityListItem).x, centerOf(cityListItem).y)
+        }
+
+        function addCityBySearchingOnline(cityName, countryName) {
+            pressHeaderButton(header, "searchButton")
+            var searchField = findChild(clockApp, "searchField")
+            tryCompare(searchField, "visible", true, 5000, "Search field is not visible")
+            typeString(cityName)
+            addCityFromList(cityName, countryName)
+        }
+
+        // *********** Test Functions *************
+
+        /*
+         Test to check if a city found in the world city list can be added
+         to the user world city list.
+        */
+        function test_addCityAlreadyPresentInWorldCityList() {
+            var clockPage = getPage(pageStack, "clockPage")
+
+            pressAddWorldCityButton()
+
+            var worldCityPage = getPage(pageStack, "worldCityList")
+            waitForRendering(worldCityPage)
+
+            addCityFromList("Amsterdam", "Netherlands")
+            assertWorldCityAddition("Amsterdam", "Netherlands")
+
+            // Clean up after the test by deleting the city which was added during the test
+            deleteWorldCity("Amsterdam", "Netherlands")
+        }
+
+        /*
+         Test to check if a city now found in the world city list can be added
+         by searcing it online and then adding it from the results returned.
+        */
+        function test_addCityBySearchingOnline() {
+            var clockPage = getPage(pageStack, "clockPage")
+
+            pressAddWorldCityButton()
+
+            var worldCityPage = getPage(pageStack, "worldCityList")
+            waitForRendering(worldCityPage)
+
+            /*
+             #TODO: Jenkins machine may run in a confined environment with no
+             access to internet to run this function. Ideally we should mock
+             the data given to this function.
+            */
+            addCityBySearchingOnline("Venice", "Provincia di Venezia, Veneto, Italy")
+            assertWorldCityAddition("Venice", " Veneto, Italy")
+
+            // Clean up after the test by deleting the city which was added during the test
+            deleteWorldCity("Venice", " Veneto, Italy")
+        }
+    }
+}