assert(false) not failing in tests but it should

[synaptic]

Description

Public service announcement: I tried building from source to make some slight changes. I determined by running the tests that assert(false) was not failing though it should. One test in particular [0] calls state.getRigidlyConnectedParentLinkModel(...) successfully though it should fail since there are dirty link transforms and that method internally calls assert(checkLinkTransforms()) [1]. Since that second method should return false, the test should fail, but it is currently passing.

Looking further I found that assert() relies on the NDEBUG macro being undefined to work correctly. If NDEBUG is defined, the assertion is disabled, and assert() does nothing [2]. I wrote some test code to determine if NDEBUG is defined, and it is -- though I cannot determine where.

This is causing the CI tests to run in such a way that assert(false) statements are silently passing, which is not good. Probably there are not many of these, but the one I linked to should fail.

[0] https://github.com/moveit/moveit2/blob/main/moveit_core/robot_state/test/robot_state_test.cpp#L785 [1] https://github.com/moveit/moveit2/blob/main/moveit_core/robot_state/src/robot_state.cpp#L1309 [2] https://en.cppreference.com/w/cpp/error/assert

ROS Distro

Rolling

OS and version

Ubuntu 24.04

Source or binary build?

Source

If binary, which release version?

No response

If source, which branch?

main

Which RMW are you using?

None

Steps to Reproduce

Run the test I mentioned.

Expected behavior

The test should fail.

You can force it to fail by putting this at the top of the test I mentioned [0], put this at the top to make NDEBUG undefined. Then run the test and it will fail.

#undef NDEBUG

[0] https://github.com/moveit/moveit2/blob/main/moveit_core/robot_state/test/robot_state_test.cpp#L785

If you want to fix that particular test so it doesn't fail when NDEBUG is undefined, call this just before the line that is failing:

state.updateLinkTransforms();
EXPECT_EQ(state.getRigidlyConnectedParentLinkModel("link_b"), link_a);

Actual behavior

The test does not currently fail.

Backtrace or Console output

No response

[TSNoble]

Ah I think this explains what I'm seeing in #3103. After looking at the code I concluded the same thing -- the latter part of that test should be consistently failing, but only does so on the pipeline and not locally. I've fixed that test by updating the transforms, but that is concerning. I'm glad I wasn't going insane 😅

[TSNoble]

It appears to be a macro variable defined by cmake depending on the CMAKE_BUILD_TYPE argument. Release should set NDEBUG, Debug should not.

I'll try building with these options and check the test results

[TSNoble]

colcon build --cmake-args -DBUILD_TESTING=ON --packages-up-to moveit_core
colcon build --cmake-args -DBUILD_TESTING=ON -DCMAKE_BUILD_TYPE=Debug --packages-up-to moveit core

The tests pass after building with the former, and fail (expectedly) after building with the latter.

It feels like very unintuitive behavior (though I suppose these are the kinds of issues that crop up when using three build systems stacked together in a trench coat 😛 )

Maybe -DBUILD_TESTING=ON should set -DCMAKE_BUILD_TYPE=Debug by default?

[mikeferguson]

I don't think we want to default to Debug - that will likely slow things down considerably since you don't get optimizations with that (for some things it can be catastrophic - I remember at one point we determined that the sensor_msgs::PointCloudIterator is about 300x slower in Debug than in Release/RelWithDebInfo)

[TSNoble]

The alternative is that failing tests pass, which also feels catastrophic. In your example, are you wanting to set BUILD_TESTING on? If so, maybe the better approach is to replace the assert() call with something that isn't NDEBUG dependent, or perhaps remove that call entirely (it doesn't feel unreasonable to want to validate link transforms outside of debug mode)