Начинается она с того, что в проекте в результате определенного безобидного коммита упало порядка 150 тестов, набор падающих тестов при этом не являлся стабильным. Тесты не были связаны между собой, выполнение тестов происходило последовательно. В качестве источника данных для тестов служит in-memory база данных h2. Падение подавляющего большинства из этих 150 тестов сопровождалось ошибкой в логе: «Cannot get a connection, pool error Timeout waiting for idle object». Следует сказать, что размер пула коннектов при выполнении тестов в проекте равен 1.
Небольшое лирическое отступление: в коде проекта периодически используется отвязка транзакции от потока, далее выполнение кода в отдельной транзакции и, наконец, обратная привязка транзакции. Для такого рода случаев написан вспомогательный класс, использование которого выглядит примерно так:
TransactionRunner.run(dbDataManager(), new MethodTransaction() {
@Override
public ExecutionResult runInTransaction() throws Exception {
// код, который необходимо выполнить в отдельной транзакции
return result;
}
);В результате анализа было выявлено, что ошибка начинает проявляться после провалившегося теста, содержащего в себе вызов кода в транзакции:
TransactionRunner.run(dbDataManager(), new MethodTransaction() {
@Override
public ExecutionResult runInTransaction() throws Exception {
// ... рабочий код
// assert который валится
assert( 1, result.getSomeNotEqualOneIntValue() );
return result;
}
);Заглянем внутрь класса TransactionRunner, вызов метода приводит к следующему коду:
protected ExecutionResult run() throws CommonException {
Transaction outerTr = getThreadTransaction();
bindThreadTransaction(null);
try {
beginTransaction();
try {
setResult(transactionCode.runInTransaction());
} catch (Exception e) {
dbDataManager().rollbackTransaction();
if (transaction.onException(this, e))
throw e;
}
dbDataManager().commitTransaction();
return getResult();
} catch (Exception e) {
throw ExceptionUtil.createCommonException(e);
} finally {
bindThreadTransaction(outerTr);
}
}Итак, в чем же здесь проблема? А проблема заключается в том, что AssertionError возникающий в результате выполнения кода теста не наследуется от Exception, а значит вложенная транзакция ни откатывается, ни коммитится. Так как размер пула коннектов равен единице — получаем ту самую ошибку «Cannot get a connection, pool error Timeout waiting for idle object» при попытке получить объект Connection последующими тестами.
Мораль: необходимо размещать assert'ы в тестах с осторожностью, а в случае неочевидных и, в особенности, массовых падений одним из вариантов решения является проверка, учитывает ли обработка исключений объекты не наследующиеся от Exception.
Случай показался мне достойным фиксации, возможно кому-нибудь пригодится этот опыт.
Комментарии (23)
ViceCily
18.08.2018 05:39Ожидал прочитать откровения по особенностям внутренней реализации инструкции assert на уровне байт-кода.
По статье:
1. AssertionError, как и Exception — можно ловить по Throwable
2. Если имеется общий ресурс на весь тестовый сьют, который «дорого» поднимать для каждого теста заново — следует предусмотреть механизм гарантирующий очистку данного ресурса перед/после каждого тестового сценария.omickron
18.08.2018 11:02+1Ловить Throwable — bad practice.
Как вариант, можно в блоке finally проверять, закрыта ли транзакция, и если нет, откатывать.
Lure_of_Chaos
18.08.2018 17:24А все эти Assertions.assertEquals(), которые бросают Exception, для кого писали?
Т.е. странно использовать именно джавовский assert, чье поведение еще зависит от конфиги рантайма…
Так что, неудивительно, что отхватили граблей.qw1
18.08.2018 20:19Возможно, программист, который писал assert, использовал его по прямому назначению — проверять инварианты в runtime, вообще не думал о тестах (Assertions у него нет в зависимостях класса). Но при подключении тестов обнаружилась такая особенность.
aamonster
Идиотский вопрос: а что, продолжать выполнение после сработавшего assert уже стало нормальным?
Dreyk
ну если сработавший assert — это даже не Exception то почему бы не продолжить
aamonster
Именно что "даже не Exception". Сработавший ассерт — это ситуация "всё пропало, шеф!". Fatal Error. Что-то, чего мы не предусмотрели в принципе.
И, соответственно, чтобы имело смысл продолжать выполнение — надо вначале его починить, потому что всё, что происходит после него — происходит в сломанной системе.
qw1
Если приложение Test Runner будет умирать на первом assert-e, как и положено для ассертов, а тесты будут проверять пост-условия ассертами, то первый проваленный тест остановит тестирование и не позволит увидеть картину по всем тестам.
Об этом и статья — не пользуйтесь ассертами в тестах, чтобы не попасть в такие ситуации.
aamonster
Если приложение Test Runner будет умирать на первом assert-e — программисты будут вынуждены починить этот ассерт для запуска остальных тестов. Нет проблемы.
А потом, скорее всего, перестанут писать ассерт там, где можно продолжать работу.
P.S. и ассерт может быть совсем не в коде теста, а в коде тестируемой функции или даже глубже. Так что остановиться придётся.
pankraty
Это если сразу понятно, какой из тестов обрушил остальные. Но когда внезапно падает 150 тестов, отыскивать тот единственный, из-за которого не работают все — удовольствие сомнительное. Оказывался в подобной ситуации, когда один из тестов начал выдавать StackOverflowException, чем обрушивал TestRunner.
aamonster
Стоп-стоп. Если речь идёт об assert — всё сразу понятно. Который заасертился — тот и чинить.
Хотя, конечно, если всё плохо и какой-то тест ещё до заассертившегося сломал окружение и при этом не зафейлился… Но это совсем уж ад.
pankraty
Может, я что-то не так понял, но у меня сложилось впечатление, что внезапно 150+ тестов стали красными, при это ошибка только в одном, а остальные не смогли выполниться из-за этой ошибки. И да, исправить нужно только один тест, но сначала его надо найти из 150+ красных. Непродуктивная трата времени.
aamonster
Ну вот если бы всё падало на assert — была бы диагностика типа "10 красных тестов, тест XXX вообще упал, невозможно продолжать". И автор починил бы XXX срочно, прогнал бы тесты заново и получил, допустим, 15 красных (10 до XXX и 5 после). И уже не спеша "озеленял" бы остаток.
bezromantik Автор
Один тест зафейлил окружение (оставил незавершенной транзакцию по единственному коннекту), при этом на нем был AssertionError, другие 150 упали из-за невозможности получить коннект, при этом часть стала красными, часть желтыми. Ситуация усугблялась тем, что задача в целом стояла починить тесты на сборке, то есть кроме 1 упавшего ассерта лежало еще порядка 15 тестов не связанных с этой ошибкой.
aamonster
Я знал, знал, что их было 15: https://habr.com/post/420509/?reply_to=19009351#comment_19007515 :-D
Да, и какое решение в итоге приняли, чтобы повторно не наступить на грабли? Понятно, что запретить стандартный assert совершенно недостаточно (ну, в коде тестов ему не место, но он же может быть и в рабочем коде, а то и в используемых библиотеках).
Ловить assert-овский exception тоже и делать cleanup (затыкание конкретной дыры, не общее решение — плохо)? Перезагружать test runner по возникновению exceptions не из списка разрешённых? Повысить severity для тестов, проваленных по таким причинам, чтобы чинить их первыми? Ещё что-то?
humbug
Надо писать тесты на тесты. Неужели это не очевидно?
humbug
Ну так надо писать тесты на тесты. Они написали некий глючный функционал, который вызывает тесты, а потом ругают assert. Молодцы, чо.
lair
В юнит-тестах продолжать выполнение других тестов помимо упавшего — всегда было нормально.
aamonster
Маленькая тонкость: не упавшего (crashed), а проваленного (failed).
В случае крэша (а assert следует рассматривать именно как крэш) максимум — можно перезапустить процесс со следующего теста. Ну, это от фреймворка для тестов зависит, конечно. Но в любом случае — нужно переинициализировать окружение, а не полагаться на "зачистку", выполняемую заведомо сломанным кодом.
lair
А как вы одно от другого отличаете? Ну вот по упавшему процессу можно, наверное, да. Но здесь же не тот случай.
А почему, собственно?
Окружение? Какое такое окружение в юнит-тестах?
aamonster
Отличаю просто: assert — только для фатальных ошибок. Рассматриваю ассерт как полезную разновидность крэша.
А для проверки на fail в тестах в известных мне тестовых фреймворках есть свои макросы (сорри, я из мира C++), не assert.
Окружение — ну, мы живём не в идеальном мире. Вон, у автора статьи транзакция незавершённая остаётся.
В идеальном мире мы бы каждый тест стартовали на новой машине с системой в исходном состоянии. Собственно, это можно сделать (виртуалка), но дорого (тесты слишком долго выполняться будут). Приходится обходиться тем, что есть.
lair
Это вы не тесты упавшие отличаете, а что в коде написано. А тесты падают с еще кучей разных вариантов, и так как же вы будете отличать crash от fail?
Более того, вот кто-то на входе в метод параметры assert x != null, а кто-то — guard.notnull(x), и почему это должно по-разному отражаться на тестах?
Ииии что? Типа, все, что не дошло до такого "макроса" (которые, кстати, принято называть assertions, и пишутся они вполне себе assert-что-то) — crash? Ну так у вас неимоверное количество тестов будет crashed, это непродуктивно.
Ну так у него и не очень юнит-тесты, прямо скажем.