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

[no squash] Fix building unittests & benchmarks by upgrading Catch2 #14735

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

appgurueu
Copy link
Contributor

@appgurueu appgurueu commented Jun 7, 2024

Fixes #14692, alternative to #14693, by simply upgrading Catch2 to v3. Catch uses a separate implementation and header now, which helps resolve the issue. This uses the amalgamation.

Bonus: Catch2 version upgrade.

Small change (I consider this an improvement, but you may differ): Gets rid of #define CATCH_CONFIG_CONSOLE_WIDTH 160. I think this (arbitrarily?) chosen number is too large. It is also inconsistent with unit tests. Using the Catch default (80) here is better (and simpler) IMO.

Fun fact: You can run benchmarks via --run-unittests --test-module <benchmark> now. Catch2 also prints "Randomness seeded to: <seed>" now.

To review, look at the second commit (8b02699). The first commit updates the vendored copy, which causes a lot of noise.

How to test

  1. Compile with unittests and benchmarks enabled.
  2. Confirm that --run-unittests and --run-benchmarks work as expected.
  3. Confirm that both still work as expected if --test-module is used.

@appgurueu appgurueu added @ Build CMake, build scripts, official builds, compiler and linker errors Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Bugfix 🐛 PRs that fix a bug labels Jun 7, 2024
@sfan5
Copy link
Member

sfan5 commented Jun 7, 2024

ps. Catch did not go from 20kloc to 200kloc. The git diff is largely from Catch2 test files (text files). Removing the tests would reduce the size of Catch2 by ~5 MB.

Importing 200kLOC is a terrible idea.
We should either vendor the amalgated source, require catch2 to already be installed or consider a submodule. (in order of personal preference)

Downloading content at build time is a nuisance, so FetchContent is not worthy of consideration.

  • Gets rid of our "modification" to Catch which forced everything to be formatted as microseconds - now it will format as ns, µs, ms, etc. as appropriate. I prefer this.

I'm pretty sure this was required to make the automated benchmarks work correctly.

@sfan5 sfan5 changed the title Fix building unittests & benchmarks by upgrading Catch2 [no squash] Fix building unittests & benchmarks by upgrading Catch2 Jun 7, 2024
@appgurueu
Copy link
Contributor Author

appgurueu commented Jun 7, 2024

Alright, I'll go with the amalgamated source, and re-apply the patch to output everything in microseconds.

CI reveals that the return value of Catch::Session().run(...) is just an exit code now, meaning it fails when there are no tests to run. However we can simply tell catch to consider zero tests a success via the config.

@appgurueu
Copy link
Contributor Author

Alright, switched to the amalgamation. The custom cout / cerr / clog pose a problem, because they cause ODR definition violations vs. Catch internals. We need to tell Catch to always use our redefinitions (for benchmarks as well) which just use rawstream, which is what I did now.

src/catch.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

Thank you for fixing the ability to build unittests and benchmarks together. This is a nice clean solution. I will be closing #14693 in favor of this.

lib/catch2/CMakeLists.txt Outdated Show resolved Hide resolved
src/catch.h Outdated Show resolved Hide resolved
src/catch.h Show resolved Hide resolved
src/unittest/test.cpp Outdated Show resolved Hide resolved
@appgurueu appgurueu force-pushed the fix-build branch 2 times, most recently from 3479e71 to ab026e8 Compare June 7, 2024 18:58
Copy link
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

LGTM

@appgurueu appgurueu mentioned this pull request Jun 8, 2024
5 tasks
src/unittest/test.cpp Outdated Show resolved Hide resolved
@sfan5
Copy link
Member

sfan5 commented Jun 10, 2024

I also suggest changing the author of the first commit to something artificial to not skew the LoC change statistics.

@appgurueu
Copy link
Contributor Author

I also suggest changing the author of the first commit to something artificial to not skew the LoC change statistics.

Done, thanks for the reminder. (I remember having done this initially, but it probably got lost when I switched to the amalgamation, redoing the commit.)

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

CI green = good

@appgurueu appgurueu merged commit ae4cd1e into minetest:master Jun 10, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug @ Build CMake, build scripts, official builds, compiler and linker errors Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't build benchmark and unit tests at the same time
3 participants