Improve tests - #80
Conversation
3057ece to
398762d
Compare
|
Thanks for the review @curquiza , I included all your changes. I will re-read all the files again to see if there are some simple mistakes that I haven't seen yet! |
There was a problem hiding this comment.
Requests overview
- Could you remove all the in MeiliSearch that are added in some comments? Those are unnecessary
- Should comment have whitespace before and after the content of the comment? In which case could you do either one or the other?
- Could you, on the tests you did not move (so I could not suggest modifications), change
test an API call to ...toTests getting/creating ... - Some search tests are missing
This is for consistency between every comment and because An API call is not useful information.
I tried to update as many as I could.
Example
"""Tests an API call to get an index's info in MeiliSearch with a wrong UID"""
Becomes
"""Tests getting an index's info with a wrong UID"""
bidoubiwa
left a comment
bidoubiwa
left a comment
There was a problem hiding this comment.
- Could you remove all the
in MeiliSearchthat are added in some comments? Those are unnecessary - Should comment have whitespace before and after the content of the comment? In which case could you do either one or the other?
| response = self.index.search('How to Train Your Dragon') | ||
| assert isinstance(response, object) | ||
| assert response['hits'][0]['id'] == '166428' | ||
|
|
There was a problem hiding this comment.
There should be three additionals tests that tests the following values during a search.
On the query parameters that accepts either an asterix, a string, or an array or string
- One tests that tries query parameters with a "*" value
- One tests that tries query parameters with a simple string value
- One tests that tries query parameters with an array of string as value
Better explaination was made in the PHP sdk: meilisearch/meilisearch-php#19 (comment)
This explanation does not add the tests on a simple string, as it is potentially breaking I would prefer that it is also tested.
The reason I mentioned those tests is because of the hight probability that the codes act in a certain way depending on the type of data it expects. For example the JS library crashed on a simple string before those tests were done.
There was a problem hiding this comment.
The first two tests were added. The third one actually uncovered a bug, detailed in #85. Test was removed and will be integrated when that issue is solved.
3 participants
This is a complete review / refacto of the whole module tests.
Every file and method has been checked, and I did my best to complete the very hard work that had already been done in this test set.
The main goal was to go a little bit deeper in testing every method, and I feel confident that this test set is enough for a guarantee of the quality of code we do and maintain.
Tests have also been classified in directories, corresponding to the package structure, so it is easier to navigate inside of it.
I know this is going to be hard to review, as it is a big fat Pull Request 😄
I want to say congrats to those who have worked on it before, a huge work was done on this before I came (cheers @bidoubiwa)
Closes #53