Пролог
...Теперь давайте обсудим появившиеся проблемы. Задача, которую мы обещали отдать на приемку в начале недели, до сих пор на ревью, что не так?
Все так, идет ревью.
Там изменения должны быть в двух файлах, мы их неделю смотрим?
Нет.
Что нет?
Нет, не в двух.
То есть пришлось изменять больше, чем планировалось?
Нет.
Что нет?
Нет, изменили в итоге только один файл.
Тогда тем более, почему задача еще не в приемке?
Там спорный момент.
В задаче нужно изменить текст сообщения. О чем там можно спорить?
Там строка длинная.
И?
И она не влезает в 80 символов.
И?
В нее добавили перенос.
И?
Теперь написано правильно, но некрасиво.
Мы такую же задачу на прошлой неделе отдали за час. Мы не могли сделать в этой так же, как в прошлой?
Но мы так и сделали.
Так если мы так и сделали, почему задача ревью пройти не может?
Потому что ее делал Леша.
У нас Леша не может строчку поменять по аналогии с прошлой задачей?
Он ее и поменял.
Что тогда не так с Лешей?
С Лешей все нормально.
А с кем не нормально?
Со всеми нормально. Просто ревью смотрит Слава.
Так прошлое ревью тоже Слава смотрел, почему она так быстро прошла?
Так ее Коля делал, Коля точно сделает правильно, ее и смотреть не надо...
По мотивам некоторых проблем ревью...
Давайте знакомиться
Всем привет! Меня зовут Дмитрий, я тимлид в команде разработки. Мы классная команда, пишем большой продукт с не менее большой кодовой базой. У нас есть множество разных процессов, и сегодня хотелось бы рассказать про один из них, достаточно важный – это ревью кода. В статье будет немного истории о том, как этот процесс развивался у нас, как мы решали проблемы из пролога к статье, а также мысли и опыт того, как этот процесс влияет на другие составляющие в работе команды.
Обещанная история
Во всех командах, с которыми я успел поработать с момента прихода в компанию, этап ревью присутствовал в том или ином виде. Даже там, где в разработке был всего один человек, в этом вопросе помогали участники других команд. Сам процесс от команды к команде отличался. Где-то ревью проводили закрепленные за этим этапом люди, где-то назначался случайно выбранный разработчик. За каждый pull request/задачу, как правило, отвечал один ревьюер. Критерии ревью тоже были очень разные. На них влияли (да и сейчас влияют) как правила самой команды, так и личные предпочтения ревьюера. Кто-то смотрел только техническое состояние кода, кто-то мог проверять все, начиная от форматирования кода и заканчивая соответствию бизнес-требованиям задачи.
Если говорить о нашей команде, то процесс ревью на сегодняшний день выглядит так:
Ревью проходит по изменениям, которые хранятся в отдельных ветках, созданных для конкретной задачи. Изменения попадут в основные ветки только после успешного прохождения ревью. Да, это кажется очевидным, но так было не всегда.
Этап ревью – строго после прохождения автотестов. И это выглядит логичным, но так тоже было не всегда по тем или иным причинам.
Ревьюеров двое. Сначала набор изменений смотрит один разработчик, затем – второй.
Если помимо изменений в продукт в рамках задачи сделаны изменения в общие компоненты, которые затрагивают другие продукты, то они отправляются на ревью заранее определенным людям из разных команд. При этом изменения могут быть слиты до продуктовых , если ревью прошло успешно и это не помешает работе других продуктов. Если есть сомнения – ждем успеха по всем сделанным изменениям и сливаем все одновременно.
Если есть изменения в другие продукты, они также отправляются соответствующим ревьюерам.
Если по изменениям есть замечания – задача возвращается на исправление и после доработки заново должна пройти ревью.
Повторное ревью продуктовых изменений проводят два разработчика, которые могут не совпадать с ревьюерами предыдущей итерации. Это может показаться странным, но далее я постараюсь объяснить, почему мы делаем именно так.
Весь описанный процесс нам помогает поддерживать автоматизация через связку таск-трекера, хранилища кода и телеграм-бота. Именно последний берет на себя всю рутину по выбору и назначению соответствующего ревьюера, а также предоставляет другую полезную информацию, например ссылки на результаты тестов. Да, в некоторых моментах у нас есть «ручной труд», особенно в момент появления общих изменений между продуктами. Но верю, что в конечном итоге и эти вопросы удастся автоматизировать.
Двойное ревью
Как и во многих других механизмах в команде, к двойному ревью мы пришли постепенно. Сначала был классический один ревьюер, он смотрел изменения, оставлял замечания, снова смотрел изменения и так далее, пока все написанное не получало заветную зеленую галочку. Этот процесс хорошо работал до того момента, пока в команде не появилось определенное количество новеньких. Вообще, когда в команду приходят новые сотрудники – это хороший тест для процессов в команде. И для ревью в том числе. Сразу появляются вопросы:
В какой момент новенькому дать возможность проводить ревью?
Как сделать так, чтобы он чего-то не пропустил случайно?
А помните нашу договоренность про то, что функция не больше 100 строк, но только если это не модуль А? Ему ведь надо сказать про нее...
И так далее. Здесь есть несколько подводных камней, которые могут утащить команду за собой в пучину неправильных решений. Например, одной из реакций может стать такая: давайте тогда новеньких вообще подключать не будем, пусть они пишут код, состав ревьюеров остается прежним. Это удобно «стареньким», пока есть «старенькие». Но на дистанции здесь будет очень много минусов. Если в команду пришли новые люди – значит объем изменений за единицу времени, скорее всего, вырастет. И, соответственно, прежним ревьюерам придется больше времени уделять ревью, чтобы сохранить тот же уровень самого процесса. Либо делать ревью более поверхностно, а это может привести к потере качества.
Хорошая мысль: давайте будем учить новеньких делать ревью. Допустим, посадим его рядом с собой и будем рассказывать на примере текущих задач, как это делать и на что смотреть. В целом, рабочая схема, только не всегда получится быстро синхронизировать график двух разработчиков, особенно если они в разных часовых поясах. И даже если получится – нам кажется, что такой процесс выглядит чуть менее стройным и довольно частным, потому что через какое-то время мы так делать перестанем – новенький наберется опыта и пойдет делать ревью сам, а нам бы не хотелось иметь слишком много процессов, про которые надо помнить.
Поэтому схему с обучением мы немного переделали и постепенно она превратилась в двойное ревью. В общем случае оно проходит так: первый ревьюер смотрит набор изменений и принимает по нему свое решение: либо оставляет замечания, либо ставит approve. После этого задача переходит ко второму ревьюеру. Он видит набор изменений и замечания, которые оставил первый ревьюер. Он может как оставить свои замечания, так и не согласиться с правками первого ревьюера. В последнем случае они обсуждают между собой спорный вопрос и стараются прийти к общему решению. В итоге второй ревьюер либо возвращает задачу на доработку, если есть замечания, либо вливает их в общие ветки и отдает задачу дальше по процессу. Важным моментом является то, что второй ревьюер обязательно «подсвечивает» первому свои замечания в личном сообщении, а первый смотрит и делает выводы, по какой причине он не оставил эти замечания.
Такая схема позволяет встраивать новеньких в процесс ревью, при этом не теряя в качестве. Причем в разное время у нас в команде первый и второй ревьюер выбирались по-разному. Был период, когда новенькие попадали строго в пул «первых» ревьюеров. Это логично, потому что так мы гарантируем, что за ним будет второй, более опытный человек, который подстрахует и подскажет. Пробовали мы и другую схему, когда новенький может быть как первым, так и вторым. Тоже работает нормально, потому что он может получить как изменения еще без замечаний, так и с уже оставленными вопросами. При таком раскладе важно донести до новеньких, что второй ревьюер может и должен проверять как изменения, так и замечания. И неважно, насколько опытным был первый ревьюер. Вообще, идеальная схема, при которой ревьюер не видит имен – только условные «Разработчик» и «Ревьюер». Но до такого, увы, мы пока не дошли. Важное замечание – нужно исключить ситуацию, при которой и первым, и вторым ревьюером становятся новенькие. В целом, это можно ограничить правилами в автоматизации, и поэтому, в том числе, она важна.
Помимо того, что такой подход позволяет достаточно просто подключать к процессу ревью новеньких, он еще и дает построить достаточно понятный для разработчиков процесс передачи знаний. Здесь не требуется писать статью, готовить мастер-класс или презентацию, ты просто делаешь свою работу и показываешь ее другому человеку.
Случайный выбор ревьюера
Выше уже упоминалось о том, что в качестве ревьюера изменений выбирается случайный разработчик из определенной группы. Более того, в случае повторного ревью, когда нужно посмотреть изменения с учетом исправления предыдущих замечаний, ревьюером может быть выбран человек, которые не участвовал в предыдущей итерации. На первый взгляд это может показаться нелогичным: ведь мы уже выбрали первого (а может быть и второго) ревьюера, он оставлял замечания, он же и должен посмотреть исправления по ним. Это действительно разумный довод, и некоторые команды, которые используют в нашей компании случайный выбор ревьюера, так и делают. Я же попробую объяснить, почему мы поступаем иначе.
Представим процесс выбора разработчика для повторного ревью. Смотрим, кто был в прошлый раз и хотим назначить его же. Всегда ли у нас это получится? Нет. Допустим, человек ушел в отпуск, он на больничном, взял отгул или уехал командировку. Значит, нам придется выбрать другого. Кто-то может возразить, что это не такая частая ситуация, и большую часть времени человек будет доступен. Хорошо, давайте выберем его. И одновременно заглянем в набор изменений по задаче. Там есть замечания, много их или мало – зависит от многих факторов. В том числе от того, насколько глубоко человек «копает» на ревью. И здесь получается тонкий момент: тот, кто оставляет больше замечаний, будет чаще занят ревью. В обратную сторону – если очень хочется хакнуть систему и мы не очень любим ревью, то мы будем меньше комментировать и реже участвовать в этом процессе. Поэтому нам кажется, что честнее будет выбирать случайного ревьюера из пула, для того чтобы немного распределить нагрузку.
Еще одно справедливое «но» касается того, что новый ревьюер может не до конца разобраться в том, насколько правильно разработчик понял и исправил оставленное замечание. Это действительно так, ревьюеру нужно будет время на осознание, а возможно, даже помощь автора замечания. Но при правильной работе с такими ситуациями это дает больше плюсов, чем минусов.
Во-первых, приводят «язык» замечаний к общему знаменателю: если хочешь, чтобы к тебе перестали ходить с вопросами по поводу твоих комментариев – пиши так, чтобы другим было понятно. Шутка ли, за все время проведения ревью я не раз и не два видел, как замечания состояло всего из одного слова: «Исправить». И в большинстве своем, мне и как ревьюеру, и как разработчику было абсолютно непонятно, что именно здесь нужно исправить.
Во-вторых, это все тот же процесс обучения, как и в связке между первым и вторым ревьюером. Когда ты получаешь на ревью очередные изменения с замечаниями, всегда полезно спросить себя: «Оставил бы я такое замечание, если бы его здесь не было?».
В-третьих, новый ревьюер позволяет убрать субъективность. Был кейс, когда ревьюер оставлял по одному замечанию за раз. Разработчик исправлял его, отправлял на ревью снова. Приходило новое замечание, уже по другому участку кода. Процесс повторялся снова и снова. Очевидно, что ничего общего с поддержанием качества кода и передачей знания такая история не имеет. Более того, она может привести к конфликту в команде или развить его, если причина такого поведения первоначально в нем.
Еще одно замечание, связанное с таким процессом: «На это уходит так много времени, точно ли это стоит того?». Действительно такой подход требует больше времени, ведь каждый новый ревьюер должен осознать код, посмотреть прошлые замечания и исправления по ним, подумать о том, что он сам хочет прокомментировать. Но мы для себя на текущем этапе решили, что это стоит того. Кроме того, такой затратный по времени процесс подстегивает к тому, чтобы искать пути сокращения затрат. И это приводит нас к тому, что, как и любой процесс в команде, ревью может и должно влиять на другие процессы.
Влияние на другие процессы
Задачи бывают разные. Большие и маленькие, сложные и простые, хорошие и .... нормальные. И объем изменений в коде тоже бывает разный. Небольшие изменения ревьюить проще и дешевле, большие – сложнее и дороже. Но ревью как раз и позволяет посмотреть, все ли мы сделали правильно на предыдущих этапах. На этот вопрос не всегда может и должен ответить ревьюер, поэтому здесь важно участие человека, который отвечает за построение и поддержание процессов, например тимлида.
На что же обратить внимание и какие выводы из этого можно сделать? Например, как часто нам приходят на ревью «большие» изменения. Если нам кажется (пока хотя бы по ощущениям – это вообще очень важная часть работы), что часто – возможно мы на предыдущем этапе плохо разбили требование на отдельные задачи. Для проверки можно взять одну из таких задач и посмотреть, действительно ли ее можно было подробить и не делать все изменения в одной. На этапе, когда кода еще нет, не всегда очевидно, где можно отрезать от задачи кусок, но на ревью этот код уже у нас есть. И мы можем за счет этого «дать обратную связь» другим механизмам работы, где-то что-то подкрутить в процессах.
Вывод – смотрим объем изменений и пробуем разбить задачи, чтобы объем становился меньше.
Что еще можно увидеть в данной ситуации? Допустим, задачи мы подробили, но изменений на каждую все равно прилетает много. В этом случае можно и нужно посмотреть, почему эти изменения сделаны. Возможно, разработчик в процессе решения основной задачи решил сразу «прибраться» в соседних модулях. Такое тоже нельзя оставить без внимания. Если разработчик регулярно делает это, при этом не ломает все в системе – нужно обязательно это отметить, желательно перед всей командой. Возможно, это мотивирует других поступать также или узнать о том, как этот разработчик выбирает, что делать прямо сейчас, а что оставить до следующего раза.
Вывод – смотрим случаи, когда человек удачно вышел за границы задачи, и показываем команде, когда это уместно.
Или в обратную сторону – разработчик поправил соседние модули так, что мы получили кучу ошибок. Тогда стоит обсудить с ним, что так делать не стоит и предложить альтернативу: поставь задачу на рефакторинг, опиши в ней то, что нужно будет сделать. Когда будет возможность – мы возьмем ее и сделаем. В итоге, мысли разработчика никуда не потерялись, но и мы в рамках текущей задачи ничего лишнего не меняли. Кстати, в какой-то момент к подобному подходу мы сами пришли и сейчас делаем именно так.
Вывод – смотрим случаи, когда человек неудачно вышел за границы задачи, и показываем ему это.
Еще один кейс – это анализ переписки по замечаниям. Поскольку у нас есть правило, что на каждое оставленное замечание должен быть дан ответ, здесь можно найти очень много полезной информации. Например, увидеть ситуацию, когда один ревьюер делает одно замечание, его исправляют, а затем другой ревьюер дает противоположную рекомендацию. Этот момент важен, потому что может говорить о том, что в процессе есть какая-то «серая» зона, которую каждый может трактовать по-своему. Если это действительно так – самое время внести дополнения и изменения в правила команды, например, в соглашение по стилю кода.
Вывод – там, где есть противоположные мнения, – мы что-то пропустили, например строчку в гайде.
Заключение
Мы понимаем, что описать готовое решение к каждому случаю, который происходит на этапе ревью, конечно, невозможно. Равно как и нельзя описать абсолютно все правила в различных гайдах и регламентах. Самое главное – сделать так, чтобы процесс помогал в достижении тех целей, которые ставит перед собой команда.