Skip to content

First two exercise for review#1

Closed
NikitaTiv wants to merge 5 commits intolearnpythonru:mainfrom
NikitaTiv:test_review
Closed

First two exercise for review#1
NikitaTiv wants to merge 5 commits intolearnpythonru:mainfrom
NikitaTiv:test_review

Conversation

@NikitaTiv
Copy link
Copy Markdown

No description provided.



@pytest.fixture
def max_string():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Название не супер – это не "максимальная строка", а просто длинная строка.



@pytest.fixture
def bank_card():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Фикстура называется "банковская карта", а возвращает внезапно список из нескольких карт.

def expense():
return Expense(
amount=Decimal('1020'),
card=BankCard(last_digits='3326', owner='Nikita T.'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сделать бы фикстуру одной карты и переиспользовать её тут.

date.month,
date.day,
16,
48
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Запятой не хватает.



@pytest.fixture
def date_today():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Префикс date_ можно удалить: и так понятно, что today – это дата.



@pytest.fixture
def date_tomorrow(date_today):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут префикс тоже лишний.

[
('hh.ru', 'applicant/negotiations', {
'hhtmFrom': 'employer', 'hhtmFromLabel': 'header'
}, 'hh.ru/applicant/negotiations?hhtmFrom=employer&hhtmFromLabel=header'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут форматирование поломалось. Если такая последовательность не умещается в одну строку, разбивай в длинном варианте, чтобы у каждого элемента была своя строка:

(
    'hh.ru',
    'applicant/negotiations',
    {'hhtmFrom': 'employer', 'hhtmFromLabel': 'header'},
    'hh.ru/applicant/negotiations?hhtmFrom=employer&hhtmFromLabel=header',
),

'date_str, time_str, expected_result',
[
('today', '16 : 48', pytest.lazy_fixture('date_today')),
('tomorrow', '16 : 48', pytest.lazy_fixture('date_tomorrow')),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут у тебя получается, что фикстуры намертво завязаны на константы в этом параметрайзе: поменяешь одно и не поменяешь другое – тест сломается.

Это хороший признак того, что фикстура лишняя. Фигани объекты datetime.dateme прямо сюда, а фикстуры удали. Получится читаемее и проще.

@NikitaTiv NikitaTiv closed this Feb 22, 2023
@NikitaTiv NikitaTiv reopened this Feb 22, 2023
@NikitaTiv NikitaTiv closed this Feb 22, 2023
@NikitaTiv NikitaTiv reopened this Feb 22, 2023


@pytest.fixture
def list_bank_cards():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А тут list_ лишний. Понятно, что если bank_cards, то их несколько :)

Все названия нужно стараться сделать как можно более подробными, но без лишней/дублирующейся информации.

@pytest.fixture
def list_bank_cards():
return [
BankCard(last_digits='3326', owner='Nikita T.'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажется, тут можно переиспользовать фикстуру bank_card


def test_compose_datetime_from():
pass
date = datetime.date.today() + datetime.timedelta(days=1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну вот today и tomorrow я бы в фикстуры вынес – они универсальные и много где пригождаются обычно.


@pytest.fixture
def today():
today = datetime.date.today()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажись эта переменная лишняя, можно сразу ретурнить.


@pytest.fixture
def yesterday():
yesterday = datetime.date.today() + datetime.timedelta(days=1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут бы тоже без промежуточной переменной и фикстуру тудей можно переиспользовать для красоты и простоты.

[
(pytest.lazy_fixture('male_verb'), pytest.lazy_fixture('female_verb'), 'male', pytest.lazy_fixture('male_verb')),
(pytest.lazy_fixture('male_verb'), pytest.lazy_fixture('female_verb'), 'female', pytest.lazy_fixture('female_verb')),
]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут есть лишние параметры: первый, например, всегда одинаковый, второй тоже. Убери их из параметрайза.

И фикстуры я бы эти тоже удалил: они завязаны на этот тест и не очень универсальные, поэтому просто захардкодь слова тут и будет нормас.

[
('hh.ru', 'applicant/negotiations',
{'hhtmFrom': 'employer', 'hhtmFromLabel': 'header'},
'hh.ru/applicant/negotiations?hhtmFrom=employer&hhtmFromLabel=header'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Форматирование всё ещё поломано: не видно, где тупл начинается, где заканчивается и что в нём есть. Я в прошлом комменте приводил пример, как правильно его можно отформатировать.

def test_compose_datetime_from():
pass
def test_compose_datetime_from_today(today):
assert compose_datetime_from('today', '16 : 48') == datetime.datetime(today.year, today.month, today.day, 16, 48,)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут после последнего аргумента запятая лишняя, в следующем тесте тоже.

@NikitaTiv NikitaTiv closed this Feb 25, 2023
@NikitaTiv NikitaTiv deleted the test_review branch February 25, 2023 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants