Merge lp:~nik90/ubuntu-clock-app/reorganise-stopwatch-tests into lp:ubuntu-clock-app/saucy

[Nekhelesh Ramananthan]

Refactor stopwatch helpers to follow the page object pattern.

Refactor stopwatch helpers to follow the page object pattern. Tested on both phone and tablet interface.

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

FAILED: Continuous integration, rev:355
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/293/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1518
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-raring-amd64-ci/293
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-saucy-amd64-ci/293
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-trusty-amd64-ci/211

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

review: Needs Fixing (continuous-integration)

[Nekhelesh Ramananthan (nik90)]

The jenkins failure above is not a clock AP issue.

[Leo Arias (elopio)]

75 + def get_stopwatch_time(self):
76 + """Returns the stopwatch time label"""
77 + return self.wait_select_single('Label', objectName='label')

This is actually an implementation detail, the fact that the stop watch time is kept in a label called label. The service your UI is providing to the users is to give them the time of the stopwatch, so that's what we should return in the public helpers, instead of a label element.

So, this would be better:

def get_stopwatch_time(self):
    return self.wait_select_single('Label', objectName='label').text

That way, if you or a designer decide to show the time in a component called 'Timer' that has a 'time' property instead of a 'Label' component with a 'text' property, you will only have to change one line and all your tests will keep working.

Now, on:

83 + @autopilot_logging.log_action(logger.info)
84 + def get_stopwatch_lap(self, index):

What you want is the element so you can later delete it, right?
So, this method should be kept private, to be sure that it's not in use by any tests in case you need to refactor it.
I'd say _get_stopwatch_lap_list_element is a better name.

And about the log_action decorator, what's nice is to put it only on the actions a user does. Like go_to_some_page, or save_something, or delete_something. We don't really need to log any method that starts with get_something, because they are only used as lower level helpers to perform an action, or as things that you would assert on a test. I'm not sure if I'm being clear, if not, let me know on the chat. Also, feel free to disagree, this is not an exact science and we are just inventing things here :)

93 + def start_stopwatch(self):
107 + def reset_stopwatch(self):
117 + def create_lap(self):

These need a log_action ^

A small detail is that the docstring of methods "prescribes the function or method's effect as a command", so instead of a verb like Returns, it should be Return. And instead of "Function to stop..." it should be "Stop..."
This comes from pep257. http://legacy.python.org/dev/peps/pep-0257/

review: Needs Fixing

[lp:~nik90/ubuntu-clock-app/reorganise-stopwatch-tests]

356. By Nekhelesh Ramananthan on 2014-02-26

Returned proper time instead of label

357. By Nekhelesh Ramananthan on 2014-02-26

Added autopilot loggers to the public functions and renamed get_stopwatch_lap function

358. By Nekhelesh Ramananthan on 2014-02-26

Fixed comments according to pep257 standards

[Leo Arias (elopio)]

227 + def test_start_button_must_start_stopwatch(self):
234 + def test_stop_button_must_stop_stopwatch(self):

By looking just at the UI, there isn't really a start and a stop button. Well, they are there, but that's an implementation detail that we know because we have read the code.

From a user story point of view, I think this would be clearer named as something like:

test_click_clock_center_must_start_stopwatch(self)
test_click_clock_center_with_stop_watch_started_must_stop_it(self)

If that last name makes the line longer than 80 characters, then it could be just:
test_click_clock_center_must_stop_stopwatch(self):

240 + self.assertThat(
241 + stopwatch_time,
242 + NotEquals(self.page.get_stopwatch_time().text))

I think that one is not necessary because in essence it would be the same check we are doing on the previous tests, I think.
What if we do something like this:

stopwatch_time_after = lambda: self.page.get_stopwatch_time()
time.sleep(1)
self.assertThat(
   stopwatch_time_after,
   Equals(self.page.get_stopwatch_time())))

The eventually is not needed in there, btw.

[Nekhelesh Ramananthan (nik90)]

>
> So, this would be better:
>
> def get_stopwatch_time(self):
> return self.wait_select_single('Label', objectName='label').text
>
> That way, if you or a designer decide to show the time in a component called
> 'Timer' that has a 'time' property instead of a 'Label' component with a
> 'text' property, you will only have to change one line and all your tests will
> keep working.
>

I actually had in mind, but forgot it at the last second :)

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

FAILED: Continuous integration, rev:358
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/296/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1522
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-raring-amd64-ci/296
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-saucy-amd64-ci/296
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-trusty-amd64-ci/214

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

review: Needs Fixing (continuous-integration)

[lp:~nik90/ubuntu-clock-app/reorganise-stopwatch-tests]

359. By Nekhelesh Ramananthan on 2014-02-26

Clarified test names and fixed failing test

360. By Nekhelesh Ramananthan on 2014-02-27

Fixed stop stopwatch test

[Leo Arias (elopio)]

I like this so much, I'll print it ;)

review: Approve (code review)

[Nekhelesh Ramananthan (nik90)]

;)

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

PASSED: Continuous integration, rev:360
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/297/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1523
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-raring-amd64-ci/297
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-saucy-amd64-ci/297
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-trusty-amd64-ci/215

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

review: Approve (continuous-integration)

=== modified file 'stopwatch/LapList.qml'
--- stopwatch/LapList.qml	2014-02-18 20:09:55 +0000
+++ stopwatch/LapList.qml	2014-02-27 00:04:13 +0000
@@ -52,6 +52,7 @@
 
     ListView {
         id: listViewLap
+        objectName: "listStopwatchLaps"
 
         clip: true
         anchors { left: parent.left; right: parent.right; }

=== modified file 'tests/autopilot/ubuntu_clock_app/emulators.py'
--- tests/autopilot/ubuntu_clock_app/emulators.py	2014-02-26 21:06:47 +0000
+++ tests/autopilot/ubuntu_clock_app/emulators.py	2014-02-27 00:04:13 +0000
@@ -55,6 +55,16 @@
         self.switch_to_tab('AlarmTab')
         return self.wait_select_single(AlarmPage)
 
+    @autopilot_logging.log_action(logger.info)
+    def open_stopwatch(self):
+        """Open the Stopwatch Tab.
+
+        :return the Stopwatch page.
+
+        """
+        self.switch_to_tab('StopwatchTab')
+        return self.wait_select_single(StopwatchPage)
+
     # Common Emulator Functions
     def _drag_page(self, page, direction):
         """Function to drag the page up/down"""
@@ -141,34 +151,6 @@
         return self.wait_select_single(
             "Standard", objectName="savedworldcity0")
 
-    # STOPWATCH Page Emulator Functions
-    def get_stopwatch_label(self):
-        """Returns the select for the stopwatch label"""
-        return self.select_single(
-            "StopwatchPage").wait_select_single("Label", objectName="label")
-
-    def get_stopwatch_button(self):
-        """Returns the select for the stopwatch button"""
-        return self.wait_select_single(
-            "StopwatchFace", objectName="stopwatchFace")
-
-    def get_lap_button(self):
-        """Returns the select for the lap button"""
-        return self.wait_select_single("ImageButton", objectName="lapButton")
-
-    def get_reset_button(self):
-        """Returns the select for the reset button"""
-        return self.wait_select_single("ImageButton", objectName="resetButton")
-
-    def get_num_of_laps(self):
-        """Returns the number of laps in the stopwatch page."""
-        stopwatch_page = self.select_single("StopwatchPage")
-        return int(stopwatch_page.wait_select_single("QQuickListView").count)
-
-    def get_first_laps_list_item(self):
-        """Returns the first lap list item in the stopwatch page."""
-        return self.wait_select_single("Standard", objectName="laps0")
-
 
 class LabelDots(toolkit_emulators.TextField):
     """Autopilot helper for the LabelDots component."""
@@ -214,6 +196,78 @@
             objectName='animationContainer').moving.wait_for(False)
 
 
+class StopwatchPage(Page):
+    """Autopilot helper for the Stopwatch page."""
+
+    def get_stopwatch_time(self):
+        """Return the stopwatch time"""
+        return self.wait_select_single('Label', objectName='label').text
+
+    def get_num_of_laps(self):
+        """Return the number of laps in the stopwatch page."""
+        return int(self.wait_select_single('QQuickListView').count)
+
+    def _get_stopwatch_lap_list_element(self, index):
+        """Return a lap list item in the stopwatch page.
+
+        :param index: stopwatch lap number
+
+        """
+        return self.wait_select_single(
+            'Standard', objectName='laps{}'.format(index))
+
+    @autopilot_logging.log_action(logger.info)
+    def start_stopwatch(self):
+        """Start the stopwatch"""
+        self._click_start_stop_stopwatch_button()
+
+    @autopilot_logging.log_action(logger.info)
+    def stop_stopwatch(self):
+        """Stop the stopwatch"""
+        self._click_start_stop_stopwatch_button()
+
+    def _click_start_stop_stopwatch_button(self):
+        """Click the start/stop stopwatch button"""
+        start_stop_button = self.wait_select_single(
+            'StopwatchFace', objectName='stopwatchFace')
+        self.pointing_device.click_object(start_stop_button)
+
+    @autopilot_logging.log_action(logger.info)
+    def reset_stopwatch(self):
+        """Reset the stopwatch"""
+        self._click_reset_stopwatch_button()
+
+    def _click_reset_stopwatch_button(self):
+        """Click the reset stopwatch button"""
+        reset_button = self.wait_select_single(
+            'ImageButton', objectName='resetButton')
+        self.pointing_device.click_object(reset_button)
+
+    @autopilot_logging.log_action(logger.info)
+    def create_lap(self):
+        """Create a stopwatch lap"""
+        self._click_lap_stopwatch_button()
+
+    def _click_lap_stopwatch_button(self):
+        """Click the stopwatch lap button"""
+        lap_button = self.wait_select_single(
+            'ImageButton', objectName='lapButton')
+        self.pointing_device.click_object(lap_button)
+
+    @autopilot_logging.log_action(logger.info)
+    def delete_lap(self, index):
+        """Delete stopwatch lap
+
+        :param index: stopwatch lap number
+
+        """
+        lap = self._get_stopwatch_lap_list_element(index=index)
+        if not self.main_view.wideAspect:
+            self.drag_page_up()
+        lap.swipe_to_delete()
+        lap.confirm_removal()
+
+
 class AlarmPage(Page):
     """Autopilot helper for the Alarm page."""
 

=== modified file 'tests/autopilot/ubuntu_clock_app/tests/test_stopwatch.py'
--- tests/autopilot/ubuntu_clock_app/tests/test_stopwatch.py	2014-02-19 12:03:16 +0000
+++ tests/autopilot/ubuntu_clock_app/tests/test_stopwatch.py	2014-02-27 00:04:13 +0000
@@ -20,100 +20,71 @@
 
 from __future__ import absolute_import
 
-from testtools.matchers import Equals, NotEquals, Is, Not
+import time
+from testtools.matchers import Equals, NotEquals
 from autopilot.matchers import Eventually
 
 from ubuntu_clock_app.tests import ClockAppTestCase
 
+
 class TestStopwatch(ClockAppTestCase):
     """Tests the stopwatch page features"""
 
-    """ This is needed to wait for the application to start.
-        In the testfarm, the application may take some time to show up."""
     def setUp(self):
+        """ This is needed to wait for the application to start.
+
+        In the testfarm, the application may take some time to show up.
+
+        """
         super(TestStopwatch, self).setUp()
         self.assertThat(
             self.main_view.visible, Eventually(Equals(True)))
 
-        # Move to stopwatch tab
-        self.main_view.switch_to_tab("StopwatchTab")
-
-    def tearDown(self):
-        super(TestStopwatch, self).tearDown()
-
-    def create_lap(self):
-        """Function to create a stopwatch lap"""
-        lap_button = self.main_view.get_lap_button()
-        self.pointing_device.click_object(lap_button)
-
-    def start_stop_stopwatch(self):
-        """Function to start/stop the stopwatch"""
-        start_stop_button = self.main_view.get_stopwatch_button()
-        self.pointing_device.click_object(start_stop_button)
-
-    def test_start_stop_reset_stopwatch(self):
-        """Test to check the proper functioning of the start/stop/reset of stopwatch"""
-
-        stopwatch_label = self.main_view.get_stopwatch_label()
-        reset_button = self.main_view.get_reset_button()
-
-        # Start the stopwatch and check if the stopwatch starts ticking
-        stopwatch_label_before = self.main_view.get_stopwatch_label().text
-        self.start_stop_stopwatch()
-        self.assertThat(stopwatch_label.text, Eventually(NotEquals("00:00.0")))
-
-        # Stop the stopwatch and check it has indeed stopped
-        self.start_stop_stopwatch()
-        self.assertThat(stopwatch_label_before, NotEquals(self.main_view.get_stopwatch_label().text))
-        stopwatch_label_after = lambda: self.main_view.get_stopwatch_label().text
-        self.assertThat(stopwatch_label_after, Eventually(Equals(self.main_view.get_stopwatch_label().text)))
-
-        # Press reset and check if the stopwatch has been resetted
-        self.pointing_device.click_object(reset_button)
-        self.assertThat(stopwatch_label.text, Eventually(Equals("00:00.0")))
-
-    def test_create_lap(self):
-        """Test to check creating a new stopwatch lap"""
-
-        # Start the stopwatch
-        stopwatch_label = self.main_view.get_stopwatch_label().text
-        self.start_stop_stopwatch()
-
-        # Ensure that the stopwatch actually ticks
-        self.assertThat(stopwatch_label, Eventually(NotEquals("00:00.0")))
-
-        # Create a stopwatch lap by pressing the lap button
-        num_of_laps_old = self.main_view.get_num_of_laps()
-        self.create_lap()
-
+        self.page = self.main_view.open_stopwatch()
+
+    def test_click_clock_center_must_start_stopwatch(self):
+        """Test if start button starts the stopwatch"""
+        stopwatch_time = self.page.get_stopwatch_time()
+        self.page.start_stopwatch()
+        # Check the stopwatch started running
+        self.assertThat(stopwatch_time, Eventually(NotEquals('00:00.0')))
+
+    def test_click_clock_center_with_stopwatch_started_must_stop_it(self):
+        """Test if stop button stops the stopwatch"""
+        self.page.start_stopwatch()
+        self.page.stop_stopwatch()
+        # Check that the stopwatch has stopped
+        stopwatch_time = self.page.get_stopwatch_time()
+        time.sleep(1)
+        self.assertThat(
+            stopwatch_time,
+            Equals(self.page.get_stopwatch_time()))
+
+    def test_restart_button_must_restart_stopwatch_time(self):
+        """Test if reset button resets the stopwatch time"""
+        self.page.start_stopwatch()
+        stopwatch_time = self.page.get_stopwatch_time()
+        self.page.reset_stopwatch()
+        # Check that the stopwatch has been reset
+        self.assertThat(stopwatch_time, Eventually(Equals('00:00.0')))
+
+    def test_lap_button_must_create_stopwatch_lap(self):
+        """Test if lap button creates a stopwatch lap"""
+        self.page.start_stopwatch()
+        num_of_laps_old = self.page.get_num_of_laps()
+        self.page.create_lap()
         # Confirm that the lap is created
         self.assertThat(
-            self.main_view.get_num_of_laps,
+            self.page.get_num_of_laps,
             Eventually(Equals(num_of_laps_old + 1)))
 
-    def test_delete_lap(self):
-        """Test to check deleting a stopwatch lap"""
-
-        # Start the stopwatch
-        stopwatch_label = self.main_view.get_stopwatch_label().text
-        self.start_stop_stopwatch()
-
-        # Ensure that the stopwatch actually ticks
-        self.assertThat(stopwatch_label, Eventually(NotEquals("00:00.0")))
-
-        # Create a stopwatch lap
-        self.create_lap()
-
-        num_of_laps_old = self.main_view.get_num_of_laps()
-        if not(self.main_view.wideAspect):
-            self.main_view.drag_page_up("StopwatchPage")
-
-        # Delete stopwatch lap
-        first_lap = self.main_view.get_first_laps_list_item()
-        first_lap.swipe_to_delete()
-        first_lap.confirm_removal()
-
+    def test_delete_lap_must_delete_from_stopwatch_lap_list(self):
+        """Test to delete a lap and check if it has been removed properly"""
+        self.page.start_stopwatch()
+        self.page.create_lap()
+        num_of_laps_old = self.page.get_num_of_laps()
+        self.page.delete_lap(index=0)
         # Check that the stopwatch lap has been deleted
         self.assertThat(
-            self.main_view.get_num_of_laps,
+            self.page.get_num_of_laps,
             Eventually(Equals(num_of_laps_old - 1)))