Implementing a proper testing routine #24
Conversation
|
This pull request is just for reference, obviously, it's not complete yet. I just wanted to let the maintainers know about it, and also you will be able to track the commits from here, so that'd be handy I presume. Cheers! |
There was a problem hiding this comment.
Thanks for the PR!
I've added some comments that are global to all the tests functions.
Basically:
- mock the file.
- not sure what to do with the calls to the server, probably keep them and make this an integration test at least for now.
- the assertion of types doesn't make that much sense, I would prefer to test for proper values.
I am also not sure how these tests are going to work when you need the user to be logged in. Are we sure that the order is always correct? I would try to avoid this problem logging the user whenever is needed.
Thanks for the changes again, good job! 👍
|
|
||
| @staticmethod | ||
| @pytest.fixture | ||
| def os(): |
There was a problem hiding this comment.
Maybe call it ost to don't mistake it with the package os?
| @staticmethod | ||
| @pytest.fixture | ||
| def os(): | ||
| from pythonopensubtitles.opensubtitles import OpenSubtitles |
There was a problem hiding this comment.
You are using this fixture in all the methods, maybe you could just import this at the beginning of the file.
| @pytest.fixture | ||
| def file(data): | ||
| from pythonopensubtitles.utils import File | ||
| return File(path.join(data.path, data.video)) |
There was a problem hiding this comment.
Maybe you could add a temporal path to this file?
| @pytest.fixture | ||
| def file(data): | ||
| from pythonopensubtitles.utils import File | ||
| return File(path.join(data.path, data.video)) |
There was a problem hiding this comment.
I still don-t understand how this is going to work if the file doesn't exist. I couldn-t see the mock anywhere and call something like file.get_hash() will probably return an IOError. What am I missing? 🤔
| token = os.login('[email protected]', 'badpassword') | ||
| assert token is None | ||
| token = os.login(data.username, data.password) | ||
| assert type(token) == str |
There was a problem hiding this comment.
Those assertions should probably be better checking for the correct value. I know they were originally like this, but it doesn't make that much sense, an empty string is type str as well.
They are a lot like these, it would be nice to change all them.
| # Creating a video hash | ||
| @staticmethod | ||
| def test_video_hash(file): | ||
| hash = file.get_hash() |
There was a problem hiding this comment.
This is what I mean that I don't fully understand how it will get a value without a real file.
| # Login | ||
| @staticmethod | ||
| def test_login(data, os): | ||
| token = os.login('[email protected]', 'badpassword') |
There was a problem hiding this comment.
This will try to make a call, it shouldn't do real calls in a unit test. However, maybe we should just focus in integration since this is just a client for an API.
What do you think?
| from pythonopensubtitles.utils import File | ||
| return File(path.join(data.path, data.video)) | ||
|
|
||
| # Login |
There was a problem hiding this comment.
I wouldn't add these comments, it is easily understandable from the name of the function.
|
Closing this PR in favour of 2c8a6ca Thanks for it anyway @skulltech! |
Moving the test cases already in
README.mdtopytest, also possibly expanding them.According to #18