Merge lp:~gerlowskija/music-app/repeat_button_autopilot_tests into lp:music-app/trusty

[Jason]

* Add Autopilot tests for repeat functionality
* Add utility functions for shuffle and repeat

This branch adds 4 autopilot tests to the music_app test suite, that test the behavior seen when moving between songs with the "Repeat" option toggled on.

My first stab at autopilot tests, so I'd appreciate some feedback/help where I'm sure I messed up.

[Jason (gerlowskija)]

A few notes about this branch:

1) I wrote some utility functions for code that I saw reqpeated frequently across the 4 tests I added. When I did this, I also
replaced code in existing tests to use the new utility method where appropriate. I figured this was small enough to leave in the same commit/revision, but if it should be in its own, I can do that too.

2) I had trouble determining programmatically what the first and last songs in a given "Now Playing" queue were. (I was able to get a list of songs in the queue relatively easy, but since select_many() returns results in an indeterminate order, I had trouble using it to determine where songs came in the queue.) So I had to hardcode what I expected the songs to be (see the FIRST_TITLE and LAST_TITLE constants). I'd like to avoid having these hardcoded values; does anyone have any advice on how to get an ordered list of songs/titles in the Now Playing queue?

[Victor Thompson (vthompson)]

The added tests pass on the device. I'll do a more in depth review later on tonight.

[Victor Thompson (vthompson)]

Thanks Jason! Could we use your turn_shuffle_off() utility function for the test_shuffle test?

Other than that I think this looks good. We can investigate what we do with the "next when repeat on" and "previous when repeat on" tests once the media hub capability lands. Good work! :)

review: Needs Information

[Jason (gerlowskija)]

Thanks for taking a look Victor; I'm glad to help! Or try at least : )

I'm pretty sure my change already has the turn_shuffle_off() function being used in the test_shuffle test you mentioned. You can see it at line 310 of the test_music.py (or line 104 in the diff below). It's pretty possible I'm just missing a spot, or looking in the wrong place, so feel free to double check me, but I think the call is already used there.

Let me know if there's anything I can change, or any other comments you have.

Jason

[Victor Thompson (vthompson)]

See around L290:

            #ensure shuffle is on
            if not self.player.shuffle:
                logger.debug("Turning on shuffle")
                self.pointing_device.click_object(shufflebutton)
            else:
                logger.debug("Shuffle already on")
            self.assertThat(self.player.shuffle, Eventually(Equals(True)))

Or just search for "shufflebutton". In the same regard, you can delete L255 (but this was a pointless line of code before your changes--so feel free to disregard this suggestion):

        shufflebutton = self.main_view.get_shuffle_button()

[Jason (gerlowskija)]

Oh I see. I didn't make a utility to turn shuffle on, since in the tests I
added I just needed to turn it off. But it makes sense to add a function
for it, and add it in. Good catch! I'll push up a revised change in a bit.

On Thu, Apr 10, 2014 at 10:11 PM, Victor Thompson <<email address hidden>
> wrote:

> See around L290:
>
> #ensure shuffle is on
> if not self.player.shuffle:
> logger.debug("Turning on shuffle")
> self.pointing_device.click_object(shufflebutton)
> else:
> logger.debug("Shuffle already on")
> self.assertThat(self.player.shuffle, Eventually(Equals(True)))
>
>
> Or just search for "shufflebutton". In the same regard, you can delete
> L255 (but this was a pointless line of code before your changes--so feel
> free to disregard this suggestion):
>
> shufflebutton = self.main_view.get_shuffle_button()
>
>
> --
>
> https://code.launchpad.net/~gerlowskija/music-app/repeat_button_autopilot_tests/+merge/214870
> You are the owner of
> lp:~gerlowskija/music-app/repeat_button_autopilot_tests.
>

[Victor Thompson (vthompson)]

This looks good and everything passed on my device. Thanks Jason!

review: Approve

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

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~gerlowskija/music-app/repeat_button_autopilot_tests/+merge/214870/+edit-commit-message

review: Needs Fixing (continuous-integration)

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

FAILED: Autolanding.
More details in the following jenkins job:
http://91.189.93.70:8080/job/music-app-autolanding/431/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2041/console
    FAILURE: http://91.189.93.70:8080/job/music-app-raring-amd64-autolanding/430/console
    FAILURE: http://91.189.93.70:8080/job/music-app-saucy-amd64-autolanding/430/console
    FAILURE: http://91.189.93.70:8080/job/music-app-trusty-amd64-autolanding/125/console

review: Needs Fixing (continuous-integration)

[Victor Thompson (vthompson)]

Jason,

There are a few things that need fixing. Pep8 is a program that checks python coding standards. You can check this by installing 'pep8' and running it against the python source like so:

$ pep8 tests/autopilot/music_app/tests/test_music.py

tests/autopilot/music_app/tests/test_music.py:40:80: E501 line too long (83 > 79 characters)
tests/autopilot/music_app/tests/test_music.py:621:80: E501 line too long (80 > 79 characters)
tests/autopilot/music_app/tests/test_music.py:654:80: E501 line too long (80 > 79 characters)

Here's what the Jenkins CI infrastructure sees as the issue:

+ pep8 --repeat --show-source .
./tests/autopilot/music_app/tests/test_music.py:40:80: E501 line too long (83 > 79 characters)
        song = self.main_view.get_album_sheet_listview_tracktitle(self.FIRST_TITLE)
                                                                               ^
./tests/autopilot/music_app/tests/test_music.py:621:80: E501 line too long (80 > 79 characters)
        """With repeat on, the first song should play after the last one ends"""
                                                                               ^
./tests/autopilot/music_app/tests/test_music.py:654:80: E501 line too long (80 > 79 characters)
        """With repeat on, 'previous' from the first song plays the last one."""
                                                                               ^
./debian/tmp/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:40:80: E501 line too long (83 > 79 characters)
        song = self.main_view.get_album_sheet_listview_tracktitle(self.FIRST_TITLE)
                                                                               ^
./debian/tmp/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:621:80: E501 line too long (80 > 79 characters)
        """With repeat on, the first song should play after the last one ends"""
                                                                               ^
./debian/tmp/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:654:80: E501 line too long (80 > 79 characters)
        """With repeat on, 'previous' from the first song plays the last one."""
                                                                               ^
./debian/music-app-autopilot/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:40:80: E501 line too long (83 > 79 characters)
        song = self.main_view.get_album_sheet_listview_tracktitle(self.FIRST_TITLE)
                                                                               ^
./debian/music-app-autopilot/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:621:80: E501 line too long (80 > 79 characters)
        """With repeat on, the first song should play after the last one ends"""
                                                                               ^
./debian/music-app-autopilot/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:654:80: E501 line too long (80 > 79 characters)
        """With repeat on, 'previous' from the first song plays the last one."""

review: Needs Fixing

[Jason (gerlowskija)]

Ok, sounds easy enough. Just as a process question, should I make those changes as their own commit/revision, or is there a way to edit the branch I've already pushed to contain those changes in the original commit itself?

> Jason,
>
> There are a few things that need fixing. Pep8 is a program that checks python
> coding standards. You can check this by installing 'pep8' and running it
> against the python source like so:
>
> $ pep8 tests/autopilot/music_app/tests/test_music.py
>
> tests/autopilot/music_app/tests/test_music.py:40:80: E501 line too long (83 >
> 79 characters)
> tests/autopilot/music_app/tests/test_music.py:621:80: E501 line too long (80 >
> 79 characters)
> tests/autopilot/music_app/tests/test_music.py:654:80: E501 line too long (80 >
> 79 characters)
>
> Here's what the Jenkins CI infrastructure sees as the issue:
>
> + pep8 --repeat --show-source .
> ./tests/autopilot/music_app/tests/test_music.py:40:80: E501 line too long (83
> > 79 characters)
> song =
> self.main_view.get_album_sheet_listview_tracktitle(self.FIRST_TITLE)
> ^
> ./tests/autopilot/music_app/tests/test_music.py:621:80: E501 line too long (80
> > 79 characters)
> """With repeat on, the first song should play after the last one
> ends"""
> ^
> ./tests/autopilot/music_app/tests/test_music.py:654:80: E501 line too long (80
> > 79 characters)
> """With repeat on, 'previous' from the first song plays the last
> one."""
> ^
> ./debian/tmp/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:40:80: E501 line too long (83 > 79
> characters)
> song =
> self.main_view.get_album_sheet_listview_tracktitle(self.FIRST_TITLE)
> ^
> ./debian/tmp/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:621:80: E501 line too long (80 > 79
> characters)
> """With repeat on, the first song should play after the last one
> ends"""
> ^
> ./debian/tmp/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:654:80: E501 line too long (80 > 79
> characters)
> """With repeat on, 'previous' from the first song plays the last
> one."""
> ^
> ./debian/music-app-autopilot/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:40:80: E501 line too long (83 > 79
> characters)
> song =
> self.main_view.get_album_sheet_listview_tracktitle(self.FIRST_TITLE)
> ^
> ./debian/music-app-autopilot/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:621:80: E501 line too long (80 > 79
> characters)
> """With repeat on, the first song should play after the last one
> ends"""
> ^
> ./debian/music-app-autopilot/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:654:80: E501 line too long (80 > 79
> characters)
> """With repeat on, 'previous' from the first song plays the last
> one."""

[Victor Thompson (vthompson)]

You can use as many commits as you need. When Jenkins merges this branch it uses the commit message at the top of the page under "Commit Message". So, just continue to push your changes to lp:~gerlowskija/music-app/repeat_button_autopilot_tests and you should be good to go!

[Victor Thompson (vthompson)]

Looks good, thanks Jason!

review: Approve

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

FAILED: Autolanding.
More details in the following jenkins job:
http://91.189.93.70:8080/job/music-app-autolanding/436/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2067/console
    FAILURE: http://91.189.93.70:8080/job/music-app-raring-amd64-autolanding/435/console
    FAILURE: http://91.189.93.70:8080/job/music-app-saucy-amd64-autolanding/435/console
    FAILURE: http://91.189.93.70:8080/job/music-app-trusty-amd64-autolanding/130/console

review: Needs Fixing (continuous-integration)

[Victor Thompson (vthompson)]

Sorry Jason, pyflakes is another coding standard type program and it has complained about the following:

+ pyflakes .
./tests/autopilot/music_app/tests/test_music.py:12: 'sys' imported but unused
./tests/autopilot/music_app/tests/test_music.py:217: local variable 'shufflebutton' is assigned to but never used
./debian/tmp/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:12: 'sys' imported but unused
./debian/tmp/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:217: local variable 'shufflebutton' is assigned to but never used
./debian/music-app-autopilot/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:12: 'sys' imported but unused
./debian/music-app-autopilot/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:217: local variable 'shufflebutton' is assigned to but never used

I think you need to delete the two lines of code it points out.

review: Needs Fixing

[lp:~gerlowskija/music-app/repeat_button_autopilot_tests]

413. By Jason on 2014-04-12

Remove unused variable and import

[Jason (gerlowskija)]

It's no problem, they were things I should have caught anyway. Are pep8 and pyflakes run on all the core app builds (just for future reference)? Are there any other checks I should be doing before I push a branch?

> Sorry Jason, pyflakes is another coding standard type program and it has
> complained about the following:
>
> + pyflakes .
> ./tests/autopilot/music_app/tests/test_music.py:12: 'sys' imported but unused
> ./tests/autopilot/music_app/tests/test_music.py:217: local variable
> 'shufflebutton' is assigned to but never used
> ./debian/tmp/usr/lib/python2.7/dist-packages/music_app/tests/test_music.py:12:
> 'sys' imported but unused
> ./debian/tmp/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:217: local variable 'shufflebutton' is
> assigned to but never used
> ./debian/music-app-autopilot/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:12: 'sys' imported but unused
> ./debian/music-app-autopilot/usr/lib/python2.7/dist-
> packages/music_app/tests/test_music.py:217: local variable 'shufflebutton' is
> assigned to but never used
>
> I think you need to delete the two lines of code it points out.

[Victor Thompson (vthompson)]

Approved, thanks! Currently I think it's just pep8 and pyflakes that check the python source.

review: Approve

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

review: Approve (continuous-integration)

=== modified file 'tests/autopilot/music_app/tests/test_music.py'
--- tests/autopilot/music_app/tests/test_music.py	2014-04-02 00:19:32 +0000
+++ tests/autopilot/music_app/tests/test_music.py	2014-04-12 14:31:41 +0000
@@ -21,6 +21,8 @@
 
 
 class TestMainWindow(MusicTestCase):
+    FIRST_TITLE = "Foss Yeaaaah! (Radio Edit)"
+    LAST_TITLE = "TestMP3Title"
 
     def setUp(self):
         super(TestMainWindow, self).setUp()
@@ -30,6 +32,50 @@
         spinner = lambda: self.main_view.get_spinner().running
         self.assertThat(spinner, Eventually(Equals(False)))
 
+    def populate_and_play_queue(self):
+        first_genre_item = self.main_view.get_first_genre_item()
+        self.pointing_device.click_object(first_genre_item)
+
+        title = self.FIRST_TITLE
+        song = self.main_view.get_album_sheet_listview_tracktitle(title)
+        self.pointing_device.click_object(song)
+
+    def turn_shuffle_off(self):
+        if self.player.shuffle:
+            shufflebutton = self.main_view.get_shuffle_button()
+            logger.debug("Turning off shuffle")
+            self.pointing_device.click_object(shufflebutton)
+        else:
+            logger.debug("Shuffle already off")
+        self.assertThat(self.player.shuffle, Eventually(Equals(False)))
+
+    def turn_shuffle_on(self):
+        if not self.player.shuffle:
+            shufflebutton = self.main_view.get_shuffle_button()
+            logger.debug("Turning on shuffle")
+            self.pointing_device.click_object(shufflebutton)
+        else:
+            logger.debug("Shuffle already on")
+        self.assertThat(self.player.shuffle, Eventually(Equals(True)))
+
+    def turn_repeat_off(self):
+        if self.player.repeat:
+            repeatbutton = self.main_view.get_repeat_button()
+            logger.debug("Turning off repeat")
+            self.pointing_device.click_object(repeatbutton)
+        else:
+            logger.debug("Repeat already off")
+        self.assertThat(self.player.repeat, Eventually(Equals(False)))
+
+    def turn_repeat_on(self):
+        if not self.player.repeat:
+            repeatbutton = self.main_view.get_repeat_button()
+            logger.debug("Turning on repeat")
+            self.pointing_device.click_object(repeatbutton)
+        else:
+            logger.debug("Repeat already on")
+        self.assertThat(self.player.repeat, Eventually(Equals(True)))
+
     def test_reads_music_library(self):
         """ tests if the music library is populated from our
         fake mediascanner database"""
@@ -124,14 +170,7 @@
         self.pointing_device.click_object(playbutton)
         self.assertThat(self.player.isPlaying, Eventually(Equals(False)))
 
-        #ensure shuffle is off
-        if self.player.shuffle:
-            shufflebutton = self.main_view.get_shuffle_button()
-            logger.debug("Turning off shuffle")
-            self.pointing_device.click_object(shufflebutton)
-        else:
-            logger.debug("Shuffle already off")
-        self.assertThat(self.player.shuffle, Eventually(Equals(False)))
+        self.turn_shuffle_off()
 
         """ Select next """
         #goal is to go back and forth and ensure 2 different songs
@@ -174,18 +213,11 @@
         self.pointing_device.click_object(song)
 
         playbutton = self.main_view.get_now_playing_play_button()
-        shufflebutton = self.main_view.get_shuffle_button()
 
         title = self.player.currentMetaTitle
         artist = self.player.currentMetaArtist
 
-        #ensure shuffle is off
-        if self.player.shuffle:
-            logger.debug("Turning off shuffle")
-            self.pointing_device.click_object(shufflebutton)
-            self.assertThat(self.player.shuffle, Eventually(Equals(False)))
-        else:
-            logger.debug("Shuffle already off")
+        self.turn_shuffle_off()
 
         """ Track is playing """
         count = 1
@@ -228,7 +260,6 @@
         self.pointing_device.click_object(song)
 
         """ Track is playing, shuffle is turned on"""
-        shufflebutton = self.main_view.get_shuffle_button()
         forwardbutton = self.main_view.get_forward_button()
         playbutton = self.main_view.get_now_playing_play_button()
         previousbutton = self.main_view.get_previous_button()
@@ -261,13 +292,7 @@
             if (not self.main_view.toolbarShown):
                 self.main_view.show_toolbar()
 
-            #ensure shuffle is on
-            if not self.player.shuffle:
-                logger.debug("Turning on shuffle")
-                self.pointing_device.click_object(shufflebutton)
-            else:
-                logger.debug("Shuffle already on")
-            self.assertThat(self.player.shuffle, Eventually(Equals(True)))
+            self.turn_shuffle_on()
 
             self.pointing_device.click_object(forwardbutton)
             self.assertThat(self.player.isPlaying,
@@ -283,13 +308,7 @@
             self.assertThat(self.player.isPlaying,
                             Eventually(Equals(False)))
 
-            #ensure shuffle is off
-            if self.player.shuffle:
-                logger.debug("Turning off shuffle")
-                self.pointing_device.click_object(shufflebutton)
-            else:
-                logger.debug("Shuffle already off")
-            self.assertThat(self.player.shuffle, Eventually(Equals(False)))
+            self.turn_shuffle_off()
 
             self.pointing_device.click_object(previousbutton)
 
@@ -580,3 +599,71 @@
         # verify song has been deleted
         finalqueueCount = self.main_view.get_queue_track_count()
         self.assertThat(finalqueueCount, Equals(initialqueueCount - 1))
+
+    def test_playback_stops_when_last_song_ends_and_repeat_off(self):
+        """Check that playback stops when the last song in the queue ends"""
+        self.populate_and_play_queue()
+        self.turn_shuffle_off()
+        self.turn_repeat_off()
+
+        num_tracks = self.main_view.get_queue_track_count()
+
+        #Skip through all songs in queue, stopping on last one.
+        forward_button = self.main_view.get_forward_button()
+        for count in range(0, num_tracks - 1):
+            self.pointing_device.click_object(forward_button)
+
+        #When the last song ends, playback should stop
+        self.assertThat(self.player.isPlaying, Eventually(Equals(False)))
+
+    def test_playback_repeats_when_last_song_ends_and_repeat_on(self):
+        """With repeat on, the 1st song should play after the last one ends"""
+        self.populate_and_play_queue()
+        self.turn_shuffle_off()
+        self.turn_repeat_on()
+
+        num_titles = self.main_view.get_queue_track_count()
+        #Skip through all songs in queue, stopping on last one.
+        forward_button = self.main_view.get_forward_button()
+        for count in range(0, num_titles - 1):
+            self.pointing_device.click_object(forward_button)
+
+        #Make sure we loop back to first song after last song ends
+        actual_title = lambda: self.player.currentMetaTitle
+        self.assertThat(actual_title, Eventually(Equals(self.FIRST_TITLE)))
+        self.assertThat(self.player.isPlaying, Eventually(Equals(True)))
+
+    def test_pressing_next_from_last_song_plays_first_when_repeat_on(self):
+        """With repeat on, skipping the last song jumps to the first track"""
+        self.populate_and_play_queue()
+        self.turn_shuffle_off()
+        self.turn_repeat_on()
+
+        num_titles = self.main_view.get_queue_track_count()
+        #Skip through all songs in queue, INCLUDING last one.
+        forward_button = self.main_view.get_forward_button()
+        for count in range(0, num_titles - 1):
+            self.pointing_device.click_object(forward_button)
+
+        actual_title = lambda: self.player.currentMetaTitle
+        self.assertThat(actual_title, Eventually(Equals(self.FIRST_TITLE)))
+        self.assertThat(self.player.isPlaying, Eventually(Equals(True)))
+
+    def test_pressing_prev_from_first_song_plays_last_when_repeat_on(self):
+        """With repeat on, 'previous' from the 1st song plays the last one."""
+        self.populate_and_play_queue()
+        self.turn_shuffle_off()
+        self.turn_repeat_on()
+
+        prev_button = self.main_view.get_previous_button()
+        initial_song = self.player.currentMetaTitle
+        self.pointing_device.click_object(prev_button)
+        #If we're far enough into a song, pressing prev just takes us to the
+        #beginning of that track.  In that case, hit prev again to actually
+        #skip over the track.
+        if self.player.currentMetaTitle == initial_song:
+            self.pointing_device.click_object(prev_button)
+
+        actual_title = lambda: self.player.currentMetaTitle
+        self.assertThat(actual_title, Eventually(Equals(self.LAST_TITLE)))
+        self.assertThat(self.player.isPlaying, Eventually(Equals(True)))