Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for util unit tests #288

Merged
merged 44 commits into from
Aug 26, 2024
Merged

Fixes for util unit tests #288

merged 44 commits into from
Aug 26, 2024

Conversation

tsayukov
Copy link
Collaborator

@tsayukov tsayukov commented Aug 8, 2024

Часть коммитов здесь затрагивает только CMake файлы, а самые последние коммиты – это с изменениями в util – скорее иллюстрация, как я предлагаю решить проблему. Перед финальным мёржем эти изменения собираюсь откатить.

Здесь собрал все оставшиеся юнит-тесты из util, а именно:

В списке #146 не было util/draft, но если ничего не пропустил, эта либа нигде не используется, может, убрать?

Теперь к деталям.

Падающие тесты:

  • В util/system, а именно, в src_root_ut и src_location_ut проверялась корректная работа макроса __SOURCE_FILE__, который брал макро переменные ARCADIA_ROOT или ARCADIA_BUILD_ROOT и обрезал у __FILE__ совпадающий префикс. ARCADIA_ROOT и ARCADIA_BUILD_ROOT передавались через -D опцию как ${YDB_SDK_SOURCE_DIR} и ${YDB_SDK_BINARY_DIR} соответственно, и это всегда абсолютные пути. Однако если __FILE__ вернёт относительный путь, тогда всё сломается. В нашем случае это происходило из-за использования ccache вместе с переменной окружения CCACHE_BASEDIR, а именно, "if you specify an absolute path to the source code file, __FILE__ macros will be expanded to a relative path instead". Не вижу, как можно заранее понять на стороне CMake, что потом будет использоваться CCACHE_BASEDIR, поэтому пока захаркодил нужные пути в build/action.yaml.
  • В util/random/common_ops_ut падал тест TestStlCompatibility. Насколько я понимаю, здесь тестировалась совместимость с STL, то есть чтобы TRng мог передаваться в std::normal_distribution::operator(), и всё компилировалось бы. Для этого достаточно проверок, которые всегда выполняются, – какие предлагаю я. Магические числа в предыдущем варианте скорее всего подбирались таким образом, чтобы IntHash от числа 17 совпадал с результатом от вызова std::normal_distribution::operator(), но возможно, внутренняя реализация поменялась, и теперь это не так.
  • В util/system/direct_io_ut падал тест TDirectIoErrorHandling::Constructor, там проверялось, что при попытке открытия несуществующего файла ./test.file будет выброшено исключение. Но если файл уже существует, то тест падает. Оказалось, такой файл создавался в util/stream/direct_io_ut в функции Test. Поэтому в конце функции я теперь удаляю этот файл, всё равно каждый раз он создаётся заново (из-за опции CreateAlways).
  • В util/system/fstat_ut падали тесты из-за того, что TFile не создаёт для файла родительскую директорию, если такой не существует (по крайней мере open на линуксе не создаёт). Добавил создание и удаление директорий.
  • В util/system/type_name_ut падали тесты, когда проверяли имя типа с вложенными типовыми параметрами, например, std::basic_string<char, std::char_traits<char>, std::allocator<char>>. Внутренняя реализация в libstdc++ ещё добавляет пробел между двумя подряд закрывающимися угловыми скобками. Я решил в util/system/type_name в функции TypeName ещё добавить замену всех > на >.

Отсутствующие библиотеки:

  • Тест util/system/unaligned_mem_ut зависит от отсутствующей либы library/cpp/testing/benchmark, там нужна функция NBench::Clobber, это барьер против оптимизации памяти. Думаю, придётся притащить эту либу. Этот тест я пока закомментировал.
  • Тест util/generic/string_transparent_hash_ut зависит от отсутствующей либы library/cpp/containers/absl_flat_hash. Но там единственное, что нужно, это способность std::unordered_set уметь в transparent в методе find (C++20). Может, просто навесить макросы, как я предлагаю? Ну или нужно притащить либу.

Мелочи:

@tsayukov tsayukov force-pushed the fix/util-unit-tests branch from b034840 to b52f212 Compare August 8, 2024 20:02
@tsayukov tsayukov force-pushed the fix/util-unit-tests branch 2 times, most recently from 1e53829 to e7cdc1e Compare August 12, 2024 15:16
If the variable CCACHE_PATH is already set before the call (as a normal or cache variable) then the search will not occur.
@tsayukov tsayukov force-pushed the fix/util-unit-tests branch from e7cdc1e to e115cc8 Compare August 13, 2024 18:22
@tsayukov tsayukov force-pushed the fix/util-unit-tests branch from e115cc8 to 23c91c3 Compare August 14, 2024 09:08
'direct_io_ut' from 'util/stream' doesn't remove its file, so 'direct_io_ut' from 'util/system' failed checking if the same file doesn't exist
libc 'open' (in implementation of TFile) doesn't create non-existed parent directories in given path, so we have to do that for it
@tsayukov tsayukov force-pushed the fix/util-unit-tests branch from 23c91c3 to 043286f Compare August 14, 2024 12:03
@tsayukov tsayukov marked this pull request as ready for review August 14, 2024 13:22
@tsayukov tsayukov requested a review from Gazizonoki August 14, 2024 13:22
Copy link
Collaborator

@Gazizonoki Gazizonoki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsayukov Привет, по поводу предложений:

  • util/system/direct_io_ut и util/stream/direct_io_ut. Бинари к этим тестам располагались в разных директориях, тут и тут, поэтому коллизий не было. Лучше разнеси сами тесты
  • util/system/fstat_ut. Наша система сборки Ya Make сама перед запуском тестов создает пути WorkPath и OutputPath. В данном случае лучше сделать это через CMake, в виде необязательного параметра в add_ydb_test
  • util/system/type_name_ut. Этот тест не должен запускаться с системной стандартной библиотекой, он явно выключен. Просто не подключай его
  • util/system/unaligned_mem_ut тут придется возвращать либу, заведи issue на этот тест, права вроде у тебя есть
  • util/generic/string_transparent_hash_ut во внутреннем репозитории у нас уже C++20, даже макросы не нужны. Замена должна пройти достаточно просто, этот фикс я донесу к нам
  • util/random/common_ops_ut все же на мой взгляд проверок на компилируемость тут недостаточно, такой фикс вряд ли пропустят, этот тест можно не запускать совсем, сделай макрос, который будет этот тест выключать

@tsayukov
Copy link
Collaborator Author

tsayukov commented Aug 22, 2024

@Gazizonoki Привет, добавил в add_ydb_test три новых параметра:

  • WORKING_DIRECTORY — вспомнил, что у add_test есть такой полезный параметр, — таким образом разнёс по разным директориям не только исполнение тестов stream и system, но и всех остальных. Всё-таки изначально предполагалось, что они все вместе не будут исполняться по одному пути.
  • OUTPUT_DIRECTORY для создания OutputPath, чтобы работал тест util/system/fstat_ut.
  • TEST_ARG пробрасывает аргументы для экзешника теста дальше в add_yunittest. Может быть в принципе полезно передавать такие аргументы в add_ydb_test, а в данном случае я просто отключил конкретный сломанный тест-кейс в util/random/common_ops_ut.

Открыл issue:

Также добавил новых пресетов.

@tsayukov tsayukov changed the title [Not for merging] Fixes for util unit tests Fixes for util unit tests Aug 22, 2024
@Gazizonoki
Copy link
Collaborator

Вынеси куда нибудь все фиксы, которые надо донести. Так мне будет удобнее

@Gazizonoki Gazizonoki merged commit 5383f27 into main Aug 26, 2024
7 checks passed
@Gazizonoki Gazizonoki deleted the fix/util-unit-tests branch August 26, 2024 18:47
@Gazizonoki
Copy link
Collaborator

Gazizonoki commented Aug 26, 2024

@tsayukov Заведи issue на патча теста string_transparent_hash_ut, пока я донести фикс не могу, руки не доходят, а то, что это надо сделать, потеряется. Если получится, было бы круто приложить к issue сам патч

@tsayukov
Copy link
Collaborator Author

@tsayukov Заведи issue на патча теста string_transparent_hash_ut, пока я донести фикс не могу, руки не доходят, а то, что это надо сделать, потеряется. Если получится, было бы круто приложить к issue сам патч

@Gazizonoki Готово! Я тебя отметил в issues, как assignee, если ты не против...
Ещё, если не видел, составил для тебя список, как ты просил.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants