New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Update playback code to take change track into account. #1071

Merged
merged 3 commits into from Mar 22, 2015

Conversation

3 participants
@adamcik
Member

adamcik commented Mar 22, 2015

This change has us checking the return value of change_track when deciding if
the play call was a success or if the track is unplayable. Which ensures that
the following can no longer happen: 1) play stream 2) play stream that fails
change_track 3) stream 1) continues playing. Correct behavior being the next
stream playing instead.

adamcik added some commits Mar 22, 2015

core: Update playback code to take change track into account.
This change has us checking the return value of change_track when deciding if
the play call was a success or if the track is unplayable. Which ensures that
the following can no longer happen: 1) play stream 2) play stream that fails
change_track 3) stream 1) continues playing. Correct behavior being the next
stream playing instead.
@@ -124,6 +125,22 @@ def test_play_skips_to_next_on_unplayable_track(self):
self.assertEqual(
self.core.playback.current_tl_track, self.tl_tracks[3])
def test_play_skips_to_next_on_unplayable_track(self):
"""Checks that we handle change track failing."""

This comment has been minimized.

@kingosticks

kingosticks Mar 22, 2015

Member

Should that be change_track or even the fully referenced name?

@jodal jodal added this to the v1.0 - Audio cleanup 1 milestone Mar 22, 2015

@jodal jodal self-assigned this Mar 22, 2015

@@ -124,6 +125,22 @@ def test_play_skips_to_next_on_unplayable_track(self):
self.assertEqual(
self.core.playback.current_tl_track, self.tl_tracks[3])
def test_play_skips_to_next_on_unplayable_track(self):
"""Checks that we handle change track failing."""
self.playback2.change_track().get.return_value = False

This comment has been minimized.

@jodal

jodal Mar 22, 2015

Member

We do this wrong all over the place, and thus we often need to use reset_mock() in the beginning of tests.

The proper way is to replace () with .return_value.

This comment has been minimized.

@adamcik

adamcik Mar 22, 2015

Member

Not sure I quite follow this?

This comment has been minimized.

@jodal

jodal Mar 22, 2015

Member

Instead of actually calling the method on the first mock and increase its call counts, use return_value to get hold of the inner mock.

self.playback2.change_track.return_value.get.return_value = False
@jodal

This comment has been minimized.

Member

jodal commented Mar 22, 2015

+1

@jodal jodal added the 2 - Working label Mar 22, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 22, 2015

Fixed.

@jodal jodal removed the 2 - Working label Mar 22, 2015

jodal added a commit that referenced this pull request Mar 22, 2015

Merge pull request #1071 from adamcik/fix/change-track-failure-should…
…-fail-playback

core: Update playback code to take change track into account.

@jodal jodal merged commit 5eebab6 into mopidy:develop Mar 22, 2015

2 of 3 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
Scrutinizer 1 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adamcik adamcik deleted the adamcik:fix/change-track-failure-should-fail-playback branch Mar 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment