Para obter código de produção de alta qualidade, não basta fornecer a máxima cobertura de teste. Sem dúvida, para alcançar altos resultados, o código e os testes principais do projeto devem funcionar em um conjunto perfeitamente coeso. Portanto, você precisa prestar atenção nos testes, tanto quanto no código principal. Escrever um bom teste é a chave para obter uma regressão na produção. Para mostrar a importância do fato de que os erros nos testes não são piores que na produção, consideraremos a próxima análise de avisos do analisador estático PVS-Studio. Alvo: Apache Hadoop.
Sobre o projeto
Aqueles que já se interessaram por Big Data provavelmente já ouviram ou trabalharam em um projeto como o
Apache Hadoop . Em resumo, o Hadoop é uma estrutura que pode ser usada como base para criar e trabalhar com sistemas de Big Data.
O Hadoop consiste em quatro módulos principais, cada um dos quais executa uma tarefa específica necessária para um sistema de análise de big data:
- Hadoop comum
- Mapreduce
- Sistema de arquivos distribuídos do Hadoop (Sistema de arquivos distribuídos do Hadoop)
- Fios
No entanto, existem muitos materiais para se familiarizar com ele na Internet.
Sobre a verificação
Conforme mostrado na
documentação , o PVS-Studio pode ser integrado ao projeto de várias maneiras:
- usando o plugin maven;
- usando o plugin gradle;
- Usando IntellJ IDEA
- usando o analisador diretamente.
O Hadoop foi construído com base no sistema de compilação maven, portanto não houve dificuldades com a verificação.
Tendo integrado o script da documentação e ajustado um pouco do pom.xml (havia módulos nas dependências que não estavam lá), a análise foi feita!
Após a análise, escolhendo os avisos mais interessantes, notei que tinha o mesmo número de avisos no código de produção e nos testes. Normalmente, não considero o acionador do analisador que cai nos testes. Mas, dividindo-os, não pude perder os avisos da categoria 'testes' que passaram pela minha atenção. "Por que não?", Pensei, porque os erros nos testes também têm consequências. Eles podem levar a testes incorretos ou parciais, ou até a bobagens (apenas para exibição, para que sejam sempre verdes).
Portanto, depois de coletar os avisos mais interessantes, dividindo-os por código (produção, teste) e os quatro módulos principais do Hadoop, trago à sua atenção uma análise das operações do analisador.
Código de produção
Hadoop comum
V6033 Um item com a mesma chave 'KDC_BIND_ADDRESS' já foi adicionado. MiniKdc.java (163), MiniKdc.java (162)
public class MiniKdc { .... private static final Set<String> PROPERTIES = new HashSet<String>(); .... static { PROPERTIES.add(ORG_NAME); PROPERTIES.add(ORG_DOMAIN); PROPERTIES.add(KDC_BIND_ADDRESS); PROPERTIES.add(KDC_BIND_ADDRESS);
Um valor adicionado duas vezes a um
HashSet é um defeito comum na verificação de projetos. De fato, a segunda adição será ignorada. Bem, se essa duplicação for um acidente absurdo. Mas e se realmente significasse adicionar outro valor?
Mapreduce
V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'localFiles' deva ser usada em vez de 'localArchives'. LocalDistributedCacheManager.java (183), LocalDistributedCacheManager.java (178), LocalDistributedCacheManager.java (176), LocalDistributedCacheManager.java (181)
public synchronized void setup(JobConf conf, JobID jobId) throws IOException { ....
O Diagnostics V6072 faz algumas vezes descobertas muito interessantes. A essência do diagnóstico é procurar o mesmo tipo de fragmento de código obtido por copiar-colar e substituir uma ou duas variáveis, mas ao mesmo tempo algumas variáveis foram "subestimadas".
O código acima demonstra isso. No primeiro bloco, as ações são executadas com a variável
localArchives , no próximo bloco do mesmo tipo, com
localFiles . E se você estuda conscientemente esse código e não o
repassa rapidamente, como costuma ser o caso com a revisão de código, observe o local em que você esqueceu de substituir a variável
localArchives .
Essa supervisão pode levar ao seguinte cenário:
- Suponha que tenhamos arquivos locais (tamanho = 4) e arquivos locais (tamanho = 2);
- Ao criar a matriz localFiles.toArray (nova String [localArchives.size ()]) , obtemos os dois últimos elementos nulos (["pathToFile1", "pathToFile2", null, null]);
- Depois disso, org.apache.hadoop.util.StringUtils.arrayToString retornará uma representação de string de nossa matriz na qual os últimos nomes de arquivos serão representados como "nulos" ("pathToFile1, pathToFile2, null, null" ) ;
- Tudo isso será repassado e quem sabe quais verificações existem para esses casos =).
A expressão
V6007 'children.size ()> 0' sempre é verdadeira. Queue.java (347)
boolean isHierarchySameAs(Queue newState) { .... if (children == null || children.size() == 0) { .... } else if(children.size() > 0) { .... } .... }
Devido ao fato de que a verificação do número de elementos em 0 é feita separadamente, a verificação adicional de
children.size ()> 0 sempre será verdadeira.
HDFS
V6001 Existem
subexpressões idênticas 'this.bucketSize' à esquerda e à direita do operador '%'. RollingWindow.java (79)
RollingWindow(int windowLenMs, int numBuckets) { buckets = new Bucket[numBuckets]; for (int i = 0; i < numBuckets; i++) { buckets[i] = new Bucket(); } this.windowLenMs = windowLenMs; this.bucketSize = windowLenMs / numBuckets; if (this.bucketSize % bucketSize != 0) {
Esse defeito reside no fato de que a variável é dividida em si mesma. Como resultado, a verificação da multiplicidade sempre será aprovada e, no caso de dados de entrada incorretos (
windowLenMs ,
numBuckets ), a exceção não será lançada.
Fios
V6067 Dois ou mais ramificações de caso executam as mesmas ações. TimelineEntityV2Converter.java (386), TimelineEntityV2Converter.java (389)
public static ApplicationReport convertToApplicationReport(TimelineEntity entity) { .... if (metrics != null) { long vcoreSeconds = 0; long memorySeconds = 0; long preemptedVcoreSeconds = 0; long preemptedMemorySeconds = 0; for (TimelineMetric metric : metrics) { switch (metric.getId()) { case ApplicationMetricsConstants.APP_CPU_METRICS: vcoreSeconds = getAverageValue(metric.getValues().values()); break; case ApplicationMetricsConstants.APP_MEM_METRICS: memorySeconds = ....; break; case ApplicationMetricsConstants.APP_MEM_PREEMPT_METRICS: preemptedVcoreSeconds = ....;
Nos dois ramos do
caso , o mesmo código fragmenta. Isso acontece o tempo todo! No número predominante de casos, esse não é um erro real, mas apenas uma ocasião para pensar em refatoração de
comutador . Mas não para o caso em questão. Na repetição de trechos de código, o valor da variável
preemptedVcoreSeconds é definido . Se você prestar atenção aos nomes de todas as variáveis e constantes, poderá concluir que, no caso de
metric.getId () == APP_MEM_PREEMPT_METRICS , o valor da variável
preemptedMemorySeconds deve ser definido, não
preemptedVcoreSeconds . Nesse sentido,
preemptedMemorySeconds sempre permanecerá 0 após a execução da instrução 'switch', e o valor de
preemptedVcoreSeconds pode estar incorreto.
V6046 Formato incorreto. É esperado um número diferente de itens de formato. Argumentos não utilizados: 2. AbstractSchedulerPlanFollower.java (186)
@Override public synchronized void synchronizePlan(Plan plan, boolean shouldReplan) { .... try { setQueueEntitlement(planQueueName, ....); } catch (YarnException e) { LOG.warn("Exception while trying to size reservation for plan: {}", currResId, planQueueName, e); } .... }
Variável não utilizada
planQueueName durante o log. Aqui, eles copiaram demais ou não modificaram a string de formato. Mas, no entanto, sou inclinado ao bom e velho serviço de copiar e colar, às vezes baixo.
Código de teste
Hadoop comum
V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'allSecretsB' deva ser usada em vez de 'allSecretsA'. TestZKSignerSecretProvider.java (316), TestZKSignerSecretProvider.java (309), TestZKSignerSecretProvider.java (306), TestZKSignerSecretProvider.java (313)
public void testMultiple(int order) throws Exception { .... currentSecretA = secretProviderA.getCurrentSecret(); allSecretsA = secretProviderA.getAllSecrets(); Assert.assertArrayEquals(secretA2, currentSecretA); Assert.assertEquals(2, allSecretsA.length);
E novamente V6072. Cuidado com as variáveis
allSecretsA e
allSecretsB .
V6043 Considere inspecionar o operador 'for'. Os valores inicial e final do iterador são os mesmos. TestTFile.java (235)
private int readPrepWithUnknownLength(Scanner scanner, int start, int n) throws IOException { for (int i = start; i < start; i++) { String key = String.format(localFormatter, i); byte[] read = readKey(scanner); assertTrue("keys not equal", Arrays.equals(key.getBytes(), read)); try { read = readValue(scanner); assertTrue(false); } catch (IOException ie) {
Um teste sempre verde? =). O corpo do loop, que faz parte do teste, nunca é executado. Isso se deve ao fato de os valores inicial e final do contador corresponderem na instrução
for . Como resultado, a condição
i <start imediatamente nos fornecerá false, o que levará a esse comportamento. Corri o arquivo com os testes e cheguei à conclusão de que era necessário escrever na condição do loop
i <(start + n) .
Mapreduce
a href = "
www.viva64.com/en/w/v6007 "> A expressão V6007 'byteAm <0' é sempre falsa. DataWriter.java (322)
GenerateOutput writeSegment(long byteAm, OutputStream out) throws IOException { long headerLen = getHeaderLength(); if (byteAm < headerLen) {
A condição
byteAm <0 é sempre falsa. Para entender, vamos subir o código acima. Se a execução do teste atingir a operação
byteAm - = headerLen , isso significa que haverá
byteAm> = headerLen . A partir daqui, após executar a subtração, o valor de
byteAm nunca será negativo. O que era necessário para provar.
HDFS
V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'normalFile' deva ser usada em vez de 'normalDir'. TestWebHDFS.java (625), TestWebHDFS.java (615), TestWebHDFS.java (614), TestWebHDFS.java (624)
public void testWebHdfsErasureCodingFiles() throws Exception { .... final Path normalDir = new Path("/dir"); dfs.mkdirs(normalDir); final Path normalFile = new Path(normalDir, "file.log"); ....
Não acredito nisso, e novamente V6072! Basta seguir as
variáveis normalDir e
normalFileV6027 As variáveis são inicializadas através da chamada para a mesma função. Provavelmente é um erro ou código não otimizado. TestDFSAdmin.java (883), TestDFSAdmin.java (879)
private void verifyNodesAndCorruptBlocks( final int numDn, final int numLiveDn, final int numCorruptBlocks, final int numCorruptECBlockGroups, final DFSClient client, final Long highestPriorityLowRedundancyReplicatedBlocks, final Long highestPriorityLowRedundancyECBlocks) throws IOException { .... final String expectedCorruptedECBlockGroupsStr = String.format( "Block groups with corrupt internal blocks: %d", numCorruptECBlockGroups); final String highestPriorityLowRedundancyReplicatedBlocksStr = String.format( "\tLow redundancy blocks with highest priority " + "to recover: %d", highestPriorityLowRedundancyReplicatedBlocks); final String highestPriorityLowRedundancyECBlocksStr = String.format( "\tLow redundancy blocks with highest priority " + "to recover: %d", highestPriorityLowRedundancyReplicatedBlocks); .... }
Nesse fragmento, as variáveis
maximumPriorityLowRedundancyReplicatedBlocksStr e
maximumPriorityLowRedundancyECBlocksStr são inicializadas com os mesmos valores. Freqüentemente deveria ser, mas não nessa situação. Os nomes das variáveis aqui são longos e semelhantes entre si, portanto, não estou surpreso por não haver modificações correspondentes com copiar e colar. Para solucionar a situação, ao inicializar a variável
maximumPriorityLowRedundancyECBlocksStr, você deve usar o parâmetro de entrada
maximumPriorityLowRedundancyECBlocks . Além disso, provavelmente, você ainda precisa corrigir a string de formato.
V6019 Código inacessível detectado. É possível que haja um erro. TestReplaceDatanodeFailureReplication.java (222)
private void verifyFileContent(...., SlowWriter[] slowwriters) throws IOException { LOG.info("Verify the file"); for (int i = 0; i < slowwriters.length; i++) { LOG.info(slowwriters[i].filepath + ....); FSDataInputStream in = null; try { in = fs.open(slowwriters[i].filepath); for (int j = 0, x;; j++) { x = in.read(); if ((x) != -1) { Assert.assertEquals(j, x); } else { return; } } } finally { IOUtils.closeStream(in); } } }
O analisador jura que a alteração do contador
i ++ no loop é inatingível. Isso significa que em um loop
for (int i = 0; i <slowwriters.length; i ++) {....}, não será executada mais de uma iteração. Vamos descobrir o porquê. Portanto, na primeira iteração, associamos o fluxo ao arquivo correspondente a
slowwriters [0] para leitura adicional. Através do loop
for (int j = 0, x ;; j ++), lemos o conteúdo do arquivo por byte, onde:
- se lemos algo adequado, por meio de assertEquals, comparamos o byte de leitura com o valor atual do contador j (no caso de verificação sem êxito, encerramos o teste com falha);
- se o arquivo passou no teste e chegamos ao final do arquivo (leia -1), sairemos do método.
Portanto, não importa o que aconteça ao verificar
slowwriters [0] , ele não chegará à verificação dos seguintes elementos. Provavelmente, deve-se usar o
intervalo em vez do
retorno .
Fios
V6019 Código inacessível detectado. É possível que haja um erro. TestNodeManager.java (176)
@Test public void testCreationOfNodeLabelsProviderService() throws InterruptedException { try { .... } catch (Exception e) { Assert.fail("Exception caught"); e.printStackTrace(); } }
Nessa situação, o stacktrace nunca será impresso se ocorrer uma exceção, porque o método
Assert.fail interromperá o teste. Se houver uma mensagem suficiente de que a exceção foi detectada, para não confundir, a impressão de rastreamento de pilha precisa ser removida. Se for necessário imprimir, basta trocá-los.
Existem muitos lugares assim:
- V6019 Código inacessível detectado. É possível que haja um erro. TestResourceTrackerService.java (928)
- V6019 Código inacessível detectado. É possível que haja um erro. TestResourceTrackerService.java (737)
- V6019 Código inacessível detectado. É possível que haja um erro. TestResourceTrackerService.java (685)
- ....
V6072 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'publicCache' deva ser usada em vez de 'usercache'. TestResourceLocalizationService.java (315), TestResourceLocalizationService.java (309), TestResourceLocalizationService.java (307), TestResourceLocalizationService.java (317)
@Test public void testDirectoryCleanupOnNewlyCreatedStateStore() throws IOException, URISyntaxException { ....
E, finalmente, novamente V6072 =). Variáveis para se familiarizar com o snippet suspeito:
usercache e
publicCache .
Conclusão
Durante o desenvolvimento, centenas de milhares de linhas de código são escritas. Se o código de produção está tentando se manter limpo de bugs, defeitos e deficiências (o desenvolvedor testa seu próprio código, realiza uma revisão de código e muito mais), os testes são claramente inferiores a isso. Defeitos nos testes podem se esconder silenciosamente atrás de um "sinal verde". E como você entende pela análise atual dos avisos, um teste aprovado com sucesso está longe de ser sempre um teste garantido.
Ao verificar a base de código do Apache Hadoop, a análise estática demonstrou sua necessidade não apenas do código que entra em produção, mas também de testes que também desempenham um papel importante no desenvolvimento.
Portanto, se você se preocupa com a qualidade do seu código e da base de testes, recomendo que analise a estática. E o primeiro candidato ao teste, proponho experimentar o
PVS-Studio .
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Maxim Stefanov.
Qualidade do código do Apache Hadoop: teste de produção vs.