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)

[TSNoble]

@sea-bass I'm thinking of addressing this by replacing all asserts with exceptions, such that they can't be accidentally disabled by build type.

What's the general policy on using std::runtime_error vs custom exception types?

[TSNoble]

Actually, to avoid breaking behaviour it might make more sense to print an error, and let the tests use ASSERT_TRUE. That way, users making use of assert-based functions won't be presented with a bunch of new exceptions to catch

[TSNoble]

It looks like there are 106 usages of assert across 31 files. It also seems that assert calls std::abort on failure, so maybe an exception would be preferable.

It might be nicest to define a macro which provides identical behaviour, minus the turn-off with NDEBUG, and simply do a find and replace.

something like:

// We avoid calling `assert` directly, as some tests rely on these calls.
// `assert` is disabled automatically by CMake when the build type is
// Release, which can cause failing tests to pass. See #3102
#define ALWAYS_ASSERT(condition)                                           \
   if (!condition) {                                                       \
      std::err << "Assertion '" << #condition << "' failed." << std::endl; \
      std::abort();                                                        \
   }

My preprocessor knowledge is a little rusty 😅

I believe this preserves assert behaviour, i.e.:

• Checks if the condition evaluates to 0 (i.e. falsy condition)
• Outputs to std::err and calls std::abort if so

@sea-bass what do you think?

Edit: I think there are a few cases where the code does assert(x) == y, which might break with this macro
Edit Edit: This is not the case. I misread assert(x == y) as assert(x) == y 😅

[sea-bass]

I might let @henningkayser chime in on this one... it may be beyond my knowledge of computers :)

[mikeferguson]

So, I can see changing asserts in tests - but we should be very careful about doing this sort of change outside the tests. Typically, the "it goes away in release mode" is actually a feature that someone intended when they use an assert - especially in high performance code.

For the particular assert() you called out in RobotState - developers would want to verify they never have dirty transforms when they are creating a new feature, but at run time these RobotState functions get called many times during each plan generation and the overhead of always checking transforms can significantly slow down planning. Remember, you aren't just adding in the assert boolean check here - you're also adding the whole call to checkLinkTransforms() - because in release mode that function call is currently being removed entirely.

Actionable thoughts:

• We should probably make our CI build in a debug mode - so that we catch these asserts (but, we might have to resolve some timing issues when doing that change since things will run slower)
• We should probably add documentation about "test your stuff in debug" for newer developers of MoveIt - so they know about the use of these asserts (honestly, much of RobotState is so highly optimized internally, there are also lots of warnings in the C++ API docs that should be called out more prominently in developer onboarding docs).
• Anywhere we actually want to change asserts in the regular codebase (anything that isn't tests), you would want to profile the impact of those changes on runtime.

[sea-bass]

Does building with RelWithDebInfo give the desired (correct) test failures? That would be better than just Debug.

[mikeferguson]

RelWithDebInfo is allegedly same as Release, but with debugging symbols (so it's still optimized, and assertions are disabled) - I believe it is also what our Debians are built with by default (so that there are still some symbols when you gdb things).

For the CI, we could do something like start with Debug and then add some optimization flags back in, while keeping assert()...

[TSNoble]

Would it be possible to use macros to determine if a build-type-dependent call is being made as part of a test, and produce a warning if this is the case? I feel like this would help developers understand these effects when writing tests, and prompt them to design the tests to use functionality that is globally available?

(this is unrelated to the CI jobs, but based on the responses I think I agree that using Debug is the best option if possible)

[TSNoble]

So, I can see changing asserts in tests - but we should be very careful about doing this sort of change outside the tests. Typically, the "it goes away in release mode" is actually a feature that someone intended when they use an assert - especially in high performance code.

For the particular assert() you called out in RobotState - developers would want to verify they never have dirty transforms when they are creating a new feature, but at run time these RobotState functions get called many times during each plan generation and the overhead of always checking transforms can significantly slow down planning. Remember, you aren't just adding in the assert boolean check here - you're also adding the whole call to checkLinkTransforms() - because in release mode that function call is currently being removed entirely.

Actionable thoughts:

• We should probably make our CI build in a debug mode - so that we catch these asserts (but, we might have to resolve some timing issues when doing that change since things will run slower)
• We should probably add documentation about "test your stuff in debug" for newer developers of MoveIt - so they know about the use of these asserts (honestly, much of RobotState is so highly optimized internally, there are also lots of warnings in the C++ API docs that should be called out more prominently in developer onboarding docs).
• Anywhere we actually want to change asserts in the regular codebase (anything that isn't tests), you would want to profile the impact of those changes on runtime.

Regarding point 2, I've been wanting to create a Getting Started guide for new developers (see https://github.com/orgs/moveit/discussions/3096). Feedback would be appreciated!

[rhaschke]

I fully agree with #3102 (comment):

• assert in tests can be (and probably should be) replaced by ASSERT_TRUE
• assert in production code should not be touched

To evaluate those tests in CI we have Debug enabled for the ccov build (which requires Debug anyway):

moveit2/.github/workflows/ci.yaml

Line 56 in 2490e51

-DCMAKE_BUILD_TYPE=${{ matrix.env.CCOV && 'Debug' || 'Release'}}

[TSNoble]

To evaluate those tests in CI we have Debug enabled for the ccov build (which requires Debug anyway):

moveit2/.github/workflows/ci.yaml

@rhaschke This is causing failing tests slip through the cracks between PR CI and main CI though, due to a difference in the build type. See for example this CI job on main and the corresponding jobs on the associated PR.

I agree that asserts in test are a bad practice and I see the point about not wanting to enable asserts in production code, however, the issue is more that some tests do call assert indirectly via source code functions that doy.

To me, this is more of an issue of incorrect test design, and I think in these situations we should print a warning to make it clear that the test will have unreliable behavior (though I'm unsure quite how this would be implemented).

[rhaschke]

To evaluate those tests in CI we have Debug enabled for the ccov build (which requires Debug anyway):

This is causing failing tests slip through the cracks between PR CI and main CI though, due to a difference in the build type. See for example this CI job on main and the corresponding jobs on the associated PR.

The problem is that ccov-testing was disabled in #2866, which is why there is no Debug build run on PRs anymore.

There is no need to change code, but just re-enable ccov/debug testing for PRs.
This will be fixed with #3115.

[TSNoble]

Ahh I see, and thank you for looking into those issues 👍

Would you agree that it's a bad testing practice to be using Debug dependent code indirectly in tests? My assumption was that it's valid to want to run tests with a Release build (and reasonable to expect them to behave identically), I'd still argue the latter point -- provided the former -- but I suppose Release does imply post-development (and testing), so maybe this assumption is incorrect.