Комментарии 50
Когда вижу комментарии в коде, то плачу от счастья. За долгие годы программирования у меня сложилось впечатление, что другие программисты категорически не любят комментировать. Прямо как в том анекдоте про пустые листы в качестве плакатов/листовок: "и так всё понятно!"
В ходе работы над задачей и правда может быть "всё очевидно": автор ведь погрузился в тем и прокопал её как следует. Порой вплоть до того, что ему уже сложно понять, что в изменении может быть непонятно другим. Эффект "проклятия знания".
Помочь тут может, к примеру, формальная договорённость о доках на уровне команды. Например "публичный api обязательно задокументировать". Для надёжности, подкрепляем правило автоматической проверкой. Теперь уже автору потребуется хотя бы написать плейсхолдеры, и дальше уже можно включить мозг и заполнить их чем-то осмысленным.
С комментариями внутри кода сложнее. Обычно нет возможности сформулировать формальные признаки, когда они нужны (чтобы автоматизировать проверку). Помогает порой разве что просьба непосредственно на ревью: "тут место мутное, оставь чуток коментов, плиз".
Тесты надо писать, а не комментарии. Тесты - это живая и всегда актуальная документация, а комментарии - надписи на заборе.
Не соглашусь. Одно другого не отменяет полностью. Тесты и комментарии могут дополнять друг друга при грамотном использовании.
В комментариях могут, например, содержаться упоминания о важных краевых случаях, которые привели к выбору конкретно этого решения, и которые не так-то просто покрыть тестами.
В комментариях могут, например, содержаться упоминания о важных краевых случаях, которые привели к выбору конкретно этого решения, и которые не так-то просто покрыть тестами.
Увидев такой комментарий рука автоматически тянется к пистолету единственное что следует сделать - заблокировать PR и написать - немедленно покройте ВАЖНЫЕ краевые случаи тестами. Поскольку краевые случаи - это чуть ли не единственное, что требует обязательного покрытия тестами. Особенно, важные краевые случаи. "Не так просто" - это какая-то нелепая отмазка.
Гм. Получается, такой комментарий - довольно полезная штука. Но не в том смысле в котором вы думали. Сразу помечает в каком месте и какого "пограммиста" бить канделябром. :)
О, ну так поделитесь покрытием в собственных проектах, а мы посмотрим и посмеёмся. Как насчёт тестов, которые, к примеру:
Настолько длительные, что в разы увеличивают время поставки, или
Требуют специфических внешних условий, которые трудно воспроизвести чисто физически (требуется особое железо, к примеру), или
Требуют для написания радикальной переделки архитектуры проекта
Процесс разработки - это практически всегда путь из сплошных компромиссов между "сделать по-правильному" и "получить результат в обозримое время". В каких-то случаях это может значить и то, что порой лучше написать комментарий, чем автотест. Просто потому что финансовые, вычислительные и умственные ресурсы конечны и ограничены. А раздавать виртуальные звездюли любой дурак сможет, конечно.
О, ну так поделитесь покрытием в собственных проектах
Сперва добейся? К вашему большому огорчению, я-то как раз добился. А вот вам следовало бы знать, что перед тем как вот так хамски чего-то требовать, следует предъявить собственный код. Ну, же, похвастайтесь своим нетестируемым кодом с комментариями к важным краевым случаям!
Как насчёт тестов, которые, к примеру:
Вы, должно быть, удивитесь, но с ними всё в порядке.
Настолько длительные, что в разы увеличивают время поставки
Это в большинстве случаев всего лишь означает, что на предыдущих этапах не нашлось ни одного ревьюера с яйцами, который заблокировал бы PRы с говнокодом. В меньшинстве случаев такие тесты выносятся в отдельный бандл и запускаются в соответствующем месте пайплана. Не знали? Теперь знайте.
Требуют специфических внешних условий, которые трудно воспроизвести чисто физически (требуется особое железо, к примеру), или
И поэтому вы не тестируете такой код? Мило. Это вы написали код для Луны-25?
Требуют для написания радикальной переделки архитектуры проекта
А вот это абсолютно и безоговорочно означает, что на предыдущих этапах не нашлось ни одного ревьюера с яйцами, который заблокировал бы PRы с говнокодом. Но оправдание - я не могу потестировать говнокод, потому что я спроектировал нетестируемую говноархитектуру, конечно, зачётное!
Процесс разработки - это практически всегда путь из сплошных компромиссов между "сделать по-правильному" и "получить результат в обозримое время".
Никак нет. Это означает, что вы жестоко кидаете своих заказчиков, поставляя им код о котором никто не может сказать работает ли он. Причём в ВАЖНЫХ краевых случаях. Например, в циклограмме своевременного отключения тормозного двигателя посадочного лунного модуля. Ну, "а чо такого" - там же требуется соблюдение специфических внешних условий, которые трудно воспроизвести чисто физически (требуется особое железо, к примеру) - зачем это тестировать? Напишем вместо теста комментарий, херак - и в продакшн. И та-а-ак сойдёт!..
Как ни крути, но ваш комментарий является классическим примером токсичности на CodeReview.
Под токсичностью я понимаю критику (перепиши этот говно-код), без образа правильного результата.
Для любой сколь угодно запутанной лапши можно применить не сложный алгоритм:
1) извлечь часть бизнес-логики в отдельный модуль (функцию / класс / UI- компонент)
2) применить принцип инверсии зависимостей - все необходимые зависимости передать через параметры, не ссылаться на зависимости из внешнего лексического окружения (исключить прямые импорты других файлов)
3) применить принцип разделения интерфейсов, не передавать избыточные зависимости, ограничить контракт только необходимым.
После этих нехитрых действий функциональность легко покрыть модульными тестами, передав вместо зависимостей заглушки (jest.fn()).
Причём пункты 1-3 значительно дешевле писать на этапе разработки модуля, но учишься им как правило переписывая свою лапшу.
Как ни крути, но ваш комментарий является классическим примером токсичности на CodeReview.
Так это и не кодревью ))
Под токсичностью я понимаю критику (перепиши этот говно-код), без образа правильного результата.
На ревью, если человек ниже меня квалификацией или я вижу, что просто не понял/не знал о какой-то особенности, я и сам, как возможный вариант, кусок кода ему предложу как я бы его сделал. Выгода тут обоюдная - когда я буду что-то в окружении этого кода менять, я буду знать, что как минимум важные кейсы покрыты тестами, а не комментариями. Но тут пассажир берётся учить других что нужно а) писать комментарии ВМЕСТО тестов, на ВАЖНЫЕ кейсы, б) не писать тестов вообще, если они выполняются долго, в) требуют стороннего железа (моки-симуляторы? не, не слышали!) потому, что он г) написал нетестируемый в принципе код и потому, что он д) видел в жизни много плохих тестов. Ну, а что тут ещё скажешь? Он ведь считает, что прав.
Для любой сколь угодно запутанной лапши можно применить не сложный алгоритм:
Конечно, я поэтому и сказал, что у этого гражданина не нашлось никого на предыдущем этапе, кто бы рассказал про этот несложный алгоритм. Теперь, видимо, уже поздно.
Мне-то как раз несложно привести пример собственного кода. Берём пет-проект, который пишется левой пяткой в редкое свободное время, и смотрим на покрытие кода тестами:
Видим чёткое разграничение: все модули покрыты на 90+% за исключением одного, который покрыт на 0%. Почему? Потому что в нём сконцентрирован весь код, касающийся десктопного GUI на Qt. Я не собирался и не собираюсь писать десктопные тесты в своём проекте, потому что мне принципиально важна скорость разработки и стабильность запускаемых тестов. Текущий набор тестов, который обеспечивает покрытие действительно важной логики, отрабатывает на моей машине за 8 секунд и содержит ровно 0 флакающих тестов. Именно такой выбор между "что тестировать" и "что не тестировать" позволял мне держать zero bug policy с самого старта. И даже сейчас, когда я порой захожу в код на 15 минут после 2-месячного перерыва, я могу без опаски проводить любые доработки.
Хотелось бы напомнить, что в каждом софтверном проекте есть свои собственные соотношения между рисками, сроками и возможностями нести расходы. Если в одном проекте нам жизненно важно тестировать абсолютно все краевые варианты, это не значит, что это жизненно важно абсолютно во всех проектах (и наоборот). Если в одном проекте нам жизненно важно завершить работу к определённой дате, это не значит, что каждый другой проект должен иметь дедлайны (и наоборот). Если в одном проекте мы можем себе позволить любые дополнительные траты (больше железа, больше времени на сборку и тестирование, больше разработчиков и т.п.), это не значит, что так можно позволить вести себя в любой ситуации (и наоборот).
Автотесты - это полезный и важный инструмент разработки. Точно так же, как и code review, статический анализ, и т.п. Но это должен быть инструмент, а не объект поклонения. Всегда надо понимать, какую цену приходится платить за его использование.
Бывают ли баги в непокрытом тестами коде из примера выше? Да, случаются раз в год-другой. Сколько времени я трачу на то, чтобы найти и исправить их? Обычно, несколько минут - потому что в гуёвом коде практически нет никакой бизнес-логики. Поэтому я считаю отсутствие гуёвых автотестов в этом проекте вполне оправданным. Это мой пет-проект, где я никому ничего не должен. Стал бы я покрывать GUI тестами, если бы писал коммерческий проект? Практически наверняка да.
Вопрос: понимаете ли Вы наличие этих соотношений и их зависимость от условий проекта? Похоже, что нет. Это лишь делает Ваши аргументы смешными в глазах тех, у кого опыт более широкий и разнообразный.
Тесты не панацея ещё и потому, что люди нередко пишут их через одно место. Чего я только не видел за годы!
Пишем тесты на тривиальный код типа DTO, но не на ключевую бизнес-логику, ибо "чот сложна"
Пишем тесты только на самом верхнем уровне (ещё и через selenium), но не на компоненты, по той же причине
Пишем тесты на логику, но густо обмазываем их моками "чтобы зависимости не мешали". Обновлением моков при смене поведения реального "замоканного" кода, конечно же, не занимаемся
Пишем тесты без ассертов. Нуачо, зато всегда зелёные!
Создаём в тестах 100500 слоёв абстракций, чтобы типа писать меньше. Вот только при разборе падений хрен у тебя получается две программы, которые выдают разный результат (прод-код и тест-код), так кто же из них прав?
В таких условиях даже минимальным комментариям будешь рад, чистая правда.
люди нередко пишут их через одно место
В таких условиях даже минимальным комментариям будешь рад
Ну слава б-гу, что комментарии пишут всегда на совесть, не через одно место.
Сарказм неуместен. Очевидно, что в комментах шлака тоже нередко полно. Однако следствием из этого факта должно быть "учитесь сами и учите других писать более качественные и уместные комменты", а не "комменты не нужны, ибо есть тесты".
Может лучше учиться писать более качественный код?! Разбивать на функции, называть их понятным образом, давать вменяемые имена переменным?
Да, бывают случаи, когда в коде какой-то лютый костыль/хак и стоит оставить комментарий. Но если в вашем коде такое встречается часто, может стоит вернуться к пункту 1...
Может лучше учиться писать более качественный код?! Разбивать на функции, называть их понятным образом, давать вменяемые имена переменным?
Опять же, не вижу здесь противопоставления "или то, или это", как и в случае с тестами. Умение понимать, что в определённом месте надо поставить определённый комментарий, - это такой же составной элемент общего умения писать хороший код, как и всё остальное, что Вы перечислили.
Кодь ревью полу бесполезная практика. Я, например, любой код большее 1000 строк или 10 файлов одобряю не смотрев. Линтер за меня проверил качество кода, тестировщик провел тестирование, если задача работает то мне глубоко плевать на код внутри таких партянок для ревью.
Код ревью полу бесполезная практика.
Только хотел это же написать. Абсолютно безрезультатная трата кучи времени на очередной бессмысленный холивар. Принесу вот я на ревью полный бред, мне напишут, что это бред, а я напишу в ответ, что у меня еще дедушка так делал, поэтому мне так нравится - и хер кто мне что сделает - закончится просто тем, что весь бред так и уйдет в общую ветку, только время будет зря потрачено.
одобряю не смотрев
Скажу по секрету, давным-давно уже делаю точно так же :))
Надеюсь, что вы и работаете вместе!
Если мне на ревью принесут такой код и на обоснованные замечания ответят в том же стиле, то я такой код в прод просто не пущу.
UPD: Какой там прод, даже на тестирование не пущу. Ибо время тестировщиков можно потратить более полезным образом.
Буду раз за разом отправлять с небольшими изменениями - не мытьем, так катанием. Архитектора у нас с вами в команде нет, начальник наш такой же пофигист, как и я. Рано или поздно вы сдадитесь, а в следующий раз так и вообще связываться не станете - поставите молча "approve" и всё. Шутка, конечно, но как говорится, в каждой шутке...
Помните старый анекдот: "Можно ли сделать защиту от дурака ? Да, но только от неизобретательного!". С практиками разработки - все то же самое. Мы предполагаем, что команда в целом (и каждый ее участник в отдельности) - хотят писать хороший, читаемый и надежный код. Однако, все они люди - и поэтому иногда случайно допускают ошибки. В этом смысле, наши код-ревью заменяют чтение контрольных карт в авиации. И да, разумеется, если пилот сознательно хочет чтобы двигатель встал в воздухе - никакая контрольная карта от этого не спасет.
Что делать если один из участников команды решил перейти на сторону зла и упорно пихает в кодовую базу говнокод не реагируя на замечания код-ревью? Ну, сначала - подойти поговорить. Может у человека что-то нехорошее в жизни случилось, и ему бы недельку отпуска... Не помогает - эскалировать, чтобы с ним поговорили специально обученные люди из HR и линейного менеджмента. Если же гм, индивид реально обратился ко злу и упорствует - поплакать, открыть вакансию, запустить succession process, уволить...
В ситуации, когда в коде есть проблема, на первый раз с небольшими изменениями я заверну ревью на доработку, на второй найду тим-лида и попрошу присоединится к ревью, ибо ситуация требует эскалации. Если это не поможет, то будет инициирован разговор 1-to-1 для объяснения, почему так делать не стоит. Если всё ещё не понятно, то заберу на себя задачу и доработаю (первый и последний раз с данным разработчиком). При повторении ситуации с другой задачей, буду поднимать вопрос о том что делать с таким разработчиком, с описанием всех мер, которые были предприняты, для того чтобы проблема не возникала.
Это касается ситуаций, когда в коде разработчика очевидно есть проблема. Вопрос о форматировании устраняется автоматизированными средствами и Вы просто не сможете залить неотформатированный как требуется код в CVS.
P.S> Если разработчик отправляет откровенный бред на код-ревью своим коллегам, а потом ещё и игнорирует обоснованные замечания, то он не уважает ни себя ни своих коллег.
1. Перед отправкой кода на ревью обязательно проведите все автоматические проверки (тесты, линтеры и т.д.)
Цель ревью никакого отношения к автоматическим проверкам не имеет. Она в том, чтобы показать код человеку, который оценивает его с человеческой точки зрения.
И делать это можно на любом этапе, когда вам двоим удобно. Например когда код вообще еще сырой, и даже не компилируется - чтобы оценить общую структуру, идеи, алгоритмы и т.п. - то что можно примерно считать архитектурой, но не в картинках, а уже в виде кода.
При этом размер pull request может быть вообще любым, хотя завершенным и небольшим конечно лучше, как правило. Но когда изменения бывают реально большими, разбивать их на мелкие части - не самое удачное решение.
Что же до автоматических проверок, то в правильно налаженном процессе при создании pull request они уже как правило будут автоматически запущены, поэтому данный совет никакого смысла не имеет. Наладьте процесс, и забудьте про эту ерунду.
В двух словах - все что тут написано, скорее всего верно для разработчиков начинающих. А если вы синьор, вы спокойно можете все эти правила нарушать, потому что вы знаете, что делаете. И зачем делаете. А если оба участника синьоры, то и подавно.
Вплоть до того, что вообще ревью не проводить. Да, так можно было :)
В двух словах - все что тут написано, скорее всего верно для разработчиков начинающих.
Что-либо, что вообще где-либо и когда-либо написано верно для разработчиков начинающих. Пока ты начинающий ты читаешь кучу всего этого про то "как оно должно быть", а потом все эти твои знания очень быстро разбиваются об то "как оно есть", а еще позже ты вообще понимаешь, что современное искусство разработки это просто-напросто искусство выдать перед заказчиком кучу гамнища за нормальный продукт, и тебе становится уже без разницы на всякие линтеры, проверки, ревью кода и прочее. Какие нахер линтеры? У меня код, где в каждом проекте в среднем по одному-два "Compiler warnings" на каждые десяток строчек кода (специально как-то взял и для любопытства подсчитал). Ну, вот, взял я когда-то один отдельный проект из пары десятков, от силы за полчаса поисправлял все гамно типа "Unused variable", "Possible null reference" и прочее, потом поставил для сборки флаг "Treat warnings as errors". Ну и что. Следующий же коммиттер наделал еще два десятка подобных warnings, а, поскольку у него это теперь уже вообще не собиралось, то он вместо того чтобы эти предупреждения за пару минут исправить просто взял и обратно предупреждения отключил. И я тогда (в очередной раз) решил, что ну его вообще в хер - заказчик деньги работодателю платит, работодатель деньги мне платит, а остальное мне вообще до ****ы и дверцы.
Ну да. Написали немного эмоционально, но по большому счету... Я тоже на днях смотрел, что мне там эти линтеры нагенерили. Вот представьте себе, у вас в коде две строковые константы, сообщения об ошибках. И они одинаковые. СОНАР нам пишет - создайте переменную, устраните дублирование. И у этого всего уровень Critical. Што, блин? У нас на уровень бага критикал можно выпустить релиз, по упрощенной процедуре. Потому что критикал - это когда что-то не работает, и надо быстро починять. Но когда вот эта фигня получает такой уровень - ну так и отношение к результатам работы линтера соответствующее.
СОНАР нам пишет - создайте переменную, устраните дублирование.
Ну так исправить это в коде - это ведь даже не минута, а считанные секунды дела. Просто всем на это наплевать - такой вот нынешний уровень: "Ну, подумаешь, у нас два десятка проектов и в каждом по нескольку сотен предупреждений при сборке - собирается ведь в конце концов."
А представьте, что вы делаете такое исправление (на самом деле это не секунды вовсе, потому что зачастую проект еще собрать надо). И ваши пользователи, когда они видят ваш очередной релиз, читают release notes, и видят там что? Видят они задачу, где написано: "Заменил две константы на переменную, теперь SONAR счастлив". Лично для меня это будет хороший повод данный релиз не внедрять, а подождать следующего, когда автор сделает что-то более существенное.
Ну т.е. это не совсем общий нынешний уровень, если угодно, это скорее влияние качества кода на том микроуровне, где работают сонары и линтеры, на общее качество проекта. И это влияние ну оооочень опосредованное. Наверное не всегда, ведь проекты разные - но часто.
Потому что это исправление должен делать не я, а тот, кто изначально эту фигню написал, и делать его не перед релизом, а ещё до того как это из его личной ветки попадет в общую. Но всем пофигу и проще сначала накопить несколько тысяч предупреждений, а потом на них вообще забить по причине уже невозможности все это исправить.
Применяйте к линтеру простой принцип: "Устраняй или объясняй". Либо вы исправляете замечание линтера (не потому что линтер написал, а потому что вы подумали и считаете разумным это исправить). Или - напишите в одну строку комментарий и прагмой отключите в этом конкретном месте конкретное замечание, если вы не считаете его разумным.
Это не значит, что надо сесть и весь проект за один раз переделать. Кто мешает в команде договориться и ввести полиси, что в новом или измененном коде линтер должен молчать (т.е. или исправлено, или отвергнуто, объяснено и отключено) ? И постепенно кодовая база станет лучше. Причем, в первую очередь станет лучше именно активно изменяемая кодовая база.
Есть, конечно, любители отключить прагмы линтера и поставить кволити гейт в 0 предупреждений. Но тут, как говорится: дураки - единственный вид на земле которому не грозит вымирание... Если команда пишет не то, что надо - а то чем линтер доволен, мне жаль и команду и их продукт...
Кто мешает в команде договориться и ввести полиси, что в новом или измененном коде линтер должен молчать
Наверное, тот же, кто мешает, для начала, использование этого линтера хотя бы просто внедрить :)
Гм, религия что-ли ?! Взять сонар, и воткнуть его в гитлаб пайплайн - не запредельный уровень усилий...
Обычно когда народ начинает объяснять, что проблема сложная и не решается - это признак нежелания, а не отсутствия возможности. Дальше тимлиду или скрам-мастеру надо работать с командой, чтобы понять чего они боятся и не понимают. И устранять страхи или непонимание - в конце концов, развитие команды - как бы то, за что они (часть) зарплаты получают.
Гм, религия что-ли ?!
Ну, вот, допустим, лесной обезьянин в лесу всю жизнь бананы с дерева камнем сбивал, тут вы приходите, приносите ему лестницу, рассказываете и показываете насколько это эффективней залезть на дерево по лестнице и нарвать за раз бананов сколько в карманы и прочее помещается. Обезьян говорит: "вау, круто!", потом вы уходите, и обезьян вашу лестницу выкидывает в ближайшую реку и продолжает бананы сбивать с дерева камнями. Подавляющее большинство людей это такие же обезьяноиды - их еще дедушка в детстве научил бананы камнем сбивать и с того момента ему хоть кол на голове теши, он будет это только так и делать :)
Ну если мы видим, что группа разработчиков не осиливает современный стандарт разработки - это менеджерская проблема. И решить ее менеджерскими способами не ахти как сложно: схантить им на рынке вожака с более высоким уровнем развития, и поставить DM поумнее, чтобы бдил... Имея положительный пример перед глазами и отрицательный по жопе (кошельку), все быстро учатся!
Да, еще для этого надо верить в то, что мы несем добро. Если начать петь песню что все разработчики разные, и имеют право на жизнь все способы работы - тогда лучше да, уйти и не смотреть что там творится. Я-то до сих пор верю что цивилизация - это путь от варварства и людоедства к светлым идеалам.. :-)
У меня создается впечатление, что и комментарии в коде (или их отсутствие), и линтеры, и код-ревью постепенно превращаются в идолы карго-культов. Со всеми атрибутами, включая религиозные войны...
Давайте начнем себе задавать вопрос "чтобы что?". Мы пишем тесты - чтобы что ? Чтобы зелененькое на экране горело ? Нет - чтобы иметь уверенность что нетривиальное поведение, заключенное в ограниченном куске кода, соответствует ожиданиям. Помогает ли это понимать код ? Да, безусловно - один пример иногда стоит 10 страниц объяснений. Является ли это серебряной пулей - конечно нет! Тесты никогда не отвечают на вопрос "как это внутри работает?" и "почему сделано именно так?". Кроме того, правда что тесты - это код, а код - это затраты. И на написание, и на поддержание в актуальном состоянии. Поэтому попытки обмазаться тестами в три слоя - ну такое себе... Хотя в условной медицине или условном аэроспейсе - или ты обмажешься тестами, или сядешь... В общем, надо разумно подоходить к вопросу.
Дальше - комментарии мы пишем чтобы что ? Чтобы выполнить норматив в линтере ? Нет - чтобы снизить неожиданность для читателя. Когда ты видишь в коде нечто странное - явное чесание правого уха левой ногой - всегда есть два варианта. Первый - это написали в спешке, или так получилось в результате прошлых рефакторов - и это надо выкинуть и переписать нормально. Второй - это уже пробовали написать нормально, но столкнулись с проблемами и пришлось переписать вот так чтобы заработало. И вот тут очень помогает комментарий, который в трех словах объясняет почему так. Или смотришь иногда - в 10 местах в коде один и тот же паттерн: отловили исключение, сделали логгинг, перевыкинули его выше. А в 11 месте логгинг есть, а повторного кидания нету - что это: просто забыли дописать, или так требует логика кода ?! Ну напиши ты однострочный комментарий: "<exceptionType> is not fatal, continuing" - и сразу всем читающим легче жить станет...
Код-ревью - мы для чего делаем ? Чтобы отловить ошибки ? Это - глупо. Ошибки надо ловить тестами. В большой команде код-ревью как минимум служит для взаимной синхронизации: просматривая код-ревью по диагонали я продолжаю понимать, чем занимается другая часть команды. После этого не надо на дейликах пересказывать MR на русском языке - все и так всё видят. Второй момент - это ответственность за кодовую базу. Когда я смотрю MR - я для себя отвечаю на вопрос: лично я, если меня поднимут по звонку в выходной день - готов ли отлаживать или менять этот код ? Или мне некомфортно, потому что, например, тестов нету. Или я не понимаю логику которая там реализована, или еще что-то. Если меня устраивает - тогда палец вверх. Если нет - надо это говорить сейчас, а не шипеть потом друг на друга "какой дурак такое пишет...".
В сухом итоге - главную задачу (написание качественного кода) никто не отменял и в ближайшие годы, не отменит. Инструментов для этого существует много. Применять их разумно - ответственность самой команды разработки. Неразумное применение инструментов влечет такие же негативные последствия как в жизни. Кто не верит - имеет право завтра начинать резать лук зубной щеткой и чистить зубы столовым ножом. Выжившие могут попробовать рассказать, насколько им это понравилось...
Все это разумно кроме одного - тесты не ловят ошибки. Ну или точнее, ловят конечно, какие-то, но они не могут гарантировать отсутствие ошибок.
Ревью тоже может ловить ошибки - но другого сорта. Например, использование алгоритмов не с тем О-малым. Но тоже не гарантирует их отсутствия.
Чтобы провести ревью на проверку логики надо сначала самому в этой логике разобраться, а на это нужно практически столько же времени как и на разработку того, что на ревью отдано. Оно, очень даже возможно, того стоит, но мало кто на такое согласится, вот и выходит, что всё ревью на деле скатывается к очередному дебильному спору уровня "надо ли ставить в конце файла пустую строку" - в итоге очередная пустая трата времени. А мой подход "аппрувь не глядя" это просто экономия времени :))
Если ревью скатывается к обсуждению "надо ли ставить пустую строку в конце файла", то ревьювер не отвечает за код, ревью которого проводит, либо ССЗБ и в какой-то момент в 3 часа ночи будет в срочном порядке его дебажить, потому что на проде стреляет, а тесты не отловили такой кейс.
Как описали выше, ревью нужно для:
синхронизации знаний о коде между разработчиками.
убедиться, что техническое решение в целом корректно и соответствует требованиям решаемой задачи.
А форматирование и прочее оставьте линтерам и SAST.
А форматирование и прочее оставьте линтерам и SAST.
Ну так ведь нету ни того, ни другого :)
Ну так это уже не проблема код-ревью, верно? Это проблема подхода команды разработки, которая точно также (спустя рукава) может подойти к любой проблеме.
P.S. Это я к тому, что это не повод избавляться от практики ревью, а повод повысить культуру разработки, чтобы применяемые для повышения качества кода инструменты давали нужный эффект.
Я не люблю вот этот формально-логический подход к оценке полезности тестов. Понятно, что если 10 тестов зеленые, но мы можем потенциально написать одиннадцатый который будет красный - то код содержит ошибку. Если же подходить с практически-инженерной точки зрения, то каждый тест снижает вероятность того, что в тестируемом коде находится ошибка негативно влияющая на систему в целом. Одна ситуация, когда система падает в экзотическом случае: в четверг с 8-00 до 8-20 в четную секунду если четыре раза быстро нажать клавиши шифт и esc. Другое - когда в happy path валится NPE и все пользователи стоят на ушах. И вероятности возникновения разные, и последствия...
А алгоритмы надо (сугубое IMHO) обсуждать до имплементации (бэклог грумминг, синки, дейлики, и т.д.). Если не тот алгоритм применен - и мы видим это только на этапе MR review, то обнять и плакать - затраченные на имплементацию часы и деньги уже не вернутся...
Я не люблю вот этот формально-логический подход к оценке полезности тестов.
Если же подходить с практически-инженерной точки зрения, то каждый тест снижает вероятность того, что в тестируемом коде находится ошибка негативно влияющая на систему в целом.
Может вы и математику не любите?
Переформулирую другим способом: еще один, два или десять тестов не приближают вас ни на на миллиметр к гарантии того, что в коде нет ошибок. Тесты теоретически этого обеспечить не могут, только показать наличие. Доказательства правильности у вас не будет. В общем случае (насколько я понимаю, доказательство правильности сродни проблеме останова машины Тьюринга, которая не имеет решения).
В частных же конкретных случаях - ну да, вы можете написать тесты на некоторые сложные части кода, и спать спокойно, ибо ощущение усталости от проделанной работы написанные тесты конечно же дают :) И оно реально будет работать лучше - особенно с учетом того, что для написания тестов код надо немного понимать, а это положительно сказывается на его качестве само по себе. Даже если только задумать их писать и потратить время на это обдумывание.
И причем заметьте - вы приводите пример, когда одна ошибка не воспроизводится, и де факто на нее и тест написать невозможно (в моем понимании, многопоточная логика приложений вообще на сегодня тестами как следует не покрывается). А второй случай явного падения, при всех его неприятных последствиях, диагностируется за пять минут, и исправляется навсегда. Потому что ошибка обычно простая. И строго говоря, такие ошибки предотвращаются не тестами, а рефакторингом.
Я люблю математику, но вижу разницу между выпускниками матфака классического университета, и инженерами из технического.
Я согласен с тем, что тестами очень сложно проверять side-effects (такие как гонки в многопоточности). Но если мы говорим о проверке поведения кода в методе - каждый тест очевидно покрывает не только конкретный кейс, а и все эквивалентные ему по code execution paths. На практике, если мы видим что бОльшая часть (и все ожидаемые happy paths) покрыты и горят зеленым - с большой долей вероятности этот код не доставит проблем в продакшене.
Собственно, вероятностный подход к надежности является в инженерной сфере общеприменимым (см. например заданные вероятности катастроф в авиации или в ядерной энергетике). Математики - замечательные люди, и чем дальше они продвинутся в формальном доказательстве корректности программ - тем лучше для человечества. Но мне их ждать некогда - я работаю сегодня, и с тем что есть...
Весь вопрос в сложности и типе проекта. Многопоточность, интеграция (с чем угодно) - это все сразу усложняет тестирование в разы. Причем один из таких случаев - это (тривиальная на вид) интеграция с любой СУБД. Все что мы тут можем - это сделать из этой интеграции, условно, что-то типа CRUD API, и тестировать с его моком. Что многие реальные классы ошибок ну ни разу не предотвращает.
Ничего не имею против тестов в целом, но есть куча случаев, когда они вообще ничего не дают. И да, я сам как раз из этой самой, инженерной школы.
Я согласен, что с увеличением сложности проекта сложность тестирования тоже увеличивается, и в какой-то момент встает вопрос экономической целесообразности, и где положить конец этому процессу.
С другой стороны, это еще и вопрос архитектуры. Ну потому что если у нас есть алгоритм проверки контрольной цифры штрих-кода или расчета скидки - и мы не можем его протестировать не запуская контейнер с базой данных - это сигнал о проблемной архитектуре и early warning об огромных, невообразимых проблемах которые нас ожидают в недалеком будущем...
В итоге, если тесты не писать с самого начала, то имеем много "воняющего кода" к которому тесты написать без его рефакторинга просто технически невозможно, и рефакторить его тоже нельзя, потому что тестов нет.
Конечно. Поэтому я и писал выше, что иногда достаточно подумать о том, как тестировать, чтобы в коде не было такого, что штрихкод не тестируется без базы.
Ну, строго говоря, рефакторинг это то что без изменения функциональности ) И он может быть проведен автоматически, правда далеко не в любом языке.
в коде не было такого, что штрихкод не тестируется без базы.
Про это часто забывают, что ценность написания тестов сразу она еще и в том, что подобные косяки дизайна/архитектуры сразу и вылазят, а не тогда, когда исправлять их уже либо слишком накладно, либо слишком рискованно. Просто пишешь код, тут же пытаешься написать к нему тесты, видишь, что это тяжело - это сразу сигнал, что, возможно (и, скорее всего, не просто возможно, а так оно и есть) с кодом что-то не так. Самый частый случай, это то, что код просто слишком сложный и его надо разбивать на отдельные куски так, чтобы тестировать каждый получившийся кусок отдельно от остальных. Второй по частоте вариант - код нарушает "принцип инверсии зависимости" (dependency inversion), его отдельные части сильно сцеплены (tight coupling) друг с другом напрямую, минуя абстракции, и из-за этого невозможно протестировать отдельный кусок кода не вовлекая в это остальной код.
Если взять разницу в трудоемкости исправления ошибки в коде сразу после написания и тогда когда её обнаружили тестеры (а еще хуже, когда её обнаружили только в production), то даже если тест отлавливает хотя бы одну ошибку из десяти, то все равно это уже выгода.
В моей практике (а это сильно интеграционные проекты) - почти нет такой выгоды. То что мы иногда ловим в проме, всплывает лишь на одной интеграции скажем из 20 похожих. Не всегда на это просто написать тест даже постфактум. А уж предугадать заранее кейс, который потребует теста - вообще можно в редчайших случаях.
Это не значит что везде так, далеко не все проекты вообще такие.
Эффективные Практики Подготовки к Code Review