Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@ jobs:
run: pip install -r requirements.txt
- name: check-format
run: python ./check_format.py
- name: run-tests
run: pytest
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a new follow-up command to run pytest, which will find all the tests inside the package.

2 changes: 2 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
autopep8
python-dbusmock
jeepney
lyriq
pycodestyle
pylint
pytest
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added 2 packages -- not sure why I added them in these specific lines, weird. But anyway, pytest a pretty modern test finder and runner, so that should be fine. python-dbusmock is a convenient library to build mockup testing for libraries that depend heavily on dbus communication.

setuptools
twine
49 changes: 26 additions & 23 deletions spotifycli/spotifycli.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,27 +135,28 @@ def add_arguments():

def get_arguments():
return [
("--version", "shows version number"),
("--status", "shows song name and artist"),
("--version", "shows version number", False),
("--status", "shows song name and artist", False),
(
"--statusposition",
"shows song name and artist, with current playback position"
"shows song name and artist, with current playback position",
False
),
("--statusshort", "shows status in a short way"),
("--song", "shows the song name"),
("--songshort", "shows the song name in a short way"),
("--artist", "shows artist name"),
("--artistshort", "shows artist name in a short way"),
("--album", "shows album name"),
("--position", "shows song position"),
("--arturl", "shows album image url"),
("--playbackstatus", "shows playback status"),
("--play", "plays the song"),
("--pause", "pauses the song"),
("--playpause", "plays or pauses the song (toggles a state)"),
("--lyrics", "shows the lyrics for the song"),
("--next", "plays the next song"),
("--prev", "plays the previous song"),
("--statusshort", "shows status in a short way", False),
("--song", "shows the song name", False),
("--songshort", "shows the song name in a short way", False),
("--artist", "shows artist name", False),
("--artistshort", "shows artist name in a short way", False),
("--album", "shows album name", False),
("--position", "shows song position", False),
("--arturl", "shows album image url", False),
("--playbackstatus", "shows playback status", False),
("--play", "plays the song", False),
("--pause", "pauses the song", False),
("--playpause", "plays or pauses the song (toggles a state)", False),
("--lyrics", "shows the lyrics for the song", False),
("--next", "plays the next song", False),
("--prev", "plays the previous song", False),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are still needed, right? I can't quite tell where they went. But if I'm wrong and you removed them intentionally, let me know and I'll be happy to update it!

("--songuri", "plays the track at the provided Uri", True),
("--listuri", "plays the playlist at the provided Uri", True),
]
Expand All @@ -167,7 +168,9 @@ def show_version():

def get_song():
metadata = get_spotify_property("Metadata")
artist = ", ".join(metadata['xesam:artist'][1])
artist = metadata['xesam:artist'][1]
if isinstance(artist, list):
artist = ", ".join(artist)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this change is an artifact of dbus message mocking. I'll share more detail in a following comment here, but as it is right now, the get_song function can take artist as a string or list of strings. It shouldn't have any effect on the operation of the library.

title = metadata['xesam:title'][1]
return artist, title

Expand All @@ -192,9 +195,9 @@ def show_status_position():
artist, title = get_song()

# Both values are in microseconds
position = datetime.timedelta(milliseconds=position_raw / 1000)
position = datetime.timedelta(milliseconds=int(position_raw) / 1000)
length = datetime.timedelta(
milliseconds=metadata['mpris:length'][1] / 1000
milliseconds=int(metadata['mpris:length'][1]) / 1000
)

p_hours, p_minutes, p_seconds = convert_timedelta(position)
Expand Down Expand Up @@ -350,9 +353,9 @@ def show_position():
metadata = get_spotify_property("Metadata")
position_raw = get_spotify_property("Position")
# Both values are in microseconds
position = datetime.timedelta(milliseconds=position_raw / 1000)
position = datetime.timedelta(milliseconds=int(position_raw) / 1000)
length = datetime.timedelta(
milliseconds=metadata['mpris:length'][1] / 1000
milliseconds=int(metadata['mpris:length'][1]) / 1000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And these int changes are the other artifacts of dbus message mocking. Again, I'll provide more detail on this in a following comment, but as it is right now, this function will allow these position values to come in as either string numerics, or actual numerics. It will int them either way, so it will have no effect on existing behavior.

)

p_hours, p_minutes, p_seconds = convert_timedelta(position)
Expand Down
Empty file added spotifycli/test/__init__.py
Empty file.
14 changes: 14 additions & 0 deletions spotifycli/test/dbus_metadata_response.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
if args[1] == 'Metadata':
ret = {
'xesam:artist': ['', 'The Beatles'],
'xesam:title': ['', 'Yesterday'],
'xesam:album': ['', 'Help!'],
'mpris:length': ['', '10000000'],
'mpris:artUrl': ['', 'https://github.com/pwittchen/spotify-cli-linux'],
}
elif args[1] == 'PlaybackStatus':
ret = 'Playing'
elif args[1] == 'Position':
ret = 1000000
else:
ret = args
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Here's the meat of the testing work: building out a pretend dbus response based on the lookup argument.

What is ret?

When you mock up dbus and provide a fake interface, you need to provide some Python code to be executed within the context of an existing Python template. In this template, it will return the value of the variable ret, so the main outcome here is setting that variable.

What is args?

As with ret, in the dbus template, args is passed in as the body of the dbus method call. Just grab the index of interest and see what the dbus call was, and decide what to return.

Marshaling data

Dbus seems to be exceedingly annoying about the particular types being returned. For example, when you look up Metadata, it returns a dictionary of key value pairs. The values are all lists, where the first item is empty and the second item is the meaningful value. Note that the meaningful value here is always a string, even when it's a numeric like the mpris:length. And when you run dbus locally, the mpris:artist actually returns a list of artists in that meaningful item.

But, if I try to provide a slight variation of types on any of the meaningful items there, the dbus marshalling functions fail aggressively. I would love to mimic the way it actually works on my computer by instead of saying 'The Beatles', I could say ['The Beatles', ]. But unfortunately I can't mix that with the other types. And if I try to use a numeric value for mpris:length instead of a string number, it also fails. So annoying.

The root cause is that the dbus Python package is trying to detect the types dynamically so that it can marshal the data into the right underlying types to perform dbus communication. Dbus does allow dicts and objects, floats, ints, and bools, and more. But when you pass the data in, you need to tell Dbus exactly what each data member is.

Anyway, this little bit of annoyance is what caused the minor tweaks to the spotifycli.py file above to handle data coming in as a list/string or as a int/string.

Best path forward

I learned more about dbus today than I ever intended. The best way forward would be if someone else who knew a little bit more could come along and help me specify these return values better, we could revert my little tweaks to the library above. And that would be great. I'll keep trying as well...

163 changes: 163 additions & 0 deletions spotifycli/test/test_spotifycli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
from io import StringIO
from pathlib import Path
from subprocess import PIPE
from unittest import skip
from unittest.mock import patch

import dbus
import dbusmock

from spotifycli.spotifycli import main
from spotifycli.version import __version__


class TestSpotifyCLI(dbusmock.DBusTestCase):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are creating a new unit test class that inherits from the dbusmock test case for convenient access to things like spawn_server. Otherwise it's really just a plain unittest.TestCase

@classmethod
def setUpClass(cls):
cls.start_session_bus()
cls.dbus_con = cls.get_dbus(system_bus=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each time we start running unit tests, this classmethod sets up the dbus connection. Could probably get away with putting this inside the plain setUp that is called for each test, but I think this is more efficient and works fine.


def setUp(self):
self.dbus_spotify_server_mock = self.spawn_server(
"org.mpris.MediaPlayer2.spotify",
"/org/mpris/MediaPlayer2",
"org.freedesktop.DBus.Properties",
system_bus=False,
stdout=PIPE
)
self.dbus_spotify_interface_mock = dbus.Interface(
self.dbus_con.get_object(
'org.mpris.MediaPlayer2.spotify', '/org/mpris/MediaPlayer2'
),
dbusmock.MOCK_IFACE
)
metadata_file = Path(__file__).resolve().parent / 'dbus_metadata_response.py'
self.dbus_spotify_interface_mock.AddMethod(
'', 'Get', 'ss', 'v',
metadata_file.read_text(),
)
# TODO: Mock up the controls bus/interface, since I think it is slightly different
# self.dbus_spotify_interface_mock.AddMethod(
# '', 'Play', '', '', ''
# )
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each unit test run, we spin up the actual spotify dbus session server and build up an interface where we can stand up mock methods to be callable. We then use AddMethod to add a single Get operation that we can use to read current metadata, etc. In this case the return value from this dbus call will actually be the result of executing that little Python script above and grabbing the value from the ret variable. Pretty nice, but could be better.

What are the other args in AddMethod?

The second argument is the name of the method to add. The third is the argument type being passed into the dbus call. In this case, it is two strings, as found in this line:

body=("org.mpris.MediaPlayer2.Player", spotify_property)

These arguments are marshaled as strings ('ss') and placed into the args array when the Python script above is called. That is why we check args[1], because that will contain the specific_property we are asking for.

The fourth argument is a v, because it is currently marshaled as a variant. In this option, the Dbus package must dissect each variable type recursively and build out the dbus message with each argument type defined before its value. If we can figure out how to do this part a little better, this is how we can fix the numeric/string/list changes I had to make to get it working.


def tearDown(self):
self.dbus_spotify_server_mock.stdout.close()
self.dbus_spotify_server_mock.terminate()
self.dbus_spotify_server_mock.wait()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After each unit test, clean up the instance data.


def test_cli_usage(self):
with patch('sys.argv', ["spotifycli", "--help"]), patch("sys.exit") as mock_exit, patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn("usage", buffer.getvalue())
mock_exit.assert_called_with(0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, here's a whole list of individual unit tests that test each of the different command line options. I'll describe it here once in general:

  • Each test starts with a with block, where we utilize some patches to adjust behavior for the unit test:
    • patch('sys.argv', ["spotifycli", "--help"]) will override the values of sys.argv for the whole test so that the argparse library sees it the way we want.
    • patch("sys.exit") as mock_exit will capture sys.exit calls so we can try to make sure the process succeeded as it is about to exit back to the shell
    • patch('sys.stdout', new_callable=StringIO) as buffer creates a new string buffer that overrides sys.stdout, where we can check to see what was printed to the command line window.
  • We then call main, which will operate based on the patched argv
  • We then do assertions on the call result, or whether certain strings are found, etc.


def test_cli_version(self):
with patch('sys.argv', ["spotifycli", "--version"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn(__version__, buffer.getvalue())

def test_cli_status(self):
with patch('sys.argv', ["spotifycli", "--status"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn("The Beatles - Yesterday", buffer.getvalue())

def test_cli_status_position(self):
with patch('sys.argv', ["spotifycli", "--statusposition"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
output = buffer.getvalue()
self.assertIn('00:01', output)
self.assertIn('00:10', output)

def test_cli_status_short(self):
with patch('sys.argv', ["spotifycli", "--statusshort"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
output = buffer.getvalue()
self.assertIn('Beatles', output)
self.assertIn('Yesterday', output)

def test_cli_song(self):
with patch('sys.argv', ["spotifycli", "--song"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn("Yesterday", buffer.getvalue())

def test_cli_song_short(self):
# TODO: Change to a long song title to check the trimming
with patch('sys.argv', ["spotifycli", "--songshort"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn("Yesterday", buffer.getvalue())

def test_cli_artist(self):
with patch('sys.argv', ["spotifycli", "--artist"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn("Beatles", buffer.getvalue())

def test_cli_artist_short(self):
# TODO: Change to a long artist name to check the trimming
with patch('sys.argv', ["spotifycli", "--artistshort"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn("Beatles", buffer.getvalue())

def test_cli_album(self):
with patch('sys.argv', ["spotifycli", "--album"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn("Help!", buffer.getvalue())

def test_cli_position(self):
with patch('sys.argv', ["spotifycli", "--position"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
output = buffer.getvalue()
self.assertIn('00:01', output)
self.assertIn('00:10', output)

def test_cli_art_url(self):
with patch('sys.argv', ["spotifycli", "--arturl"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
self.assertIn('http', buffer.getvalue())

def test_cli_playback_status(self):
with patch('sys.argv', ["spotifycli", "--playbackstatus"]), patch('sys.stdout', new_callable=StringIO) as buffer:
main()
playback_symbols = ['▶', '▮▮', '■']
output = buffer.getvalue()
self.assertTrue(any([x in output for x in playback_symbols]))

@skip('Lyrics dont seem to be working right now...is it still valid?')
def test_cli_lyrics(self):
with patch('sys.argv', ["spotifycli", "--lyrics"]):
main()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following tests are decorated with @skip because they currently fail.

For this one, I'm not convinced the lyrics database is still available -- maybe I'm missing something. But if not, that command line switch should probably just be hidden away for now.


@skip('TODO: Need to mock up the spotify_action dbus interface')
def test_cli_play(self):
with patch('sys.argv', ["spotifycli", "--play"]):
main()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of these are skip-ed because there needs to be a second interface built up for actuating the mocked dbus server where it will accept these command line arguments.


@skip('TODO: Need to mock up the spotify_action dbus interface')
def test_cli_pause(self):
with patch('sys.argv', ["spotifycli", "--pause"]):
main()

@skip('TODO: Need to mock up the spotify_action dbus interface')
def test_cli_play_pause(self):
with patch('sys.argv', ["spotifycli", "--playpause"]):
main()

@skip('TODO: Need to mock up the spotify_action dbus interface')
def test_cli_next(self):
with patch('sys.argv', ["spotifycli", "--next"]):
main()

@skip('TODO: Need to mock up the spotify_action dbus interface')
def test_cli_prev(self):
with patch('sys.argv', ["spotifycli", "--prev"]):
main()

@skip('TODO: Need to mock up the spotify_action dbus interface')
def test_cli_songuri(self):
with patch('sys.argv', ["spotifycli", "--songuri"]):
main()

@skip('TODO: Need to mock up the spotify_action dbus interface')
def test_cli_listuri(self):
with patch('sys.argv', ["spotifycli", "--listuri"]):
main()
Loading