Refactor of JSON structure in response of get_by_code #4003
Conversation
There was a problem hiding this comment.
Zostawiłem kilka komentarzy. Do tego trzeba poprawić formatowanie (statyczna analiza kodu się wywaliła na CI (https://github.com/KlubJagiellonski/pola-backend/actions/runs/10523617987/job/29196796842?pr=4003)
Jeśli chodzi o inne czeki to zrobiłem fixa w tym PRze #4005, po zmergowaniu trzeba bedzie domergować/zrebasować developa.
pola/rpc_api/tests/test_views_v3.py
Outdated
| @@ -200,7 +199,6 @@ def test_should_return_200_when_one_comand_in_brand_and_product(self): | |||
| self.maxDiff = None | |||
| self.assertEqual( | |||
| { | |||
| 'all_company_brands': [{'logotype_url': None, 'name': p.brand.common_name}], | |||
There was a problem hiding this comment.
NAzwa testu sugeruje, że te brand jest tu. Proponuje dodać marki do c1.
There was a problem hiding this comment.
Hmmm nie do końca wiem o co chodzi.
Dodałem brand w kodzie testu, sprawdźcie czo to o to chodziło czy coś innego.
Prośba o info to dorobię co trzeba.
There was a problem hiding this comment.
Skoro mieliśmy all_company_brands to spodziewałbym się tutaj assertu, który będzie sprawdzał czy brands się pojawi. Ten test przechodzi po dodaniu brandu do modelu bez zmiany assertu?
There was a problem hiding this comment.
testy przechodzą, ale pamiętając zmianę struktury response, element brand (nawet jeśli jako pusta lista) musiałby istnieć wew elementu "comapnies", a tego też tutaj nie ma.
There was a problem hiding this comment.
A jesteśmy wstanie go tak zmienić by ten brand był też w assercie? Ten test traci trochę sens w obecnej formie.
There was a problem hiding this comment.
powalczymy, muszę się wgryźć w kod, tutaj trzeba dodać nie tylko brands do responce , ale całą zawartość 'companies'
There was a problem hiding this comment.
@WezSieTato
Zrobiłem małą analizę w wgryzłem się bardzie w temat. I wygląda to na trochę grubszą sprawę, niestety.
- test > test_should_return_200_when_one_comand_in_brand_and_product
- dodanie 'brands', wewnątrz listy 'companies' jest obsługiwane w metodzie 'get_result_from_code' pod warunkiem że parametr multiple_company_supported=True, inaczej nie jest to wspierane. -> to jest też zmieniane w obecnym PR
- domyślnie parametr multiple_company_supported=False
- aby przy obecnych zmianach do response była dodawana lista 'companies' parametr '' musi być True w
poniższej metodzie
- sęk w tym że mocno zmienia to odpowiedź z API - w efekcie zmianie ulegają dodatkowe 3 inne testy, poza testem 'test_should_return_200_when_one_comand_in_brand_and_product' nad którym pracowałem
Postaram się zająć tematem i zobaczyć czy nie ma jakiegoś obejścia, ale wstępnie wygląda to niestety na większą sprawę.
There was a problem hiding this comment.
Co z tym testem? Jeśli jest to zbyt skomplikowane i trudne w utrzymaniu, to może go usuniemy/wyłaczymy?
There was a problem hiding this comment.
Sprawdziłem, porównałem 2 podobne testy jakie mamy dla v3
'test_should_return_200_when_one_comand_in_brand_and_product' oraz 'test_should_return_200_when_polish_and_known_product'
- Różnica jest w danych wejściowych do testu - między dodaniem brandu do produktu i jego brakiem - i mimo tego wynik response jest podobny.
- mają niemal identyczna asercję jeśli chodzi o response.content. Różnica jest tylko między kluczem 'product_id': p.id vs p.pk
Wygląda na to że odpowiedź responce jest w zasadzie identyczna a testy sprawdzają tą samą ścieżkę - ale wnioskuję tylko po teście. Nie mamy tutaj sprawdzenia % pokrycia kodu testami. Ale dane wejściowe są inne.
Osobiście jestem za pozostawieniem testu, można zmienić nazwę na odzwierciedlającą rzeczywistość,
np: test_should_return_200_with_product_no_brand_info.
Wychodzę z założenia że usunąć test można zawsze, jeśli zacznie rzeczywiście przeszkadzać albo nie będzie potrzebny.
|
Could you please fix the conflicts @piemar1? 🙏 |
|
Rebased to master, but need to fix one error in test |
|
@WezSieTato |
No description provided.