-
-
Notifications
You must be signed in to change notification settings - Fork 133
Catch2 update #246
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
Catch2 update #246
Conversation
In order to update catch2 the following changes were made: - CMakeLists.txt: import catch2 with FetchContent_Declare, this will make it more flexible and easier to update - removed main for tests, since it is not mandatory and it doesn't add any value of having it - renaming includes in all the files
codecov-commenter
commented
Feb 13, 2025
Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 95.53%. Comparing base (
4a02bfc) to head (6050833).
Additional details and impacted files
@@ Coverage Diff @@ ## master #246 +/- ## ======================================= Coverage 95.53% 95.53% ======================================= Files 16 16 Lines 1075 1076 +1 ======================================= + Hits 1027 1028 +1 Misses 48 48
☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.
One of the reasons why I suggest introducing this new version of Catch2 is because I noticed that valgrind is unable to detect memory leaks caused by missing virtual destructor.
A practical example is the following. You can test this by running the tests this way valgrind --tool=memcheck --leak-check=yes --error-exitcode=1 bin/test-ArduinoCore-API -s '[printable-delete-01]'
#include <catch.hpp> #include <api/IPAddress.h> TEST_CASE("Testing polymorphic IPAddress memory free", "[printable-delete-01]") { arduino::Printable* p = new IPAddress(); #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor" delete p; #pragma GCC diagnostic pop }
test/src/String/test_move.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this surely makes the test pass, I'm quite sure the test is not about self assignment anymore 🙃 If there's an issue with memory leaking from this we should solve it at core level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the point of this test case, I'll drop this commit. I'll look if I can improve the preprocessor checks, since I am failing this locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Gcc online docs at 3.7.2 GCC_VERSION is not defined in the way we are using it. I think we could check against __GNUC__ only.
I don't understand why on my local machine GCC_VERSION is not defined, but __GNUC__ is.
error: moving ‘a’ of type ‘arduino::String’ to itself [-Werror=self-move] Which may happen if if GCC_VERSION is not defined, and instead __GNUC__ is defined
6050833 to
e97e5e2
Compare
since for catch2 '[' is not a valid character in the tag name
77e6218 to
a919d81
Compare
This PR aims to provide a newer version for catch2 that is directly integrated in CMake with improved functionalities.
This also solves the issue of
-Werror=self-movehappening ontest/src/String/test_move.cpp:45:7happening on some compilers.