Чтобы было интереснее, часть из них оформлена в виде проблемного кода и спойлера, открыв который, вы увидите, в чём была проблема. Так что рекомендую сначала смотреть на код, находить в нём ошибку, а затем открывать спойлер. Решения проблем указано не будет — предлагаю самим подумать над ним. Просто потому, что я ленивый. Порядок списка не имеет глубокого смысла — просто это очерёдность, в которой я вспоминал про всякие реальные проблемы, которые доводили нас до кровавых слёз. Наверняка многие вещи покажется вам очевидными — но даже опытные разработчики могут случайно написать такой код.
Итак, поехали.
0. Отсутствие тестов
Как ни странно, до сих пор многие считают, что написание тестов замедляет скорость разработки. Конечно, это очевидно, что больше времени приходится тратить на то, чтобы писать тесты, и чтобы писать код, который можно тестировать. Но на отладку и регрессы после этого придётся тратить в разы больше времени…
1. Отсутствие запуска тестов
Если у вас есть тесты, которые вы не запускаете, или запускаете от раза к разу — то это всё равно что отсутствие тестов. И это даже хуже — у вас есть устаревающий код тестов и ложное чувство безопасности. Тесты как минимум должны запускаться в CI процессах при пуше кода в ветку. А лучше — локально перед пушем. Тогда разработчику не придётся через несколько дней возвращаться к билду, который, как оказалось, не прошёл.
2. Отсутствие покрытия
Если вы ещё не знаете, что такое покрытие в тестах, то самое время во прямо сейчас пойти и прочитать. Хотя бы википедию. Иначе велики шансы того, что ваш тест будет проверять от силы 10% от того кода, который, как вы думаете, он проверяет. Рано или поздно вы на это обязательно наступите. Конечно, даже 100% покрытие кода никак не гарантирует его полную корректность — но это гораздо лучше, чем отсутствие покрытия так как покажет вам гораздо больше потенциальных ошибок. Недаром в последних версиях Node.JS даже появились встроенные инструменты для того, чтобы его считать. Вообще, тема покрытия глубокая и крайне холиварная, но не буду в неё слишком углубляться — хочется сказать по понемножку о многом.
3.
const {assert} = require('chai');
const Promise = require('bluebird');
const sinon = require('sinon');
class MightyLibrary {
static someLongFunction() {
return Promise.resolve(1); // just imagine a really complex and long function here
}
}
async function doItQuickOrFail()
{
let res;
try {
res = await MightyLibrary.someLongFunction().timeout(1000);
}
catch (err)
{
if (err instanceof Promise.TimeoutError)
{
return false;
}
throw err;
}
return res;
}
describe('using Timeouts', ()=>{
it('should return false if waited too much', async ()=>{
// stub function to emulate looong work
sinon.stub(MightyLibrary, 'someLongFunction').callsFake(()=>Promise.delay(10000).then(()=>true));
const res = await doItQuickOrFail();
assert.equal(res, false);
});
});Таймауты в юнит тестах.
Здесь хотели проверить, что установка таймаутов на долгую операцию действительно работает. В целом это и так имеет мало смысла — не стоит проверять стандартные библиотеки — но так же такой код приводит к другой проблеме — увеличению выполнения времени прохождения тестов на секунду. Казалось бы, это не так много… Но помножьте эту секунду на количество аналогичных тестов, на количество разработчиков, на количеств запусков в день… И вы поймёте, что из-за таких таймаутов вы можете терять впустую много часов работы еженедельно, если не ежедневно.
4.
const fs = require('fs');
const testData = JSON.parse(fs.readFileSync('./testData.json', 'utf8'));
describe('some block', ()=>{
it('should do something', ()=>{
someTest(testData);
})
})Загрузка тестовых данных вне блоков теста.
На первый взгляд кажется, что всё равно, где читать тестовые данные — в блоке describe, it или в самом модуле. На второй тоже. Но представьте, что у вас сотни тестов, и во многих из них используются тяжёлые данные. Если вы их грузите вне теста, то это приведёт к тому, что все тестовые данные будут оставаться в памяти до конца выполнения тестов, и запуск со временем будет потреблять всё больше и больше оперативной памяти — пока не окажется, что тесты больше вообще не запускаются на стандартных рабочих машинах.
5.
const {assert} = require('chai');
const sinon = require('sinon');
class Dog
{
// eslint-disable-next-line class-methods-use-this
say()
{
return 'Wow';
}
}
describe('stubsEverywhere', ()=>{
before(()=>{
sinon.stub(Dog.prototype, 'say').callsFake(()=>{
return 'meow';
});
});
it('should say meow', ()=>{
const dog = new Dog();
assert.equal(dog.say(), 'meow', 'dog should say "meow!"');
});
});
Код фактически заменён стабами.
Наверняка вы сразу увидели эту смешную ошибку. В реальном коде это, конечно, не настолько очевидно — но я видел код, который был обвешан стабами настолько, что вообще ничего не тестировал.
6.
const sinon = require('sinon');
const {assert} = require('chai');
class Widget {
fetch()
{}
loadData()
{
this.fetch();
}
}
if (!sinon.sandbox || !sinon.sandbox.stub) {
sinon.sandbox = sinon.createSandbox();
}
describe('My widget', () => {
it('is awesome', () => {
const widget = new Widget();
widget.fetch = sinon.sandbox.stub().returns({ one: 1, two: 2 });
widget.loadData();
assert.isTrue(widget.fetch.called);
});
});
Зависимость между тестами.
С первого взгляда понятно, что здесь забыли написать
afterEach(() => { sinon.sandbox.restore(); });
Но проблема не только в этом, а в том, что для всех тестов используется один и тот же sandbox. И очень легко таким образом смутировать среду выполнения тестов таким образом, что они начнут зависеть друг от друга. После этого тесты начнут выполняться только в определённом порядке, и вообще непонятно что тестировать.
К счастью, sinon.sandbox в какой-то момент был объявлен устаревшим и выпилен, так что с такой проблемой вы можете столкнуться только на легаси проекте — но есть огромное множество других способов смутировать среду выполнения тестов таким образом, что потом будет мучительно больно расследовать, какой код виновен в некорректном поведении. На хабре, кстати, недавно был пост про какой-то шаблон вроде «Ice Factory» — это не панацея, но иногда помогает в таких случаях.
7. Огромные тестовые данные в файле теста
Очень часто видел, как прямо в тесте лежали огромные JSON файлы, и даже XML. Думаю, очевидно, почему это не стоит делать — становится больно это смотреть, редактировать, и любая IDE не скажет вам за такое спасибо. Если у вас есть большие тестовые данные — выносите их вне файла теста.
8.
const {assert} = require('chai');
const crypto = require('crypto');
describe('extraTests', ()=>{
it('should generate unique bytes', ()=>{
const arr = [];
for (let i = 0; i < 1000; i++)
{
const value = crypto.randomBytes(256);
arr.push(value);
}
const unique = arr.filter((el, index)=>arr.indexOf(el) === index);
assert.equal(arr.length, unique.length, 'Data is not random enough!');
});
});Лишние тесты.
В данном случае разработчик был очень обеспокоен тем, что его уникальные идентификаторы будут уникальными, поэтому написал на это проверку. В целом понятное стремление — но лучше прочитать документацию или прогнать такой тест несколько раз, не добавляя его в проект. Запуск его в каждом билде не имеет никакого смысла.
Ну и завязка на случайные значения в тесте — сама по себе является отличным способом выстрелить себе в ногу, сделав нестабильный тест на пустом месте.
9. Отсутствие моков
Гораздо проще запускать тесты с живой базой и сотальными сервисами, и гонять тесты на них.
Но рано или поздно это аукнется — тесты по удалению данных выполнятся на продуктовой базе, начнут падать из-за неработающего партнёрского сервиса, или в вашем CI просто не будет базы, на которой можно их прогнать. В общем, пункт достаточно холиварный, но как правило — если вы можете эмулировать внешние сервисы, то лучше это делать.
11.
const {assert} = require('chai');
class CustomError extends Error
{
}
function mytestFunction()
{
throw new CustomError('important message');
}
describe('badCompare', ()=>{
it('should throw only my custom errors', ()=>{
let errorHappened = false;
try {
mytestFunction();
}
catch (err)
{
errorHappened = true;
assert.isTrue(err instanceof CustomError);
}
assert.isTrue(errorHappened);
});
});Усложнённая отладка ошибок.
Всё неплохо, но есть одна проблема — если тест вдруг упал, то вы увидите ошибку вида
1) badCompare
should throw only my custom errors:
AssertionError: expected false to be true
+ expected - actual
-false
+true
at Context.it (test/011_badCompare/test.js:23:14)
Дальше, чтобы понять, что за ошибка собственно случилась — вам придётся переписывать тест. Так что в случае неожиданной ошибки — постарайтесь, чтобы тест рассказал о ней, а не только сам факт того, что она произошла.
12.
const {assert} = require('chai');
function someVeryBigFunc1()
{
return 1; // imagine a tonn of code here
}
function someVeryBigFunc2()
{
return 2; // imagine a tonn of code here
}
describe('all Before Tests', ()=>{
let res1;
let res2;
before(async ()=>{
res1 = await someVeryBigFunc1();
res2 = await someVeryBigFunc2();
});
it('should return 1', ()=>{
assert.equal(res1, 1);
});
it('should return 2', ()=>{
assert.equal(res2, 2);
});
});
Всё в блоке before.
Казалось бы, классный подход сделать все операции в блоке `before`, и таким образом оставить внутри `it` только проверки.
На самом деле нет.
Потому что в этом случае возникает каша, в которой нельзя ни понять время реального выполнения тестов, ни причину падения, ни то, что относится к одному тесту, а что к другому.
Так что вся работа теста (кроме стандартных инициализаций) должна выполняться внутри самого теста.
13.
const {assert} = require('chai');
const moment = require('moment');
function someDateBasedFunction(date)
{
if (moment().isAfter(date))
{
return 0;
}
return 1;
}
describe('useFutureDate', ()=>{
it('should return 0 for passed date', ()=>{
const pastDate = moment('2010-01-01');
assert.equal(someDateBasedFunction(pastDate), 0);
});
it('should return 1 for future date', ()=>{
const itWillAlwaysBeInFuture = moment('2030-01-01');
assert.equal(someDateBasedFunction(itWillAlwaysBeInFuture), 1);
});
});
Завязка на даты.
Тоже казалось бы очевидная ошибка — но тоже периодически возникает у уставших разработчиков, которые уже считают, что завтра никогда не наступит. И билд который отлично собирался вчера, внезапно падает сегодня.
Помните, что любая дата наступит рано или поздно — так что или используйте эмуляцию времени штуками вроде `sinon.fakeTimers`, или хотя бы ставьте отдалённые даты вроде 2050 года — пускай голова болит у ваших потомков...
14.
describe('dynamicRequires', ()=>{
it('should return english locale', ()=>{
// HACK :
// Some people mutate locale in tests to chinese so I will require moment here
// eslint-disable-next-line global-require
const moment = require('moment');
const someDate = moment('2010-01-01').format('MMMM');
assert.equal(someDate, 'January');
});
});Динамическая подгрузка модулей.
Если у вас стоит Eslint, то вы наверняка уже запретили динамические зависимости. Или нет.
Часто вижу, что разработчики стараются подгружать библиотеки или различные модули прямо внутри тестов. При этом они в целом знают, как работает `require` — но предпочитают иллюзию того, что им будто бы дадут чистый модуль, который никто пока что не смутировал.
Такое предположение опасно тем, что загрузка дополнительных модулей во время тестов происходит медленнее, и опять же приводит к большему количеству неопределённого поведения.
15.
function someComplexFunc()
{
// Imagine a piece of really strange code here
return 1;
}
describe('cryptic', ()=>{
it('success', ()=>{
const result = someComplexFunc();
assert.equal(result, 1);
});
it('should not fail', ()=>{
const result = someComplexFunc();
assert.equal(result, 1);
});
it('is right', ()=>{
const result = someComplexFunc();
assert.equal(result, 1);
});
it('makes no difference for solar system', ()=>{
const result = someComplexFunc();
assert.equal(result, 1);
});
});Непонятные названия тестов.
Наверное, вы устали от очевидны вещей, да? Но всё равно придётся о ней сказать потому что многие не утруждаются написанием понятных названий для тестов — и в результате понять, что делает тот или иной тест, можно только после долгих исследований.
16.
const {assert} = require('chai');
const Promise = require('bluebird');
function someTomeoutingFunction()
{
throw new Promise.TimeoutError();
}
describe('no Error check', ()=>{
it('should throw error', async ()=>{
let timedOut = false;
try {
await someTomeoutingFunction();
}
catch (err)
{
timedOut = true;
}
assert.equal(timedOut, true);
});
});Отсутствие проверки выброшенной ошибки.
Часто нужно проверить, что в каком-то случае функция выкидывает ошибку. Но всегда нужно проверять, те ли это дроиды, которых мы ищем — поскольку внезапно может оказаться, что ошибка была выкинута другая, в другом месте и по другим причинам...
17.
function someBadFunc()
{
throw new Error('I am just wrong!');
}
describe.skip('skipped test', ()=>{
it('should be fine', ()=>{
someBadFunc();
});
});Отключенные тесты.
Конечно, всегда может появиться ситуация, когда код уже много раз протестирован руками, нужно срочно его катить, а тест почему-то не работает. Например, из-за не очевидной завязки на другой тест, о которой я писал ранее. И тест отключают. И это нормально. Не нормально — не поставить мгновенно задачу на то, чтобы включить тест обратно. Если это не сделать, то количество отключенных тестов будет плодиться, а их код — постоянно устаревать. Пока не останется единственный вариант — проявить милосердие и выкинуть все эти тесты нафиг, поскольку быстрее написать их заново, чем разбираться в ошибках.
Вот такая вот подборка вышла. Все эти тесты отлично проходят проверки, но они broken by design. Добавляйте свои варианты в комментарии, или в репозиторий, который я сделал для коллекционирования таких ошибок.
Комментарии (14)
Snowfall022
05.12.2018 08:44Тестировать код с моками, всё равно что тестировать функцию A и надеяться что от этого абсолютно независимая функция B будет работать правильно. Не логичнее ли вместо моков делать интеграционные тесты, которые хоть и сложнее, но всё же хотя бы реальные? Иначе какой смысл говорить себе «допустим, что эта функция вернула мне из бд пользователя с таким-то id...», если на самом деле эта хрень обрушится ещё 10 раз самым неожиданным для вас образом. Либо же выносить внешние зависимости из чистого кода и передавать туда уже готовые для переваривания данные. И, как мне кажется, по возможности следует использовать property-based тесты, потому что я слабо представляю как можно написать несколько тест кейсов и надеяться, что это надёжно, хотя на самом деле это просто смешно.
Sklott
05.12.2018 09:24Не логичнее ли вместо моков делать интеграционные тесты, которые хоть и сложнее, но всё же хотя бы реальные?
Логичнее отделять мух от котлет, т.е. делать юнит тесты с моками И интеграционные тесты с реальными связями.
EvilsInterrupt
05.12.2018 12:28Вы вот увидели упавший интеграционный тест, то как быстро вы можете получить ответ «В каком именно компоненте появился баг?». К примеру вы сейчас на совещании с командой и без открытой IDE и нет возможности запустить отладчик.
Тесты с моками(не стабами) говорят о том, что на том или ином наборе компонент действительно ведет себя тем или иным способом. Позвал 5 раз функцию1, 15 раз функцию3 и др. Выполняется этот тест быстрее чем интеграционый. В разы быстрее!
Интеграционный тест показывает работу кода в реальных боевых условиях. Да он приближен к реальности больше, чем тест с моками, но и выполняется он значительно медленнее! И, если он нашел какую-то багу, значит не написан какой-то из тестов с моками и это можно сделать тем самым ускорить и улучшить процесс тестирования.
При тестах с моками можно увереннее говорить «Ошибка в компоненте1» или «Ошибка работы компонента2 в методе3 при пустой строке». Да, конечно что-то вы не сможете получить сразу, т.к. ненаписан тест. Но ведь все же итеративно!
Увидели багу, добавили тест. Поправили багу, зарефакторили, запустили тесты. Все ок? Вперед к другой задаче. Тест больше не нужен, т.к. уже не актуален после рефакторинга? ОК, выпилем его. Это же до боли привычный мир разработки
Golovanoff
05.12.2018 10:08а где пример №10? :)
jehy Автор
05.12.2018 10:09Это пример формальных тестов.
Вроде он есть, а на самом деле — нет.
А если серьёзно, то проспустил при создании репозитория. Обнаружил на этапе написания статьи и решил оставить пасхалочкой для внимательных. Смайлик.
justboris
05.12.2018 11:46Пункты 4 и 14 противоречат друг другу.
Сначала говорится, что если результат `require()` нужен только одному тесту, то его не нужно выносить наверх. Затем, спустя 10 пунктов, это объявляется антипаттерном и предлагается избавиться от `require()` в теле тестов.jehy Автор
05.12.2018 11:59Да, не совсем удачный пример в 4, поправлю.
Дело в том, что тестовые данные по хорошему нужно грузить не через `requiire`, а через стандартное чтение файла с парсингом. Иначе вы всё равно навсегда оставите его в памяти и сделаете потенциально мутируемым.
Так что в примере должно было быть чтение файла, а не `require`.
Правда, есть нюанс — `require` отрабатывает в разы быстрее, чем это комбо. Поэтому в продакшне обычно используется именно он для быстрого запуска приложения.justboris
05.12.2018 12:30А есть какие-нибудь подтверждения, что такая загрузка данных локально в тесте вообще хоть что-то ускоряет?
По моей информации, V8 очень хорошо вычищает неиспользуемые значения из замыкания, и дополнительно помогать ему не нужно.
И ещё вопрос: откуда в тестах берутся гигабайты данных? Для проверки бизнес-логики столько не надо. Поэтому совет #4 выглядит как предложение сэкономить на спичках
jehy Автор
05.12.2018 13:19А есть какие-нибудь подтверждения, что такая загрузка данных локально в тесте вообще хоть что-то ускоряет?
Не ускоряет, а позволяет не тратить лишнюю память.
По моей информации, V8 очень хорошо вычищает неиспользуемые значения из замыкания, и дополнительно помогать ему не нужно.
Так они очень даже используемые, и будут все подгружены в момент запуска тестов. Возможно, по мере прохождения тестов они и будут выгружаться — но вряд ли, да и поздно уже.
И ещё вопрос: откуда в тестах берутся гигабайты данных? Для проверки бизнес-логики столько не надо. Поэтому совет #4 выглядит как предложение сэкономить на спичках
Всё зависит от объёма проекта, количества тестов, и данных, которыми он занимается. Так что в большинстве случае это, конечно, будет экономия на спичках. Но почему бы так не делать, если это не заставляет предпринимать лишние усилия.
mayorovp
06.12.2018 10:2018.
Тест, который проверяет, что тестируемый метод работает строго определенным образом: обращается к другим методам в строго определенном порядке и не обращается ни к чему еще.
Lure_of_Chaos
Гораздо приятней было бы читать, если бы листинги были оформлены аккуратней, тесты визуально отделены от кода, проставлены комментарии, что делает код или даны более осмысленные названия.
А еще лучше был бы формат в виде советов с примерами, а не загадок, поскольку проблемных мест больше, чем указано )
jehy Автор
Да, данная публикация прям наглядно показывает причину появления описанных в ней юнит тестов — не надо садиться писать статью уставшим в десять вечера.
dagen
На самом деле этот набор легко укладывается в короткий перечень рекомендаций, который можно выложить на конфлюенс и прикрепить временно в фронтовом чатике. И в любом случае эти вещи будут контролироваться в рамках код ревью, даже если нет строгой последовательности issue-PR-review.
А повысить качество написания тестов в команде удобнее на собственном примере, отталкиваясь от лучших практик, от того, как надо делать, а не как не надо. И обучалку-доклад-митап построить в виде "вот у нас есть такой кейз в нашем коде, и покрывать его тестами надо вот таким образом, как у меня (вот пермалинк на гитлаб). Это лучше, чем решение в лоб таймаутом по таким-то причинам." По крайней мере я так делал. С выкладыванием этого на конфлюенс. Что не отменяет короткого перечня рекомендаций по оформлению тестов)