Testing Worst Practices
Резюме: при достаточной популярности написания тестов (особенно модульных), появляются очень разнообразные “лучшие практики” в связи с этим. Однако похоже многие разработчики (даже очень опытные) все-таки не понимают как не нужно писать тесты. Поэтому поговорим о worst practices в тестировании. Все примеры в статье - из настоящих проектов.
Лирическое отступление
Интересен тот факт, что при всей популярности многочисленных книг и советов по написанию модульных тестов, авторы практически никогда не ссылаются на материалы из области QA, хотя казалось бы у кого нужно учиться тестированию. Может поэтому программисты не умеют писать тесты? Многие из нижеперечисленных советов опираются на то, как тест инженеры подходят к написанию тест кейсов.
Проблем с тестами может быть три: их трудно поддерживать, читать и писать. Каждая проблема может решаться по-своему, во многих случаях достаточно просто порефакторить тесты, сделать их более грамотными. Бывает этого недостаточно и нужно рефакторить сам код - не имея адекватного кода писать тесты достаточно проблематично. Разработчики же в погоне за “лучшими практиками” забывают, что главное - это простота и эффективность, поэтому хочу показать во что выливается слепое следование идеалам из интернетов.
Прежде чем начнем рассматривать конкретные случаи, - немного терминологии:
SUT - system under test, то бишь то, что мы тестируем. Бывает еще называют CUT (component under test). В случае модульных тестов это как правило тестируемый класс.
DOC - depended on objects, то бишь то, от чего зависит SUT. Например, класс может использовать другой класс делегируя часть функционала. Этот делегат и называют DOC’ом.
Тест - в данном контексте это тестовый метод, тест кейс. Не класс! Тестовый класс - это набор тестов.
Практика
Итак, не тестируйте несколько результатов. Если функция затрагивает несколько аспектов, то у разработчиков руки чешутся протестировать их все в одном тесте, к примеру:
@Test
public void testTopicChanged() throws MailingFailedException {
prepareEnabledProperty();
topic.getSubscribers().add(user1);
topic.getSubscribers().add(user2);
topic.getSubscribers().add(currentUser);
when(subscriptionService.getAllowedSubscribers(topic)).thenReturn(topic.getSubscribers());
service.subscribedEntityChanged(topic);
verify(mailService, times(2)).sendUpdatesOnSubscription(any(JCUser.class), eq(topic));
verify(mailService).sendUpdatesOnSubscription(user1, topic);
verify(mailService).sendUpdatesOnSubscription(user2, topic);
assertEquals(topic.getSubscribers().size(), 3);
}
В данном тесте проверяется, что подписанные пользователи получают письмо, а неподписанные его не получают. Недостаток такого подхода в том, что когда упадет тест будет не понятно что не работает - нужно лезть в сам тест и смотреть его код. Если же было бы два теста:
subscribedUserShouldReceiveNotification
& notSubscribedUserShouldNotBeNotified
тогда при красном тесте ясно ЧТО не работает.Плюс сам тест становится сложней. Много маленьких тестов с короткими сценариями намного лучше, чем мало, но с длинной простыней.
Не проверяйте вызовы методов. Ваша SUT должна вернуть результат. И только его (этот результат) нужно проверять. Как не надо:
...
when(userDao.getByUsername(username)).thenReturn(user);
...
boolean isAuthenticated = userService.loginUser(username, PASSWORD, false, httpRequest, httpResponse);
assertTrue(isAuthenticated);
verify(userDao).getByUsername(username);
В данном случае вызов
userDao.getByUsername()
является внутренним устройством SUT. Нам не надо знать об этом, нам важен результат. Дабы вообще было возможно протестировать SUT нам приходится лезть своими грязными руками в реализацию и определять поведение DOC’a - но это плохо. Что еще хуже - мы на этом не останавливаемся и проверяем, что взаимодействие с DOC’ом на самом деле произошло. Но зачем, является ли это результатом функции? Это личная жизнь SUT, не нужно в нее вмешиваться ибо при рефакторинге нам многое придется в тестах переписывать. Если хотите писать поддерживаемые тесты, избегайте проверок вызовов.Исключение составляет тот случай, когда взаимодействие с DOC’ом собсно является результатом выполнения функции. К примеру, оповещение пользователей по мейлу о каком-то событии - это результат действий SUT. К сожалению без влезания во внутренности реализации SUT в таком случае не всегда можно обойтись.
Не усложняйте тестовые данные “на всякий случай”. Наглядней на примере:
@Test
public void extractMentionedUserShouldReturnAllMentionedCyrillicUserInBBCodes() {
String textWithUsersMentioning = "In this text we have 3 user mentioning: first [user]Иванов[/user]," +
"second [user notified=true]Петров[/user]," +
"third [user]Сидоров[/user]";
MentionedUsers mentionedUsers = MentionedUsers.parse(textWithUsersMentioning);
Set<String> extractedUserNames = mentionedUsers.extractAllMentionedUsers(textWithUsersMentioning);
assertTrue(extractedUserNames.size() == 3, "Passed text should contain 3 user mentioning.");
assertTrue(extractedUserNames.contains("Иванов"), "Иванов is mentioned, so he should be extracted.");
assertTrue(extractedUserNames.contains("Петров"), "Петров is mentioned, so he should be extracted.");
assertTrue(extractedUserNames.contains("Сидоров"), "Сидоров is mentioned, so he should be extracted.");
}
Данный тест должен проверить, что метод может работать с русскими пользователями. Однако он проверяет сразу 3х, а не одного! Знаю о чем думал разработчик, и возможно подумали вы, - на всякий случай, авось проблема обнаружится когда пользователей несколько. Но задача тестов - предоставлять гарантии, а не тыкать пальцем в небо. Гарантии предоставляются конкретными тест кейсами, которые проверяют вполне конкретные случаи. Это раз. А два - кто сказал, что с одним пользователем не сломается? Если хотите проверить случай - добавьте тест специально для этого случая, но не мешайте все вместе - лишь переводите чужое и свое время.
Другое, о чем может подумать разработчик, когда пишет такое - потестирую-ка я и другие значения, может на другом значении упадет! Но эта логика тоже не верна, у QA есть несколько методик при написании тест кейсов. Одна из них - это разбиение тестовых данных на классы эквивалентности. Суть в том, что весь набор данных делится на группы, и мы считаем, что данные из одной и той же группы вряд ли приведут к ошибке. Пока у вас не будет четкого разбиения данных на группы, ваши тесты пишутся на авось. Не пишите лишние тесты для одного и того же класса эквивалентности.
Не используйте DataProvider’ы. Похожих результатов в JUnit можно достигнув с помощью @Parameterized и @Theory. Они позволяют задать какие-то тестовые данные в одном месте сразу кучей, а затем использовать один тестовый метод для работы с этими данными:
@DataProvider(name = "status-provider")
public Object[][] statusData() {
return new Object[][]{
{PrivateMessageStatus.NEW, false},
{PrivateMessageStatus.DRAFT, false},
{PrivateMessageStatus.SENT, true},
{PrivateMessageStatus.DELETED_FROM_INBOX, false},
{PrivateMessageStatus.DELETED_FROM_OUTBOX, true}
};
}
Этот метод предоставляет данные и результат теста. А сам тест выглядит так:
@Test(dataProvider = "status-provider")
public void testIsReplyAllowed(PrivateMessageStatus status, boolean expectedResult) {
pm.setStatus(status);
assertEquals(expectedResult, pm.isReplyAllowed());
}
Почему так - ай-ай-ай:
Если упадет тест, то догадайся какой же случай все-таки свалился.
Дебажить такое тоже нелегко, тест-то всегда запускается со всеми параметрами, а что если нужен 3й?
Тест не показывает что же мы все-таки тестируем. Одно дело если тест называется
userCanReplyToMessageInSentState
- тут прям из названия понятна бизнес задача и что требуется от функции, а другое - это просто мапа Object-Boolean. Хотя, признаться, в этом случае все еще не так плохо.
Не мокайте то, что можно не мокать. Моканье сущностей, утилит - дело неблагодарное, не каждый вызов DOC’a нужно мокать. Самый яркий пример - моканье обычных сущностей (entities). Пример:
post = mock(Post.class);
when(post.getId()).thenReturn(POST_ID);
when(post.getUserCreated()).thenReturn(user);
when(post.getPostContent()).thenReturn(POST_CONTENT);
when(post.getTopic()).thenReturn(topic);
Почему бы не сделать:
new Post(POST_CONTENT, topic).withId(POST_ID).withAuthor(user)
, ведь проще же, наглядней. Избегайте использования полей в тестовых классах. Помните, что поля для класса глобальны и могут использоваться любым методом. Каждое добавленное поле в классе - существенное усложнение для чтения всех методов. Потому как для прочтения тестов часто не достаточно прочесть лишь метод, как правило используются разного рода моки, которые матрешкой собираются друг в друга. Чем меньше будет этого “шума”, тем проще будет читать каждый отдельный тест. И для новичков в вашем проекте намного проще будет писать свои тесты - для этого им не нужно будет читать 40 строк странных полей. Пример:
private static final String USER_NAME = "username";
private static final String FIRST_NAME = "first name";
private static final String LAST_NAME = "last name";
private static final String EMAIL = "[email protected]";
private static final String PASSWORD = "password";
private static final String IMAGE_BYTE_ARRAY_IN_BASE_64_STRING = "it's dummy string";
@Mock
private UserService userService;
@Mock
private MessageSource messageSource;
@Mock
private ImageControllerUtils imageControllerUtils;
private AvatarController avatarController;
private byte[] validAvatar = new byte[] {-119, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82, 0,
0, 0, 4, 0, 0, 0, 4, 1, 0, 0, 0, 0, -127, -118, -93, -45, 0, 0, 0, 9, 112, 72, 89, 115, 0, 0, 1,
-118, 0, 0, 1, -118, 1, 51, -105, 48, 88, 0, 0, 0, 32, 99, 72, 82, 77, 0, 0, 122, 37, 0, 0,
-128, -125, 0, 0, -7, -1, 0, 0, -128, -23, 0, 0, 117, 48, 0, 0, -22, 96, 0, 0, 58, -104, 0, 0,
23, 111, -110, 95, -59, 70, 0, 0, 0, 22, 73, 68, 65, 84, 120, -38, 98, -40, -49, -60, -64, -92,
-64, -60, 0, 0, 0, 0, -1, -1, 3, 0, 5, -71, 0, -26, -35, -7, 32, 96, 0, 0, 0, 0, 73, 69, 78, 68,
-82, 66, 96, -126
};
Это не рекордсмен на моей практике, но уже такой перечень полей и констант должен отпугивать желающих его прочесть, а тем более, - дописать. Можно попытаться найти оправдания, мол, это позволяет избегать дублирования кода и уменьшить размеры самих тестов. Однако вы усложняете все(!) тесты для локального выигрыша. Если вам хочется подготовить данные или вы хотите избежать дублирования - используйте приватные методы, к примеру. Да, они тоже будут глобальны для класса, но они не являются обязательными для прочтения во всех тестах, для написания дополнительного теста вам не нужно перечитывать все методы.
Диагностировать такую патологию очень просто - если хоть один тест не использует ваше поле, начинайте думать. Если таких тестов становится больше, начинайте действовать.
Не используйте наследование. Разработчики любят изобретать. А еще любят писать красивый код. Жаль, что у них ни то, ни другое не получается. Тесты не исключение - частенько разработчики норовят написать очередной фреймворк для тестирования. Это случается когда у большого количества тестовых классов есть что-то общее. К примеру, у нас есть интеграционные тесты и все создают какой-то контекст и подготавливают какое-то окружение, а затем многие из них производят похожие действия для полной готовности SUT. И тут идея.. а не написать ли нам абстрактный тестовый класс, который будет это все делать, а мы его будем лишь наследовать в конкретных тестах. Даешь изобретателям по ушам! Проблема в том, что наследование привносит свою долю сложности в дизайн классов. Недаром от него в последнее время отказываются в пользу делегирования (гуглим delegation over inheritance). Так вот, имейте в виду, что для понимания вашего класса вам придется ити вверх по иерархии и читать предка. Вы ведь не знаете вот так наугад что в нем.
Следующая проблема возникает сразу после введения общего предка - ведь оказывается тесты группируются, и одной группе нужно меньше однотипных действий вполнять, другой - больше. И тут идея.. а не сделать еще один абстрактный класс, который наследуется от предыдущего абстрактного класса? Даешь изобретателям по рукам! Ведь теперь нам для чтения теста нужно лезть все глубже и глубже. А про эти супер-классы нужно еще знать. И они тоже обрастают логикой. И каждый из них имеет свои поля.
В общем был у меня опыт работы в проекте, где такая иерархия была доведена до абсурда. А все ведь намного проще - сделайте вы этот класс с общей логикой, да вызовайте разные его методы в @BeforeMethod. Да, в каждом тестовом классе это нужно делать, но это понятно, это явный вызов явного метода с нормальным названием.
Замечу, что я не противник всяких AbstractTransactionalTestNGSpringContextTests
- во-первых, этот класс всем известен, не только тем, кто его писал. Во-вторых, хороший фреймворк - это фреймворк, в исходники которого не нужно заглядывать каждые 5 минут, в исходники этого класса вряд ли приходится заглядывать постоянно. Не у каждого рядового разработчика получается написать такой фреймворк.
Избегайте использования @Before. Не вообще, конечно, - создать объект SUT’a вполне там можно, и моки заинжектить тоже. Но случается что используют prerequisite методы слишком интенсивно. Пример:
@BeforeMethod
public void init() throws NotFoundException {
initMocks(this);
Branch branch = new Branch("branch", "branch");
branch.setId(BRANCH_ID);
Topic topic = new Topic(user, "title");
topic.setBranch(branch);
topic.setId(TOPIC_ID);
post = new Post(user, POST_CONTENT);
post.setId(POST_ID);
post.setTopic(topic);
topic.getPosts().addAll(asList(post));
when(postService.get(POST_ID)).thenReturn(post);
when(topicFetchService.get(TOPIC_ID)).thenReturn(topic);
when(breadcrumbBuilder.getForumBreadcrumb(topic)).thenReturn(new ArrayList<Breadcrumb>());
controller = new PostController(
postService, breadcrumbBuilder, topicFetchService, topicModificationService,
bbCodeService, lastReadPostService, userService);
}
Тут опять проблемы - во-первых, то же самое, что и с полями - нужно всегда читать этот код, он ведь выполняется для каждого теста. Во-вторых, часть этого кода является пререквизитами только некоторых тестов. В общем здесь усложнение опять ни к чему, снова на выручку приходят приватные методы.
8Comments
Хорошая статья. Несомненно, чем проще тесты, тем лучше. Отлично, если тесты простые и при этом обеспечивают хорошее покрытие и могут быть использоваться в разных конфигурациях.
Комментарий по поводу DataProvider:
В последних версия JUnit для каждого вызова теста с параметром можно назначить индивидуальное имя, которое будет отображаться в отчете.
https://github.com/junit-team/junit/wiki/Parameterized-tests
Да, можно (в статье присутствует его упоминание), это правда добавляет параметр, что само собой может снизить читаемость.
Мне кажется таким способом имеет смысл пользоваться когда тестовых данных становится и вправду много. QA, бывает, используют Decision Tables - думаю это как раз тот случай, когда писать на каждый случай тестовый метод будет нецелесообразно. Сам я правда этим еще не баловался, но кажется логичным.
Вряд ли добавление параметра в аннотацию в одном месте сильно снижает читаемость. Зато при грамотном использовании это может снять 1 и 3 пункты в вашем списке “почему DataProvider это плохо”.
Еще одно момент, если вам интересно мое мнение. Вы пишите про TestNG и, вероятно, широко используете его. Это прекрасный функциональный инструмент, однако, если во главу угла ставится изоляция тестов и простота, а не возможности, то JUnit, на мой вкус, более логичный выбор. И да, с JUnit меньше приходиться бить инженеров по рукам :-).
Работаю тестировщиком. Скажу что Parametrized-тесты только улучшают жизнь. Возьму к примеру задачу тестирования функции сложения двух чисел. Меня как тестировщика будут интересовать ситуации: “А что будет если я нули подам?”, “А что если я подам отрицательные числа?”, “А что если подам на единицу больше чем максимальное?” и др. Эти вопросы сами собою возникают у тестировщика! Чтобы ответить на эти вопросы, мне нужно будет написать либо несколько тест-методов каждый из которых дает ответы на эти вопросы, либо написать один testSumNumbers с таблицей в которых как раз заданы входные параметры чтобы вызвать ответы на эти вопросы. Опять же в виду очевидных вопросов(см.выше) также сами собою возникнут и входные параметры, которые также будут очевидны! Я обеими руками “ЗА” применение параметризованных методов.
В теории у параметризированных тестов все круто (собсно исходя из теории эти @DataProvider’ы и были наверно добавлены), проблемы возникают именно на практике, когда такие тесты уж очень трудно читать и поддерживать.
sys_dev, ты вот говоришь именно про красивые теоретические случаи. На практике у тебя не будет метода который вычисляет сумму, будет что-то по-сложней да по-заковыристей. И вот тогда начинается мессиво с условиями и циклами в тестах. И вот тогда ты не захочешь этот тест поддерживать (если он не тобою был написан).
Да, есть случаи когда параметризированные тесты могут помочь - когда у нас уж очень много классов эквивалентности, когда большие Decision Tables. Но это достаточно нечастые случаи.
https://code.google.com/p/spock/ здесь поможет
Отвечу спустя год ))) Случайно гугл выкинул сюда.
Это не теория! Это практика. Причем ежедневная! Слишком часто слышу “Проверь другую вещь, это уже работает”. Настолько часто что удивляет насколько ж люди бывают самоуверенными.
Я могу показать в каких случаях это работает, вот пример: тестирование BB кодов. Да, пришлось сделать используя DataProvider. Почему так сделали? Ну потому что случаев настолько много, что делить их по методам менее целесообразно чем вынести кейсы в DataProvider. Если взять один кейс и проследить цепочку выполнения, то как будет понятней - в одном методе или вот так, когда разнесено по кускам? Конечно в одном, хотя бы потому что прыгать туда-сюда не нужно. Но так бывает в программировании и тестировании, что когда хочешь добавить гибкости и динамичности, приходится жертвовать читабельностью. Важно только определить стоит ли полученная гибкость того чтоб усложнять код. Это вопрос на который приходится отвечать по несколько раз на дню. Справедливости ради стоит заметить, что разобравшись один раз, сам список кейсов проходить намного проще - они все в одном месте. Но так оно само собой и получается - когда растет кол-во кейсов, сложность их чтения один за одним возрастает по сравнению со сложностью одного конкретного кейса.
Ну и не стоит забывать что это системный тест, в таких тестах использовать DataProvider’ы приходится чаще чем при написании модульных, потому как классов еквивалентности как правило намного больше.
В общем-то еще раз повторил то же, что отвечал в предыдущих комментариях :)