У бага критерий проверки был. У фичи был. У рефакторинга критерия нет по определению: снаружи ничего не должно измениться. Успех выглядит как отсутствие событий.
Это делает рефакторинг задачей, на которой агент ошибается тише всего. Четвёртый заход серии, тот же стенд: Symfony 8, 96 тысяч строк, 784 теста, покрытие 61 процент.
Цель захода
IssueService — класс на 780 строк с 24 публичными методами. Создание, изменение, перенос между спринтами, смена исполнителя, переходы статусов, работа с метками, подсчёт времени. Классический сервис, который рос вместе с продуктом.
Задача: разнести на несколько классов по зонам ответственности, поведение сохранить.
Два разных рефакторинга
Прежде чем что-то отдавать агенту, стоит разделить понятие.
Механический рефакторинг — переименование, извлечение метода, перенос класса, замена конструкции на эквивалентную. Для него есть детерминированные инструменты вроде Rector: они воспроизводимее агента, хотя их результат всё равно надо проверять тестами и ревью.
Смысловой — разделение ответственности, изменение границ, введение абстракции. Правильность зависит от понимания предметной области.
Первое агенту отдавать не нужно: для него есть воспроизводимые инструменты. Второе отдавать можно, но только с подготовкой.
Я это разделение вывел не сразу. Первый заход был как раз попыткой отдать всё целиком.
Заход без подготовки
Формулировка: «разбей IssueService на несколько классов по ответственностям, поведение не меняй».
Дифф: 1240 строк, 11 новых файлов, 41 минута. Все 784 теста зелёные.
Зелёные тесты и стали ловушкой. Я читал дифф два часа и нашёл три изменения поведения, которые тесты не поймали.
Первое. В исходном методе смены исполнителя стояла проверка, что новый исполнитель — участник проекта. В новом классе она пропала. Не потому что агент решил её убрать: он собрал метод из двух кусков исходного кода, и проверка оказалась в куске, который ушёл в другой класс, где вызывается в другом порядке. Тест на это был, но он проверял успешный сценарий.
Второе. Порядок операций при переносе задачи изменился: раньше журнал активности писался до сохранения, стало после. Это уже наблюдаемое поведение для слушателей и тестов последовательности вызовов. Последствия при откате зависят от реализации журнала: запись в той же транзакции откатится вместе с задачей, внешний вызов — нет. В исходной постановке эта граница вообще не была зафиксирована.
Третье. Метод, возвращавший null при отсутствии задачи, стал бросать исключение. Формально чище. Фактически два вызывающих места проверяли на null, и один из них теперь получает исключение вместо ветки «ничего не делаем».
Ни одно из трёх не поймал ни один из 784 тестов. Линейное покрытие 61 процент означает, что 39 процентов измеренных строк не исполняются в тестах, и переставлять код в этой зоне — стрельба вслепую.
Характеризующие тесты
Правильный порядок оказался обратным тому, что я делал: сначала зафиксировать текущее поведение, потом менять код.
Характеризующий тест отличается от обычного тем, что он описывает не желаемое, а фактическое поведение — включая странности, которые, возможно, являются багами.
Задача на этот шаг — только тесты, код не трогать.
Для каждого публичного метода IssueService напиши тест, фиксирующий
ТЕКУЩЕЕ поведение, включая:
- возврат null там, где возвращается null
- порядок побочных эффектов (журнал, уведомления, сохранение)
- поведение при отсутствующей сущности
- поведение при недостаточных правах
Если поведение выглядит неправильным — всё равно зафиксируй его как есть
и пометь комментарием ПОДОЗРИТЕЛЬНО. Не чини.Пометка «подозрительно» оказалась ценной сама по себе. Агент поставил её в семи местах, и в двух из них действительно были баги, о которых никто не знал.
Результат шага: 46 тестов, покрытие класса выросло с 58 до 94 процентов, полтора часа работы агента и мои двадцать минут на чтение.
После этого повторный рефакторинг поймал два из трёх изменений поведения сразу. Третье, с порядком записи в журнал, не поймал никто — на него теста не было и в характеризующем наборе, потому что порядок побочных эффектов агент зафиксировал не везде.
Лимит на размер диффа
Дифф на 1240 строк невозможно отревьюить внимательно. Я читал его два часа и всё равно нашёл не всё.
Второй заход шёл шагами с явным потолком:
Шаг 1: вынести работу со спринтами в SprintAssignmentService.
Перенести методы, вызовы в старом классе заменить делегированием.
Дифф не больше 200 строк. Тесты не менять. Остановись после шага.Пять шагов, каждый до 200 строк, каждый со своим прогоном тестов и своим ревью. После них — отдельная уборка делегирующих методов. Суммарный объём остался сопоставимым, но читается совершенно иначе: в небольшом диффе видно каждую строку.
Промежуточное делегирование — важная деталь. После каждого шага старый класс остаётся на месте и просто вызывает новый. Это значит, что после любого шага можно остановиться, и система рабочая. Финальная уборка делегирующих методов идёт отдельным шестым шагом.
Чего агент не понимает в рефакторинге
Он хорошо переставляет код и плохо оценивает, что является поведением.
Возврат null вместо исключения для него — деталь реализации. Порядок побочных эффектов — тоже. Момент вызова flush() — тем более. А это ровно те вещи, на которые полагается остальной код.
Из этого следует практический вывод: в постановке надо перечислять, что именно считается поведением. У меня в задачах на рефакторинг теперь стоит блок:
ПОВЕДЕНИЕМ СЧИТАЕТСЯ И НЕ МЕНЯЕТСЯ:
- сигнатуры публичных методов, включая nullable-типы
- что возвращается при отсутствии сущности
- порядок побочных эффектов: изменение состояния, журнал, уведомление, flush
- какие исключения бросаются и в каких случаях
- границы транзакцийС этим блоком третий заход прошёл без изменений поведения вовсе.
Замер
| Заход | Подготовка | Дифф | Ревью | Пропущено изменений поведения |
|---|---|---|---|---|
| Без подготовки | нет | 1240 строк одним куском | 2 ч | 3 |
| Характеризующие тесты + шаги | 1,5 ч на тесты | 5 × до 200 строк + уборка | 40 мин | 1 |
| То же + определение поведения | 1,5 ч | 5 × до 200 строк + уборка | 35 мин | 0 |
Полтора часа подготовки против двух часов ревью и трёх пропущенных регрессий. Считать тут особо нечего.
И побочный результат, который я не планировал: 46 характеризующих тестов остались в проекте навсегда. Общее покрытие выросло с 61 до 64 процентов, а класс, который чаще всего трогают, покрыт почти полностью.
Что дальше
В этом заходе тесты были инструментом. В следующем они станут целью — и выяснится, что «написать тесты» агент понимает как «написать тесты, которые проходят», а это совсем не то же самое, что тесты, которые ловят.
