Bugfixes and Unit Tests #69
Conversation
Adds unit tests for OCP class
Codecov Report
@@ Coverage Diff @@
## dev #69 +/- ##
=====================================
Coverage ? 0.00%
=====================================
Files ? 13
Lines ? 2633
Branches ? 0
=====================================
Hits ? 0
Misses ? 2633
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
WIP OCPMediaPlayer tests
|
Closes #22 |
…hods Raise an exception if trying to get a stream from nothing
Add init tests for registered events Fixes #22 with unit tests for duck/unduck calling pause/resume
|
Closes #66 |
Split `player` tests to separate test file
| if self.state == PlayerState.PLAYING: | ||
| self.pause() | ||
| self._paused_on_duck = True |
There was a problem hiding this comment.
should a DuckingState be introduced? could be useful if we want to signal it in the GUI/LED events etc
There was a problem hiding this comment.
I'm not sure the state would need to be more than a boolean.. Either we were playing and are now ducked, or we don't care about the ducking state, right?
There was a problem hiding this comment.
i meant as a generic property to be exposed via messagebus and MPRIS, thinking about 3rd party consumers of this info
There was a problem hiding this comment.
I think it would make more sense to put that in the audio module since we could then also resolve the TODO of waiting for audio output to end. It also would make it available to installations/devices that don't implement OCP media playback.
There was a problem hiding this comment.
not sure about that.... because OCP can be used standalone too, it is not a audio module property either, audio delegates playback to OCP so OCP should be the one reporting the end of playback
There was a problem hiding this comment.
dont mean this as a PR blocker, it can be done some other time, it's quite unimportant and out of scope! want those fixes and unittests in ASAP :)
NeonDaniel
force-pushed
the
FIX_OCPBugfixesWithTests
branch
from
bfc2f7b
to
b3e738c
Compare
| if state == MediaState.END_OF_MEDIA: | ||
| # playback ended, allow next track to change metadata again | ||
| self.reset() | ||
| # Don't do anything. Let OCP manage this object's state |
There was a problem hiding this comment.
this was added because the stream extractors dont replace old media, so if you send a request to play a uri only it will keep the title etc from previous track
the reset seems to be doing too much, but it is needed
There was a problem hiding this comment.
How would this request happen without going through OCP?
There was a problem hiding this comment.
OCP playback requests may contain full metadata or partial, metadata is then extracted from the uri automatically to fill gaps
if a playback request is missing info (title, artist, duration....) NowPlaying will keep the old info when the next track comes up, the stream extractors will refuse to replace the NowPlaying data (except uri) since they are a fallback, they assume the skill provided more accurate metadata than they are extracting
end result, Track 1 title with Track 2 playing
There was a problem hiding this comment.
note that the extractors only work at playback start, they have no way to know if the data in NowPlaying is old or not
if the reset is moved out of here it then needs to be done elsewhere before any playback request, that has more possible code paths so it seemed easier/safer to do it in here via bus events
There was a problem hiding this comment.
The call was moved here, is there another place I should add it? This handled all the paths I could find within OCP..
There was a problem hiding this comment.
i missed that change, guess that works!
please add a test for the specific scenario mentioned above if you haven't yet
There was a problem hiding this comment.
I didn't look at the diff lately and didn't realize how buried it is in other changes.
Resolve error relating to #65
Adds unit tests for OCP class
Closes #22
Closes #66