Product API refactor #90
Conversation
…filters-product-list' into feature/api-filters-sorting
…uct-list' into feature/api-filters-sorting
|
|
||
|
|
||
| def test_pagination_size_in_tests(): |
There was a problem hiding this comment.
Please remove fixture since we moved to settings_tests.
There was a problem hiding this comment.
I want to know if settings_tests are changed. So we know the cause of test's failures
| @pytest.mark.django_db | ||
| def test_product_detail(tv_product): | ||
| url = reverse('products-api-detail', kwargs={'pk': tv_product.id}) | ||
| response = APIClient().get(url) |
There was a problem hiding this comment.
After this change, this test is only checking response code, not details. Please bring back ID and other details tests - the one removed. Or use a function to test product details.
Testing just response code is not enough.
Actually, this test_product_detail_with_images test, after changing to not authenticated user, would make the current one test not needed no? They both check the same thing.
There was a problem hiding this comment.
Assert added. Also test_product_detail_with_images doesn't need authentication any more.
I still think it may be beneficial to have separate tests for image related behavior and general object retrieve.
Especially since separate tests are already there. They don't overlap.
Dshop/conftest.py
Outdated
| return APIClient() | ||
|
|
||
| @pytest.fixture | ||
| def api_client_authed(): |
There was a problem hiding this comment.
Could you please change to full name? api_client_authenticated? It makes it more readable.
Also, the previous fixture, authenticated_api_client - why it is not used anymore? Seemed ok.
There was a problem hiding this comment.
authenticated_api_client caused integrity error - unique constraint violated in this place.
With my fixture test passed.
Also my fixture is simpler - no playing with Token
There was a problem hiding this comment.
Also authenticated_api_client (old) is not located in a good place. It's hard to share it across different apps
There was a problem hiding this comment.
Regarding fixtures --> there is little bit of mess here.
There are knop-k's fixtures:
create_active_product
create_inactive_product
mkrzminski's:
tv_product
my:
inactive_product
Also there are:
api_client (bare) in Dshop/conftest.py
api_client (authed)Dshop/apps/products_catalogue/tests/conftest.py
yours
products in Dshop/apps/products_catalogue/tests/conftest.py
mine:
ten_tv_products in Dshop/conftest.py
There was a problem hiding this comment.
We need ALWAYS to check existing fixtures before creating new ones
There was a problem hiding this comment.
ok, so what is your final response? I see a lot of text, but I don't see anything actionable.
There was a problem hiding this comment.
My reponse is:
I'll change name.
I'd like to remove old authenticated_api_client fixture
I'm asking what to do about other duplicated fixtures (probably in new PR)
There was a problem hiding this comment.
Remove not needed code, new pr is ok.
…i_client_authenticated
Pagination for products ViewSet has been added and tested.
ProductViewSet is now read-only - behavior has been tested.
pytest.ini settings force pagination with size 5 right now.
Collection of fixtures: for login and for data has been added - some were improved.