Reviewing a Patch/ru

Многим кажется, что у них маловато квалификации для полной проверки патчей PostgreSQL. Однако проверка состоит из множества шагов. При этом вы можете помочь проекту на ранних стадиях проверки. Если вы умеете накатывать патчи и использовать новые фичи, значит, вы достаточно подготовлены для их ревью.
Коммитеры также проверяют патчи перед вливанием в базовый код. Перед проверяющими стоит задача максимально снять нагрузку с коммитеров, обнаруживая и устраняя простые проблемы. Проверяющий не несет обязательств по качеству, а лишь отчитывается о найденных проблемах. Этот шаг считается выполненным, как только проверяющий сочтет, что патч готов для более глубокой проверки коммитерами. В качестве примера обратитесь к этому ревью. Проверки разных патчей, конечно, могут отличаться друг от друга, все зависит от конкретной задачи.
Обратите внимание на насмешливую презентацию Джошуа Толле (Josh Tolley), и более серьезный Обзор процесса проверки патчей.
Важные моменты:
* commitfest, взятые в работу, доступны здесь.
* У каждого commitfest есть менеджер, которого можно попросить о помощи или задать вопросы организационного характера.
* Согласование на проверку не требуется.
* Пожалуйста, подписывайтесь на проверку, как запланируете ревью. Некоторые участники снимают заявку до фактического завершения проверки. Так делать не стоит, это усложняет планирование.
* С другой стороны, мы просим, чтобы первоначальная проверка укладывалась в 5 дней, чтобы избегать блокировок на этой стадии. При этом возможно провести частичную проверку, либо запросить больше времени. Достаточно оставаться на связи.
* Основное общение по вопросам ревью и разработки должно производиться через рассылку pgsql-hackers.
* Результаты проверки необходимо отправлять ответом на письмо, содержащее исходный патч. Соблюдайте целостность переписки, иначе автор патча может не увидеть результатов проверки.
Типичные стадии проверки:
== Подача заявки на проверку (требуемые навыки: патч, понимание английского языка) ==
* Патч предоставлен в 'context'-формате с контекстом? (см. context-формат diff)
** Патчи diff в normal и plain-форматах, в которых отражены только строки без контекста не приемлемы.
** По хорошему, в подаваемой заявке патч должен быть в context-формате (diff -c) или unified-формате (diff -u), что значительно облегчает чтение кода.
* Ложится ли патч на master-ветку git без проблем?
* Содержит ли патч необходимые тесты, исправления документации и тд.?
== Проверка удобства использования (требуемые навыки: статическое тестирование, умение находить и читать спецификации) == 
Прочтите описание назначения патча и обдумайте:
* Действительно ли патч реализует заявленное? 
* Нужно ли это вообще? 
* Есть ли это на текущий момент? 
* Соблюдены ли SQL-спецификации, или поведение, принятое сообществом? 
* Включает ли поддержку pg_dump (если применимо)?
* Несет ли угрозы? 
* Все ли обосновано?
== Функциональное тестирование (требуемые навыки: использование команд patch, configure и make, вывод ошибок в лог) ==
Накатите патч, скомпилируйте и протестируйте:
* Работает ли функциональность, как заявлено?
* Существуют ли частности, неучтенные автором?
* Выводятся ли предупреждения или падает код?
** Ревью необходимо проводить со включенными --enable-cassert и --enable-debug флагами configure, см. Работа с git. Это позволит обнаружить возможные пропущенные проблемы. Стоит помнить, что собранный с этими флагами PostgreSQL может работать значительно медленнее, чем без этих флагов. Если проверяется патч, связанный с производительностью, то непосредственно перед тестом на скорость, необходимо пересобрать уже без этих флагов. Некоторые издержки флага --enable-cassert можно снизить за счет опции debug_assertions = false конфигурационного файла postgresql.conf. Более подробно см. Опции для разработки. По умолчанию эта опция включена, если сервер собран с флагом --enable-cassert. Также использование флага --enable-debug однозначно приведет к издержкам с применением большинства компиляторов, кроме некоторых, таких как gcc, например.
== Проверка производительности (требуемые навыки: умение замерять производительность) ==
* Приводит ли патч к снижению производительности на простых тестах? 
* Заявлено ли улучшение производительности в описании?
* Не затрагивается ли производительность других компонентов?
== Проверка кода (требуемые навыки: оценка согласно принятому стилю, опыт с переносимым кодом, базовое умение чтения кодов на C) ==
Прочтите изменения кода и задумайтесь:
* Соблюдается ли принятый стиль?
* Есть ли проблемы переносимости? 
* Заработает ли на Windows/BSD и тд.? 
* Есть ли комментарии и в достаточном ли объеме?
* Выполняется ли заявленная логика и корректно ли?
* Генерируются ли предупреждения при компиляции?
* Можете ли вы свалить патч?
== Проверка архитектуры (требуемые навыки: опыт в архитектуре PostgreSQL) ==
Задумайтесь о вносимых изменениях в контексте всего проекта:
* Все ли согласуется в рамках проекта, функциональности/модули? 
* Вносятся ли взаимозависимости, которые могут привнести проблемы?
== Оценка проведенной проверки ==
* Проведена ли полная проверка всех предполагаемых моментов со стороны проверяющего?
Category:Development
Category:Russian