Важны ли архитектурные моменты и уровень покрытия тестами в код-ревью
Что проверяют
Умеете ли вы ревьюить глубже форматирования и понимаете ли, что процент покрытия сам по себе ничего не гарантирует. Следом обычно спрашивают, какой порог покрытия вы бы поставили в CI и что делать, если архитектурная проблема всплыла в уже готовом PR.
Как отвечать
Важны, но у них разная цена и разное время проверки.
Архитектуру дешевле обсуждать до кода: в дизайн-доке или на коротком созвоне. На ревью вы ловите нарушения уже принятых решений:
- направление зависимостей: хендлеры не ходят в базу напрямую, доменный пакет не импортирует транспорт;
- интерфейсы: маленькие и объявлены на стороне потребителя, по принципу «принимай интерфейсы, возвращай структуры»;
- границы пакетов: внутреннее спрятано в
internal/, нет пакетов-свалок вродеutils; - ошибки и контекст: ошибки оборачиваются через
fmt.Errorf("...: %w", err),context.Contextпередаётся первым аргументом, а не хранится в структуре; - конкурентность: у каждой горутины есть способ завершиться, нет утечек и гонок.
С тестами смотрите на содержание, а не на цифру. go test -cover и -coverprofile вместе с go tool cover -html показывают, какие ветки никто не трогал. Важнее, чтобы были проверены ветки ошибок и граничные случаи, а табличные тесты описывали реальные сценарии. Для конкурентного кода обязателен прогон с -race. В CI полезнее считать покрытие изменённых строк, а не всего репозитория.
Всё механическое, то есть gofmt, go vet и golangci-lint, должно проверяться автоматически, чтобы ревьюер тратил внимание на смысл.
Где спотыкаются
Делают процент покрытия целью. Появляются тесты без единой проверки, которые просто вызывают функции ради зелёной цифры.
Поднимают архитектурный спор на PR из двух тысяч строк. Автор уже вложился, и ревью превращается в конфликт. Такое нужно ловить раньше или договариваться о разбиении.
Тратят ревью на стиль, который поймал бы линтер, и пропускают утечку горутины или незакрытый resp.Body.