Skip to content
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

Add initial test suite #992

Merged
1 commit merged into from Dec 31, 2020
Merged

Add initial test suite #992

1 commit merged into from Dec 31, 2020

Conversation

aklajnert
Copy link
Contributor

This is a very initial test suite that tests mostly the script's entrypoint.
The DownloadManager class is entirely mocked, so it not tested.
All the HTTP communication is mocked, the responses are stored within tests/cassettes directory (everything there is auto-generated).
I did also some minor changes in imports to make testing easier.

I can't see any good place to put a description of how to run tests, so I've created a README.md within the tests directory for now.

Given the fact that I've focused only to test a single module, the code coverage is decent:

----------- coverage: platform win32, python 3.8.3-final-0 -----------
Name                                  Stmts   Miss  Cover
---------------------------------------------------------
spotdl\__init__.py                        1      0   100%
spotdl\__main__.py                       41      4    90%
spotdl\download\__init__.py               1      0   100%
spotdl\download\downloader.py           107     88    18%
spotdl\download\progressHandlers.py      85     53    38%
spotdl\patches\__init__.py                1      1     0%
spotdl\patches\pyTube.py                 90     90     0%
spotdl\search\__init__.py                 1      0   100%
spotdl\search\provider.py               111     12    89%
spotdl\search\songObj.py                 70     25    64%
spotdl\search\spotifyClient.py           13      1    92%
spotdl\search\utils.py                   38     10    74%
---------------------------------------------------------
TOTAL                                   559    284    49%

If you are OK with what's already done, I think it could be merged and then I'll work further on extending the suite.

@ghost
Copy link

ghost commented Nov 22, 2020

Well, as embarassing as this is, I have never written up tests before. Any things unique about test code that i should know?

I'll look through your code on a general basis and let you know by the end of this week.

@aklajnert
Copy link
Contributor Author

Most of the tests are quite similar. First I fake sys.argv to simulate different command-line arguments.
Then I call the entry point function and see what is happening. I test if the valid message is printed to the console and if the correct method from DownloadManager has been called. The download methods are not executed as I mock them within patch_dependencies() fixture.

The tests are written with pytest framework, which is the best framework available for python, and one of the best testing frameworks in general. I recommend reading a bit about it, especially about fixtures which are very useful while testing.

And, well, practice makes perfect - start small and grow :)

@ghost
Copy link

ghost commented Nov 22, 2020

cool. Thanks for the "well, check this out and this too" - a little head start helps a lot. Thanks again.

@phcreery
Copy link
Contributor

@aklajnert Thanks a ton for getting this in motion. I, like @Mikhail-Zex , have never officially used tests but I see the need for them. Also thanks for documenting use in the readme.

Question, would the monkey patches be needed for asyncio?

@aklajnert
Copy link
Contributor Author

Question, would the monkey patches be needed for asyncio?

It depends on the test, what you want to achieve with it. Probably some mocks will be needed to make the tests faster.

If you merge this, I'll build on it to extend the tests and add some more. Preferably on the DownloadManager class after #994 will be merged.

@ghost
Copy link

ghost commented Nov 30, 2020

We're definitely merging this. Hey @phcreery review this. I will too.

@phcreery
Copy link
Contributor

Looks good to go for me.

@aklajnert
Copy link
Contributor Author

Should I also change a branch here, or can it go to master? If yes, then I think it could go to both actually.

@ghost
Copy link

ghost commented Dec 4, 2020

Change the branch. I'll merge it.

@aklajnert aklajnert changed the base branch from master to next-rel-dev December 4, 2020 14:14
@aklajnert
Copy link
Contributor Author

@Mikhail-Zex - I've just rebased the changes to the new branch, should be good to go

@ghost
Copy link

ghost commented Dec 19, 2020

Cool.

@aklajnert aklajnert changed the base branch from next-rel-dev to master December 28, 2020 09:51
@aklajnert aklajnert changed the base branch from master to dev December 29, 2020 17:26
@ghost ghost merged commit c03873f into spotDL:dev Dec 31, 2020
aklajnert added a commit to aklajnert/spotify-downloader that referenced this pull request Jan 6, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants