Чтобы получить качественный производственный код, недостаточно просто обеспечить максимальное покрытие тестами. Без сомнения, для достижения хороших результатов требуется эффективная совместная работа основного кода проекта и тестов. Поэтому тестам нужно уделять столько же внимания, сколько и исходному коду. Достойный тест — ключевой фактор успеха, поскольку он уловит регрессию в производстве. Давайте посмотрим на предупреждения статического анализатора PVS-Studio, чтобы увидеть важность того факта, что ошибки в тестах не хуже, чем в работе. Сегодняшний фокус: Apache Hadoop. 
О проекте
Те, кто раньше интересовался большими данными, вероятно, слышали или работали с проектом Apache Hadoop . В двух словах, Hadoop — это фреймворк, который можно использовать как основу для построения и работы с системами больших данных.
Hadoop состоит из четырех основных модулей; каждый из них выполняет определенную задачу, необходимую для системы анализа больших данных:
О чеке
Как показано в документации, PVS-Studio можно интегрировать в проект различными способами:
- Использование плагина Maven.
- Использование плагина Gradle.
- Использование Gradle IntellJ IDEA.
- Непосредственно с помощью анализатора.
Hadoop основан на системе сборки Maven; следовательно, при проверке не было никаких препятствий.
После того, как я интегрировал скрипт из документации и отредактировал один из файлов pom.xml (в зависимостях были модули, которые были недоступны), анализ начался!
После завершения анализа я выбрал наиболее интересные предупреждения и заметил, что у меня было одинаковое количество предупреждений в рабочем коде и в тестах. Обычно я не рассматриваю предупреждения анализатора от тестов. Но когда я разделил их, я не мог оставить предупреждения «тесты» без присмотра. «Почему бы не взглянуть на них, — подумал я, — потому что ошибки в тестах также могут иметь неблагоприятные последствия». Они могут привести к неправильному или частичному тестированию или даже к путанице.
После того, как я выбрал самые интригующие предупреждения, я разделил их на следующие группы: производство, тестирование и четыре основных модуля Hadoop. И теперь я рад предложить обзор предупреждений анализатора.
Вам также может понравиться:
Учебное пособие по Kafka для всех, не имеет значения ваш этап в развитии .
Код производства
Hadoop Common
V6033 Элемент с тем же ключом ‘KDC_BIND_ADDRESS’ уже добавлен. MiniKdc.java (163), MiniKdc.java (162)
Джава
xxxxxxxxxx
1
public class MiniKdc {
2
....
3
private static final Set<String> PROPERTIES = new HashSet<String>();
4
....
5
static {
6
PROPERTIES.add(ORG_NAME);
7
PROPERTIES.add(ORG_DOMAIN);
8
PROPERTIES.add(KDC_BIND_ADDRESS);
9
PROPERTIES.add(KDC_BIND_ADDRESS); // <=
10
PROPERTIES.add(KDC_PORT);
11
PROPERTIES.add(INSTANCE);
12
....
13
}
14
....
15
}
Значение, добавленное дважды в a, HashSetявляется очень распространенным дефектом при проверке проектов. Второе дополнение будет проигнорировано. Я надеюсь, что это дублирование просто ненужная трагедия. Что, если было добавлено другое значение?
Уменьшение карты
V6072 Два похожих фрагмента кода были найдены. Возможно, это опечатка, и localFilesвместо нее следует использовать переменную localArchives.
- LocalDistributedCacheManager.java (183).
- LocalDistributedCacheManager.java (178).
- LocalDistributedCacheManager.java (176).
- LocalDistributedCacheManager.java (181).
Джава
xxxxxxxxxx
1
public synchronized void setup(JobConf conf, JobID jobId) throws IOException {
2
....
3
// Update the configuration object with localized data.
4
if (!localArchives.isEmpty()) {
5
conf.set(MRJobConfig.CACHE_LOCALARCHIVES, StringUtils
6
.arrayToString(localArchives.toArray(new String[localArchives // <=
7
.size()])));
8
}
9
if (!localFiles.isEmpty()) {
10
conf.set(MRJobConfig.CACHE_LOCALFILES, StringUtils
11
.arrayToString(localFiles.toArray(new String[localArchives // <=
12
.size()])));
13
}
14
....
15
}
Диагностика V6072 иногда дает некоторые интересные результаты. Цель этой диагностики — обнаружить фрагменты кода того же типа, которые возникли в результате копирования-вставки и замены одной-двух переменных. В этом случае некоторые переменные даже остались «недооцененными».
Код выше демонстрирует это. В первом блоке localArchivesпеременная используется во втором аналогичном фрагменте — localFiles. Если вы изучите этот код с должной осмотрительностью, а не быстро пройдете его, как это часто бывает при просмотре кода, вы заметите фрагмент, где автор забыл заменить localArchivesпеременную.
Эта ошибка может привести к следующему сценарию:
- Предположим, у нас есть localArchives (size = 4) и localFiles (size = 2).
- При создании массива
localFiles.toArray(new String[localArchives.size()])последние два элемента будут иметь значение null (["pathToFile1", "pathToFile2", null, null]). - Затем
org.apache.hadoop.util.StringUtils.arrayToStringбудет возвращено строковое представление нашего массива, в котором имена последних файлов будут представлены как «нулевые» («pathToFile1, pathToFile2, null, null»). - Все это будет передано дальше, и Бог знает только, какие проверки существуют для таких случаев =).
V6007 Выражение ‘children.size ()> 0’ всегда верно. Queue.java (347)
Джава
xxxxxxxxxx
1
boolean isHierarchySameAs(Queue newState) {
2
....
3
if (children == null || children.size() == 0) {
4
....
5
}
6
else if(children.size() > 0)
7
{
8
....
9
}
10
....
11
}
В связи с тем, что количество элементов проверяется на 0 отдельно, дальнейшая проверка children.size ()> 0 всегда будет истинной.
HDFS
V6001 Слева и справа от оператора «%» есть идентичные подвыражения «this.bucketSize». RollingWindow.java (79).
Джава
xxxxxxxxxx
1
RollingWindow(int windowLenMs, int numBuckets) {
2
buckets = new Bucket[numBuckets];
3
for (int i = 0; i < numBuckets; i++) {
4
buckets[i] = new Bucket();
5
}
6
this.windowLenMs = windowLenMs;
7
this.bucketSize = windowLenMs / numBuckets;
8
if (this.bucketSize % bucketSize != 0) { // <=
9
throw new IllegalArgumentException(
10
"The bucket size in the rolling window is not integer: windowLenMs= "
11
+ windowLenMs + " numBuckets= " + numBuckets);
12
}
13
}
ПРЯЖА
V6067 Две или более ветвей дел выполняют одинаковые действия. TimelineEntityV2Converter.java (386), TimelineEntityV2Converter.java (389).
Джава
xxxxxxxxxx
1
public static ApplicationReport
2
convertToApplicationReport(TimelineEntity entity)
3
{
4
....
5
if (metrics != null) {
6
long vcoreSeconds = 0;
7
long memorySeconds = 0;
8
long preemptedVcoreSeconds = 0;
9
long preemptedMemorySeconds = 0;
10
11
for (TimelineMetric metric : metrics) {
12
switch (metric.getId()) {
13
case ApplicationMetricsConstants.APP_CPU_METRICS:
14
vcoreSeconds = getAverageValue(metric.getValues().values());
15
break;
16
case ApplicationMetricsConstants.APP_MEM_METRICS:
17
memorySeconds = ....;
18
break;
19
case ApplicationMetricsConstants.APP_MEM_PREEMPT_METRICS:
20
preemptedVcoreSeconds = ....; // <=
21
break;
22
case ApplicationMetricsConstants.APP_CPU_PREEMPT_METRICS:
23
preemptedVcoreSeconds = ....; // <=
24
break;
25
default:
26
// Should not happen..
27
break;
28
}
29
}
30
....
31
}
32
....
33
}
Одинаковые фрагменты кода находятся в двух ветвях. Это просто повсюду! В большинстве случаев это не настоящая ошибка, а просто повод задуматься о рефакторинге оператора switch. Это не верно для рассматриваемого случая. Повторяющиеся фрагменты кода устанавливают значение переменной preemptedVcoreSeconds. Если вы внимательно посмотрите на имена всех переменных и констант, вы, вероятно, придете к выводу, что в этом случае if metric.getId() == APP_MEM_PREEMPT_METRICSзначение должно быть установлено для preemptedMemorySecondsпеременной, а не для preemptedVcoreSeconds. В связи с этим после оператора switch preemptedMemorySecondsвсегда останется 0, а значение preemptedVcoreSecondsможет быть неправильным.
V6046 Неверный формат. Ожидается другое количество элементов формата. Аргументы не используются: 2. AbstractSchedulerPlanFollower.java (186)
Джава
xxxxxxxxxx
1
2
public synchronized void synchronizePlan(Plan plan, boolean shouldReplan)
3
{
4
....
5
try
6
{
7
setQueueEntitlement(planQueueName, ....);
8
}
9
catch (YarnException e)
10
{
11
LOG.warn("Exception while trying to size reservation for plan: {}",
12
currResId,
13
planQueueName,
14
e);
15
}
16
....
17
}
planQueueNameПеременная не используется при входе. В этом случае либо слишком много копируется, либо строка формата не завершена. Но я все еще виню старую добрую копировальную пасту, которая в некоторых случаях просто великолепна, чтобы выстрелить себе в ногу.
Тестовый код
Hadoop Common
V6072 Два похожих фрагмента кода были найдены. Возможно, это опечатка, и вместо ‘allSecretsA’ следует использовать переменную ‘allSecretsB’.
TestZKSignerSecretProvider.java (316), TestZKSignerSecretProvider.java (309), TestZKSignerSecretProvider.java (306), TestZKSignerSecretProvider.java (313).
Джава
xxxxxxxxxx
1
public void testMultiple(int order) throws Exception {
2
....
3
currentSecretA = secretProviderA.getCurrentSecret();
4
allSecretsA = secretProviderA.getAllSecrets();
5
Assert.assertArrayEquals(secretA2, currentSecretA);
6
Assert.assertEquals(2, allSecretsA.length); // <=
7
Assert.assertArrayEquals(secretA2, allSecretsA[0]);
8
Assert.assertArrayEquals(secretA1, allSecretsA[1]);
9
10
currentSecretB = secretProviderB.getCurrentSecret();
11
allSecretsB = secretProviderB.getAllSecrets();
12
Assert.assertArrayEquals(secretA2, currentSecretB);
13
Assert.assertEquals(2, allSecretsA.length); // <=
14
Assert.assertArrayEquals(secretA2, allSecretsB[0]);
15
Assert.assertArrayEquals(secretA1, allSecretsB[1]);
16
....
17
}
И снова V6072. Посмотрите внимательно на переменные allSecretsAи allSecretsB.
V6043 Рассмотрите возможность проверки оператора for. Начальные и конечные значения итератора одинаковы. TestTFile.java (235).
Джава
xxxxxxxxxx
1
private int readPrepWithUnknownLength(Scanner scanner, int start, int n)
2
throws IOException {
3
for (int i = start; i < start; i++) {
4
String key = String.format(localFormatter, i);
5
byte[] read = readKey(scanner);
6
assertTrue("keys not equal", Arrays.equals(key.getBytes(), read));
7
try {
8
read = readValue(scanner);
9
assertTrue(false);
10
}
11
catch (IOException ie) {
12
// should have thrown exception
13
}
14
String value = "value" + key;
15
read = readLongValue(scanner, value.getBytes().length);
16
assertTrue("values nto equal", Arrays.equals(read, value.getBytes()));
17
scanner.advance();
18
}
19
return (start + n);
20
}
Тест всегда зеленый? знак равно Часть цикла, которая является частью самого теста, никогда не будет выполнена. Это связано с тем, что начальные и конечные значения счетчика равны в forзаявлении. В результате условие i < startнемедленно станет ложным, что приведет к такому поведению. Я пробежал по тестовому файлу и пришел к выводу, что i < (start + n)его нужно записать в условии цикла.
Уменьшение карты
V6007 Выражение «byteAm <0» всегда ложно. DataWriter.java (322)
Джава
xxxxxxxxxx
1
GenerateOutput writeSegment(long byteAm, OutputStream out)
2
throws IOException {
3
long headerLen = getHeaderLength();
4
if (byteAm < headerLen) {
5
// not enough bytes to write even the header
6
return new GenerateOutput(0, 0);
7
}
8
// adjust for header length
9
byteAm -= headerLen;
10
if (byteAm < 0) { // <=
11
byteAm = 0;
12
}
13
....
14
}
Условие byteAm < 0всегда ложно. Чтобы понять это, давайте взглянем на код выше. Если выполнение теста достигает операции byteAm -= headerLen, это означает, что byteAm >= headerLen. Отсюда, после вычитания, byteAmзначение никогда не будет отрицательным. Это то, что мы должны были доказать.
HDFS
V6072 Два похожих фрагмента кода были найдены. Возможно, это опечатка, и normalFileвместо нее следует использовать переменную normalDir. TestWebHDFS.java (625), TestWebHDFS.java (615), TestWebHDFS.java (614), TestWebHDFS.java (624)
Джава
xxxxxxxxxx
1
public void testWebHdfsErasureCodingFiles() throws Exception {
2
....
3
final Path normalDir = new Path("/dir");
4
dfs.mkdirs(normalDir);
5
final Path normalFile = new Path(normalDir, "file.log");
6
....
7
// logic block #1
8
FileStatus expectedNormalDirStatus = dfs.getFileStatus(normalDir);
9
FileStatus actualNormalDirStatus = webHdfs.getFileStatus(normalDir); // <=
10
Assert.assertEquals(expectedNormalDirStatus.isErasureCoded(),
11
actualNormalDirStatus.isErasureCoded());
12
ContractTestUtils.assertNotErasureCoded(dfs, normalDir);
13
assertTrue(normalDir + " should have erasure coding unset in " + ....);
14
15
// logic block #2
16
FileStatus expectedNormalFileStatus = dfs.getFileStatus(normalFile);
17
FileStatus actualNormalFileStatus = webHdfs.getFileStatus(normalDir); // <=
18
Assert.assertEquals(expectedNormalFileStatus.isErasureCoded(),
19
actualNormalFileStatus.isErasureCoded());
20
ContractTestUtils.assertNotErasureCoded(dfs, normalFile);
21
assertTrue( normalFile + " should have erasure coding unset in " + ....);
22
}
Верьте или нет, это снова V6072! Просто следуйте за переменной normalDirи normalFile.
V6027 Переменные инициализируются посредством вызова той же функции. Вероятно, это ошибка или неоптимизированный код. TestDFSAdmin.java (883), TestDFSAdmin.java (879).
Джава
xxxxxxxxxx
1
private void verifyNodesAndCorruptBlocks(
2
final int numDn,
3
final int numLiveDn,
4
final int numCorruptBlocks,
5
final int numCorruptECBlockGroups,
6
final DFSClient client,
7
final Long highestPriorityLowRedundancyReplicatedBlocks,
8
final Long highestPriorityLowRedundancyECBlocks)
9
throws IOException
10
{
11
/* init vars */
12
....
13
final String expectedCorruptedECBlockGroupsStr = String.format(
14
"Block groups with corrupt internal blocks: %d",
15
numCorruptECBlockGroups);
16
final String highestPriorityLowRedundancyReplicatedBlocksStr
17
= String.format(
18
"\tLow redundancy blocks with highest priority " +
19
"to recover: %d",
20
highestPriorityLowRedundancyReplicatedBlocks);
21
final String highestPriorityLowRedundancyECBlocksStr = String.format(
22
"\tLow redundancy blocks with highest priority " +
23
"to recover: %d",
24
highestPriorityLowRedundancyReplicatedBlocks);
25
....
26
}
В этом фрагменте highestPriorityLowRedundancyReplicatedBlocksStrи highestPriorityLowRedundancyECBlocksStrинициализируются одинаковые значения. Часто так и должно быть, но это не так. Имена переменных длинные и похожие, поэтому неудивительно, что скопированный фрагмент никак не изменился. Чтобы исправить это, при инициализации highestPriorityLowRedundancyECBlocksStrпеременной автор должен использовать входной параметр highestPriorityLowRedundancyECBlocks. Кроме того, скорее всего, им еще нужно исправить строку формата.
V6019 Обнаружен недоступный код. Возможно, что ошибка присутствует. TestReplaceDatanodeFailureReplication.java (222).
Джава
xxxxxxxxxx
1
private void
2
verifyFileContent(...., SlowWriter[] slowwriters) throws IOException
3
{
4
LOG.info("Verify the file");
5
for (int i = 0; i < slowwriters.length; i++) {
6
LOG.info(slowwriters[i].filepath + ....);
7
FSDataInputStream in = null;
8
try {
9
in = fs.open(slowwriters[i].filepath);
10
for (int j = 0, x;; j++) {
11
x = in.read();
12
if ((x) != -1) {
13
Assert.assertEquals(j, x);
14
} else {
15
return;
16
}
17
}
18
} finally {
19
IOUtils.closeStream(in);
20
}
21
}
22
}
Анализатор жалуется, что i++счетчик в цикле не может быть изменен. Это означает, что в for (int i = 0; i < slowwriters.length; i++) {....}цикле будет выполняться не более одной итерации. Давайте выясним, почему. На первой итерации мы связываем поток с файлом, который соответствует slowwriters[0]для дальнейшего чтения. Далее мы читаем содержимое файла через цикл for (int j = 0, x;; j++).
- Если мы читаем что-то релевантное, мы сравниваем прочитанный байт с текущим значением счетчика j, хотя
assertEquals(если проверка не удалась, мы проваливаем тест). - Если файл успешно проверен, и мы дошли до конца файла (мы читаем -1), метод завершается.
Поэтому, что бы ни происходило во время проверки slowwriters[0], оно не дойдет до проверки последующих элементов. Скорее всего, breakвместо должен быть использован return.
ПРЯЖА
V6019 Обнаружен недоступный код. Возможно, что ошибка присутствует. TestNodeManager.java (176)
Джава
xxxxxxxxxx
1
2
public void
3
testCreationOfNodeLabelsProviderService() throws InterruptedException {
4
try {
5
....
6
} catch (Exception e) {
7
Assert.fail("Exception caught");
8
e.printStackTrace();
9
}
10
}
В этой ситуации Assert.failметод прервет тест и трассировка стека не будет напечатана в случае исключения. Если здесь достаточно сообщения о перехваченном исключении, лучше удалить печать трассировки стека, чтобы избежать путаницы. Если печать необходима, вам просто нужно поменять их местами.
Было найдено много похожих фрагментов:
- V6019 Обнаружен недоступный код. Возможно, что ошибка присутствует. TestResourceTrackerService.java (928).
- V6019 Обнаружен недоступный код. Возможно, что ошибка присутствует. TestResourceTrackerService.java (737).
- V6019 Обнаружен недоступный код. Возможно, что ошибка присутствует. TestResourceTrackerService.java (685).
V6072 Два похожих фрагмента кода были найдены. Возможно, это опечатка, и publicCacheвместо нее следует использовать переменную usercache.
TestResourceLocalizationService.java (315),
TestResourceLocalizationService.java (309),
TestResourceLocalizationService.java (307),
TestResourceLocalizationService.java (313)
Джава
xxxxxxxxxx
1
2
public void testDirectoryCleanupOnNewlyCreatedStateStore()
3
throws IOException, URISyntaxException
4
{
5
....
6
// verify directory creation
7
for (Path p : localDirs) {
8
p = new Path((new URI(p.toString())).getPath());
9
10
// logic block #1
11
Path usercache = new Path(p, ContainerLocalizer.USERCACHE);
12
verify(spylfs).rename(eq(usercache), any(Path.class), any()); // <=
13
verify(spylfs).mkdir(eq(usercache), ....);
14
15
// logic block #2
16
Path publicCache = new Path(p, ContainerLocalizer.FILECACHE);
17
verify(spylfs).rename(eq(usercache), any(Path.class), any()); // <=
18
verify(spylfs).mkdir(eq(publicCache), ....);
19
....
20
}
21
....
22
}
И наконец, снова V6072 =). Переменные для просмотра подозрительного фрагмента: usercache и publicCache.
Вывод
Сотни тысяч строк кода написаны в разработке. Рабочий код обычно поддерживается в чистоте от ошибок, дефектов и недостатков (разработчики проверяют свой код, код проверяется и т. Д.). Тесты определенно уступают в этом отношении. Дефекты в тестах могут легко спрятаться за «зеленой галочкой». Как вы, вероятно, поняли из сегодняшнего обзора предупреждений, зеленый тест не всегда является успешной проверкой.
На этот раз при проверке кодовой базы Apache Hadoop статический анализ оказался крайне необходимым как в рабочем коде, так и в тестах, которые также играют важную роль в разработке. Итак, если вы заботитесь о своем коде и качестве тестирования, я предлагаю вам взглянуть на статический анализ.