Рефакторинг - уместно ли просто переписать код, если все тесты пройдены?

9

Недавно я смотрел «Все мелочи» из RailsConf 2014. Во время этого выступления Сэнди Метц реорганизует функцию, которая включает в себя большой вложенный оператор if:

def tick
    if @name != 'Aged Brie' && @name != 'Backstage passes to a TAFKAL80ETC concert'
        if @quality > 0
            if @name != 'Sulfuras, Hand of Ragnaros'
                @quality -= 1
            end
        end
    else
        ...
    end
    ...
end

Первый шаг - разбить функцию на несколько более мелких:

def tick
    case name
    when 'Aged Brie'
        return brie_tick
    ...
    end
end

def brie_tick
    @days_remaining -= 1
    return if quality >= 50

    @quality += 1
    @quality += 1 if @days_remaining <= 0
end

То, что я нашел интересным, было способом, которым были написаны эти меньшие функции. brie_tickНапример, был написан не путем извлечения соответствующих частей исходной tickфункции, а с нуля, ссылаясь на test_brie_*модульные тесты. Как только все эти юнит-тесты пройдены, brie_tickсчитается выполненным. Как только все маленькие функции были выполнены, исходная монолитная tickфункция была удалена.

К сожалению, докладчик, казалось, не знал, что этот подход привел к тому, что три из четырех *_tickфункций были неправильными (а другая была пустой!). Существуют крайние случаи, в которых поведение *_tickфункций отличается от поведения исходной tickфункции. Например, @days_remaining <= 0в brie_tickдолжно быть < 0- так brie_tickне работает правильно при вызове с days_remaining == 1и quality < 50.

Что здесь пошло не так? Это провал тестирования - потому что не было тестов для этих конкретных крайних случаев? Или провал рефакторинга - потому что код должен был быть преобразован шаг за шагом, а не переписан с нуля?

user200783
источник
2
Я не уверен, что понял вопрос. Конечно, это нормально, чтобы переписать код. Я не уверен, что именно вы подразумеваете под «нормально ли просто переписывать код». Если вы спрашиваете: «Можно ли переписать код, не вдаваясь в долгие размышления», ответ будет «нет», так как писать код таким способом нехорошо.
Джон Ву,
Это часто происходит из-за планов тестирования, в основном сосредоточенных на тестировании вариантов использования успеха и очень небольшого (или не совсем) на покрытии случаев использования ошибок или неполных случаев использования. Так что это в основном утечка освещения. Утечка тестирования.
Laiv
@JohnWu - у меня сложилось впечатление, что рефакторинг, как правило, выполнялся в виде серии небольших преобразований в исходный код («метод извлечения» и т. Д.), А не путем простого переписывания кода (под которым я имею в виду написание его заново с нуля, даже без глядя на существующий код, как это сделано в связанной презентации).
user200783
@JohnWu - Является ли переписывание с нуля приемлемой техникой рефакторинга? Если нет, то неутешительно видеть, что такая уважаемая презентация по рефакторингу использует такой подход. OTOH, если это приемлемо, то непреднамеренные изменения в поведении можно обвинить в пропущенных тестах - но есть ли способ быть уверенным, что тесты охватывают все возможные крайние случаи?
user200783
@ User200783 Ну, это более серьезный вопрос, не так ли (как я могу убедиться, что мои тесты исчерпывающие?) С практической точки зрения, я бы, вероятно, запустил бы отчет о покрытии кода, прежде чем вносить какие-либо изменения, и внимательно изучил бы все области кода, которые этого не делают. выполняйте упражнения, следя за тем, чтобы команда разработчиков учитывала их логику.
Джон Ву,

Ответы:

11

Это провал тестирования - потому что не было тестов для этих конкретных крайних случаев? Или провал рефакторинга - потому что код должен был быть преобразован шаг за шагом, а не переписан с нуля?

Обе. Рефакторинг с использованием только стандартных шагов из оригинальной книги Фаулерса определенно менее подвержен ошибкам, чем переписывание, поэтому часто предпочтительнее использовать только эти виды детских шагов. Даже если для каждого граничного случая нет модульных тестов, и даже если среда не обеспечивает автоматическое рефакторинг, одно изменение кода, такое как «вводная переменная объяснения» или «функция извлечения», имеет гораздо меньший шанс изменить подробности поведения существующий код, чем полная перезапись функции.

Однако иногда переписывание фрагмента кода - это то, что вам нужно или нужно делать. И если это так, вам нужны лучшие тесты.

Обратите внимание, что даже при использовании инструмента рефакторинга всегда существует определенный риск появления ошибок при изменении кода, независимо от применения меньших или больших шагов. Вот почему рефакторинг всегда нуждается в тестах. Также обратите внимание, что тесты могут только уменьшить вероятность ошибок, но никогда не доказать их отсутствие - тем не менее, использование методов, таких как просмотр кода и покрытие ветвлений, может дать вам высокий уровень достоверности, а в случае перезаписи раздела кода это Часто стоит применять такие приемы.

Док Браун
источник
1
Спасибо, это имеет смысл. Итак, если окончательное решение нежелательных изменений в поведении - это комплексные тесты, есть ли способ быть уверенным, что тесты охватывают все возможные крайние случаи? Например, было бы возможно иметь 100% охват, но при brie_tickэтом никогда не проверять проблемный @days_remaining == 1случай, например, с помощью @days_remainingустановки на 10и -10.
user200783
2
Вы никогда не можете быть абсолютно уверены, что тесты охватывают все возможные крайние случаи, так как невозможно выполнить тестирование со всеми возможными входными данными. Но есть много способов получить больше уверенности в тестах. Вы можете взглянуть на мутационное тестирование , которое является способом проверки эффективности тестов.
BDSL
1
В этом случае пропущенные ветви могли быть обнаружены с помощью инструмента покрытия кода при разработке тестов.
cbojar
2

Что здесь пошло не так? Это провал тестирования - потому что не было тестов для этих конкретных крайних случаев? Или провал рефакторинга - потому что код должен был быть преобразован шаг за шагом, а не переписан с нуля?

Одна из вещей, которая действительно трудна в работе с унаследованным кодом: получение полного понимания текущего поведения.

Устаревший код без тестов, которые ограничивают все варианты поведения, является распространенным явлением в дикой природе. Что оставляет вас в догадке: означает ли это, что неограниченное поведение - это свободные переменные? или требования, которые не указаны?

Из разговора :

Теперь это реальный рефакторинг в соответствии с определением рефакторинга; Я собираюсь реорганизовать этот код. Я собираюсь изменить его расположение, не меняя его поведение.

Это более консервативный подход; если требования могут быть недооценены, если тесты не отражают всю существующую логику, то вы должны быть очень и очень осторожны в том, как будете действовать.

Наверняка вы можете утверждать, что если тесты неадекватно описывают поведение системы, то у вас «провал тестирования». И я думаю, что это справедливо - но на самом деле не полезно; это обычная проблема в дикой природе.

Или провал рефакторинга - потому что код должен был быть преобразован шаг за шагом, а не переписан с нуля?

Проблема не совсем в том, что преобразования должны были быть пошаговыми; скорее, выбор инструмента рефакторинга (оператор клавиатуры? а не управляемая автоматизация) не соответствовал охвату тестами из-за более высокого уровня ошибок.

Эту проблему можно было бы решить либо с помощью инструментов рефакторинга с более высокой надежностью, либо путем введения более широкого набора тестов для улучшения ограничений в системе.

Поэтому я думаю, что ваше соединение плохо выбрано; ANDне OR.

VoiceOfUnreason
источник
2

Рефакторинг не должен изменять внешне видимое поведение вашего кода. Это цель.

Если ваши юнит-тесты не пройдены, это означает, что вы изменили поведение. Но прохождение юнит-тестов никогда не является целью. Это помогает более или менее достичь вашей цели. Если рефакторинг изменяет внешне видимое поведение и все модульные тесты проходят успешно, то ваш рефакторинг не удался.

Рабочие юнит-тесты в этом случае только дают неверное ощущение успеха. Но что пошло не так? Две вещи: рефакторинг был небрежным, а юнит-тесты были не очень хорошими.

gnasher729
источник
1

Если вы определили «правильный» как «тест пройден», то по определению нет ничего плохого в том, чтобы изменить непроверенное поведение.

Если определенное поведение ребра должно быть определено, добавьте тест для него, если нет, тогда все в порядке, чтобы не заботиться о том, что происходит. Если вы действительно педантичны, вы можете написать тест, который проверяет, trueкогда в этом крайнем случае документирует, что вас не волнует, каково поведение.

Caleth
источник