03 Jul 2013

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);
    }

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