Refatoração - é apropriado simplesmente reescrever o código, desde que todos os testes passem?

9

Eu assisti recentemente "All the Little Things" do RailsConf 2014. Durante essa palestra, Sandi Metz refatora uma função que inclui uma declaração if aninhada grande:

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

O primeiro passo é dividir a função em várias menores:

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

O que achei interessante foi a maneira como essas funções menores foram escritas. brie_tick, por exemplo, não foi escrito extraindo as partes relevantes da tickfunção original , mas a partir do zero, consultando os test_brie_*testes de unidade. Depois que todos esses testes de unidade foram aprovados, brie_tickfoi considerado feito. Depois que todas as pequenas funções foram concluídas, a tickfunção monolítica original foi excluída.

Infelizmente, o apresentador parecia não ter consciência de que essa abordagem fazia com que três das quatro *_tickfunções estivessem erradas (e a outra estava vazia!). Existem casos extremos nos quais o comportamento das *_tickfunções difere do da tickfunção original . Por exemplo, @days_remaining <= 0em brie_tickdeve ser < 0- portanto brie_tick, não funciona corretamente quando chamado com days_remaining == 1e quality < 50.

O que deu errado aqui? Isso é uma falha no teste - porque não houve testes para esses casos específicos? Ou uma falha na refatoração - porque o código deveria ter sido transformado passo a passo em vez de reescrito do zero?

user200783
fonte
2
Não tenho certeza se entendi a pergunta. Claro que não há problema em reescrever o código. Não sei ao certo o que você quer dizer com "está tudo bem em simplesmente reescrever código". Se você está perguntando "Tudo bem reescrever o código sem pensar muito nele", a resposta é não, assim como não é correto escrever o código dessa maneira.
John Wu
Isso acontece frequentemente devido a planos de teste focados principalmente em testar casos de uso de sucesso e muito pouco (ou nada) em cobrir casos de uso de erro ou subutilizados. Portanto, é principalmente um vazamento de cobertura. Um vazamento de teste.
24518 Laiv
@JohnWu - Fiquei com a impressão de que a refatoração geralmente era feita como uma série de pequenas transformações no código-fonte ("método de extração" etc.) em vez de simplesmente reescrever o código (com o que quero dizer escrevê-lo novamente do zero, sem nem mesmo olhando para o código existente, como feito na apresentação vinculada).
user200783
@ JohnWu - Reescrever do zero é uma técnica aceitável de refatoração? Caso contrário, é decepcionante ver uma apresentação tão conceituada sobre refatoração seguir essa abordagem. OTOH, se for aceitável, as mudanças de comportamento não intencionais podem ser atribuídas aos testes ausentes - mas existe alguma maneira de ter certeza de que os testes cobrem todos os possíveis casos extremos?
user200783
@ User200783 Bem, essa é uma pergunta maior, não é (como posso garantir que meus testes sejam abrangentes?) Pragmaticamente, provavelmente eu executaria um relatório de cobertura de código antes de fazer alterações e examinaria cuidadosamente quaisquer áreas do código que não exercite-se, garantindo que a equipe de desenvolvimento esteja consciente deles enquanto reescreve a lógica.
John Wu

Respostas:

11

Isso é uma falha no teste - porque não houve testes para esses casos específicos? Ou uma falha na refatoração - porque o código deveria ter sido transformado passo a passo em vez de reescrito do zero?

Ambos. A refatoração usando apenas as etapas padrão do livro original de Fowlers é definitivamente menos propensa a erros do que a reescrita, portanto, é preferível usar apenas esses tipos de etapas de bebê. Mesmo que não haja testes de unidade para todos os casos extremos, e mesmo que o ambiente não forneça refatorações automáticas, uma única alteração de código como "introduzir variável de explicação" ou "função de extração" tem uma chance muito menor de alterar os detalhes de comportamento do código existente do que uma reescrita completa de uma função.

Às vezes, no entanto, reescrever uma seção do código é o que você precisa ou deseja fazer. E se for esse o caso, você precisa de melhores testes.

Observe que, mesmo ao usar uma ferramenta de refatoração, sempre existe um certo risco de introdução de erros ao alterar o código, independentemente da aplicação de etapas menores ou maiores. É por isso que a refatoração sempre precisa de testes. Observe também que os testes podem apenas reduzir a probabilidade de erros, mas nunca provar sua ausência - no entanto, o uso de técnicas como olhar o código e a cobertura da ramificação pode fornecer um alto nível de confiança e, no caso de uma reescrita de uma seção de código, é muitas vezes vale a pena aplicar essas técnicas.

Doc Brown
fonte
11
Obrigado, isso faz sentido. Portanto, se a solução definitiva para mudanças indesejáveis ​​no comportamento é ter testes abrangentes, existe alguma maneira de ter certeza de que os testes cobrem todos os possíveis casos extremos? Por exemplo, seria possível ter 100% de cobertura brie_tickenquanto ainda não estiver testando o @days_remaining == 1caso problemático , por exemplo, testando com @days_remainingdefinido como 10e -10.
user200783
2
Você nunca pode ter certeza absoluta de que os testes abrangem todos os casos extremos possíveis, pois não é possível testar com todas as entradas possíveis. Mas existem muitas maneiras de ganhar mais confiança nos testes. Você pode analisar o teste de mutação , que é uma maneira de testar a eficácia dos testes.
Bdsl # 24/18
11
Nesse caso, as ramificações perdidas poderiam ter sido capturadas com uma ferramenta de cobertura de código durante o desenvolvimento dos testes.
Cbojar
2

O que deu errado aqui? Isso é uma falha no teste - porque não houve testes para esses casos específicos? Ou uma falha na refatoração - porque o código deveria ter sido transformado passo a passo em vez de reescrito do zero?

Uma das coisas realmente difíceis de trabalhar com o código herdado: adquirir uma compreensão completa do comportamento atual.

Código legado sem testes que restringem todos os comportamentos é um padrão comum na natureza. O que deixa você com um palpite: isso significa que os comportamentos irrestritos são variáveis ​​livres? ou requisitos não especificados?

Da palestra :

Agora, isso é refatoração real, de acordo com a definição de refatoração; Vou refatorar esse código. Vou mudar seu arranjo sem alterar seu comportamento.

Essa é a abordagem mais conservadora; se os requisitos puderem ser subespecificados, se os testes não capturarem toda a lógica existente, você deverá ter muito cuidado com a maneira como proceder.

Por certo, você pode afirmar que, se os testes descrevem inadequadamente o comportamento do sistema, há uma "falha no teste". E acho que é justo - mas não é realmente útil; esse é um problema comum de se ter na natureza.

Ou uma falha na refatoração - porque o código deveria ter sido transformado passo a passo em vez de reescrito do zero?

A questão não é que as transformações devam ter sido passo a passo; mas antes a escolha da ferramenta de refatoração (operador de teclado humano - em vez da automação guiada) não estava bem alinhada com a cobertura do teste, devido à maior taxa de erros.

Isto poderia ter sido dirigida quer usando ferramentas de refatoração com maior fiabilidade ou pela introdução de uma bateria de testes mais amplo para melhorar as limitações do sistema.

Então eu acho que sua conjunção é mal escolhida; ANDnão OR.

VoiceOfUnreason
fonte
2

A refatoração não deve alterar o comportamento visível externamente do seu código. Esse é o objetivo.

Se seus testes de unidade falharem, isso indica que você mudou o comportamento. Mas passar nos testes de unidade nunca é o objetivo. Ajuda mais ou menos a alcançar seu objetivo. Se a refatoração alterar o comportamento visível externamente e todos os testes de unidade forem aprovados, a refatoração falhará.

Os testes de unidade de trabalho nesse caso apenas dão a você a sensação errada de sucesso. Mas o que deu errado? Duas coisas: a refatoração foi descuidada e os testes de unidade não foram muito bons.

gnasher729
fonte
1

Se você definir "correto" como "os testes passam", por definição , não é errado alterar o comportamento não testado.

Se um comportamento específico da borda deve ser definido, adicione um teste, caso contrário, não há problema em não se importar com o que acontece. Se você é realmente pedante, pode escrever um teste que verifique truequando, nesse caso extremo, para documentar que não se importa com o comportamento.

Caleth
fonte