-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactor of JSON structure in response of get_by_code #4003
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAzwa testu sugeruje, że te brand jest tu. Proponuje dodać marki do c1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zmiana nazwy brzmi ok :)
Could you please fix the conflicts @piemar1? 🙏 |
Rebased to master, but need to fix one error in test |
@WezSieTato |
No description provided.