Add testing #88
Add testing #88
Conversation
There was a problem hiding this comment.
I would love to hear your thoughts on this. I definitely don't think it's ready to go in (I mean it works but could be better). I may try to hit it over the coming weekend and build up more dbus expertise just for fun :) but if anyone out there can provide suggestions, I'm happy to take them and fix things up.
| strategy: | ||
| matrix: | ||
| python-version: ["3.6", "3.7"] | ||
| python-version: ["3.6", "3.7"] # TODO: Both of these have reached end of life, should probably move to 3.12+ |
There was a problem hiding this comment.
Just a couple notes about these versions being quite old. Probably want to bump them up, and I don't see any reason that doing so would cause any issues on GitHub Actions.
| @@ -25,3 +25,5 @@ jobs: | |||
| run: pip install -r requirements.txt | |||
| - name: check-format | |||
| run: python ./check_format.py | |||
| - name: run-tests | |||
| run: pytest | |||
There was a problem hiding this comment.
Add a new follow-up command to run pytest, which will find all the tests inside the package.
| jeepney | ||
| lyricwikia | ||
| pycodestyle | ||
| pylint | ||
| pytest |
There was a problem hiding this comment.
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.
| ("--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), |
There was a problem hiding this comment.
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!
| artist = ", ".join(metadata['xesam:artist'][1]) | ||
| artist = metadata['xesam:artist'][1] | ||
| if isinstance(artist, list): | ||
| artist = ", ".join(artist) |
There was a problem hiding this comment.
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.
| # TODO: Mock up the controls bus/interface, since I think it is slightly different | ||
| # self.dbus_spotify_interface_mock.AddMethod( | ||
| # '', 'Play', '', '', '' | ||
| # ) |
There was a problem hiding this comment.
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:
spotify-cli-linux/spotifycli/spotifycli.py
Line 293 in 43143c2
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() |
There was a problem hiding this comment.
After each unit test, clean up the instance data.
| 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) |
There was a problem hiding this comment.
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
withblock, 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_exitwill capturesys.exitcalls so we can try to make sure the process succeeded as it is about to exit back to the shellpatch('sys.stdout', new_callable=StringIO) as buffercreates a new string buffer that overridessys.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.
| @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() |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
|
Heads up, it looks like the GitHub Action failed because of a dependency issue. I think it's just because it's pinned back to Python 3.7. I would guess pulling that up to something more recent would fix it. I can do that here if you'd like, or if you'd prefer to do that in a standalone PR that goes in separate, I'll hold off. |
|
Thanks for the PR! Looks like you did a lot of work and analysis. I'll review it when I find some time. |
GH Action failed during installing requirements. I don't know why, because requirements were not changed. You can verify if your local setup is the same like the setup in the repo to reproduce and fix this issue. |
Alright, so here's a starting point at addressing #12 . To test this in the current form, I decided to use mocks and build up a session dbus server and interface. I then tried to mimic the responses from dbus as close as possible to ensure that the functions in the package are handling it properly. There are a few gotchas, and I'll just start by saying I'm wide open to criticism and suggestions on this. As it sits when I'm opening this PR, I definitely consider it a starting point rather than a final product. I'll give a code walkthrough to explain the changes.