djbook-ru / djbookru

It is Django powered site for russian translation of DjangoBook
http://djbook.ru/
86 stars 35 forks source link

Рефакторинг юниттестов в приложении forum #277

Closed Alkalit closed 8 years ago

Alkalit commented 8 years ago

Файл с тестами разделен для моделей и вьюх. Сами тесты разделены по классам, в зависимости от "юнитов" которые они тестируют. Каждый тестовый случай выделен в отдельный метод.

Alerion commented 8 years ago

Вы порефакторили тесты, которые я писал во время рефакторинга. Не лучшая идея. Не пойму как я это все буду ревьювить и что в них было не так.

Alerion commented 8 years ago

Тесты были разделены не по тестовому случаю, а по определенным сценариям. Не нужно их разделять.

Alkalit commented 8 years ago

Ну вот не понравилось мне как они были написаны. Я не увидел разделения на сценарии, за нашел нарушения изоляции. Например в этой строке: https://github.com/djbook-ru/djbookru/blob/refactoring/src/forum/tests/tests.py#L921 изменяется состояние объекта, который потом используется на протяжении более чем десятка ассертов. Т.е. это может оказать влияние на утверждения к которым это действие не относится.

Плюс эти "сценарии" слишком большие и их трудно читать.

Alerion commented 8 years ago

В том и суть не создавать идеальные условия. Есть набор топиков, набор пользователей и все должно работать вместе, а не в каких-то идельных условиях для каждого случаю. "Удалить пользователя" может удалять вообще всю БД, и при проверке, что пользователя реально не стало, тест будет отлично проходить.

Ну и не известно на сколько вас хватит. Вы пришлете пару пулриквестов с кодом, которым вам нравится, потом пропадете навсегда, а нам с этим кодом жить. Одно дело, если вы в своем стиле новые тесты напишете, а другое, когда вы просто переделали что уже есть, как вам нравится.

Alkalit commented 8 years ago

Юниттесты проверяют только "единицы кода". То о чем вы говорите это уже скорее функциональное тестирование. Это уже нужен селениум или аналогичный движок, чтобы тестировать сайт как нечто единое целое. А про удаление БД - это повод написать еще один краткий тесткейс, а не городить портянки.

Ну это странная позиция для опенсорса. Извините, а если вас не хватит? Как быть тому кто захочет поддерживать сайт? Думаю в моем случае достаточно будет посмотреть на уже написанный код и продолжить в том же духе.

Скажем, если я пообещаю покрыть тестами как минимум приложение forum?

Alerion commented 8 years ago

Цели писать тесты по всем канонам у нас нет. Они не идеальны, но покрывают основные моменты. И селениума у нас никогда не будет, исходим из этой позиции. Также не забываем, что этот проект делается в свободное время, и большая часть кода написана мной, все остальное Русланом. Цель иметь максимально простой и понятный для нас код, чтобы с ним можно было работать в максимально позднее время и в максимально неработоспособном состоянии, так делался весь этот проект.

А про удаление БД - это повод написать еще один краткий тесткейс, а не городить портянки.

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

Ну это странная позиция для опенсорса. Извините, а если вас не хватит? Как быть тому кто захочет поддерживать сайт?

Это не опенсорс, это бесплатный софт. У нас нет времени решать проблему поддержки сайта после нас. Сайт самый простой какой может быть. Не пойму что еще мы должны сделать, чтобы упростить поддержку. Писать код там, чтобы всем он нравился?

Думаю в моем случае достаточно будет посмотреть на уже написанный код и продолжить в том же духе.

Так а почему вы тогда решили переформатировать существующий код, а не писать в том же духе?

Скажем, если я пообещаю покрыть тестами как минимум приложение forum?

Есть приложения без тестов. В чем смысл переписывать существующие?

Alerion commented 8 years ago

Юниттесты проверяют только "единицы кода".

Если лезут в БД, разве это уже не интеграционные тесты? Юниттесты, на сколько помню, никуда не должны лезть.

Alkalit commented 8 years ago

Вы предлагаете написать N юнитов, затем еще по юниту на каждую комбинацию.

Ну да. Конкретный кейс на конкретную ситуацию. Например, если вы ожидаете что у вас при удалении пользователя может грохнуться БД, то надо написать тесткейс где имитируется подобное поведение. А как вы предлагаете тестировать, например, кусок кода с if-ами?

Писать код там, чтобы всем он нравился?

Тут подходит золотое правило "критикуешь - предлагай". Мне не понравился код тестов и я предложил альтернативу.

а не писать в том же духе

Потому что я могу написать лучше.

это уже не интеграционные тесты?

Этот вопрос выходит за рамки пуллреквеста. Скажу только что эти термины можно использовать как взаимозаменяемые.

Alerion commented 8 years ago

Ну да. Конкретный кейс на конкретную ситуацию. Например, если вы ожидаете что у вас при удалении пользователя может грохнуться БД, то надо написать тесткейс где имитируется подобное поведение.

Ну вот я ожидаю, что одно действие может влиять на другое. Добавил все в один тест, чтобы повысить вероятность словить это. Вы же разнесли это все по отдельным методам. Вы говорите красиво, а в коде ничего не поменялось, кроме его вида.

А как вы предлагаете тестировать, например, кусок кода с if-ами?

Это тут при чем? Я ничего вам не собираюсь предлагать или учить. Пишите как хотите.

Потому что я могу написать лучше.

По каким объективным критериям? Пока только теоретические рассуждения как должны выглядеть тесты, и вам не нравится мой подход.

Скажу только что эти термины можно использовать как взаимозаменяемые.

Это вы про юниттесты и интеграционные?

Исправляйте какие-то объективные моменты, например, исправить Deprecated Warning, или добавить тесты, где их еще нет.

sergkarpenko commented 8 years ago

Запилите лучше что-то из беклога и прикрутите туда тесты как нравится :) Такое Дима с Русланом одобрят.

Alkalit commented 8 years ago

Проще найти проект где действительно нужна помощь.

RaD commented 8 years ago

Удачи.