fix: correct ASSERT_EQ to ASSERT_NE in admittance controller load test

[SouriRishik]

Fixes #1258

Problem

The test_load_controller test in admittance_controller was using ASSERT_EQ to check against nullptr, which means the test was asserting that the controller fails to load. Since the controller loads successfully with the params file provided, the test was always passing, even though it was checking the wrong condition.

Fix

Changed ASSERT_EQ to ASSERT_NE so the test correctly asserts that the controller loaded successfully (i.e. the result is not nullptr).

Note: range_sensor_broadcaster already has the correct ASSERT_NE in its load test and does not need changes.

Is this user-facing behavior change?

no

Did you use Generative AI?

no

[thedevmystic]

Looks good, but if previous ASSERT_EQ() was passing so now this ASSERT_NE() would certainly fail, won't it?

[SouriRishik]

Looks good, but if previous ASSERT_EQ() was passing so now this ASSERT_NE() would certainly fail, won't it?

Good question. The previous ASSERT_EQ(..., nullptr) was the bug because it was asserting that the controller fails to load, which is the wrong condition. Since the controller does load successfully with the params file provided, the result is a valid pointer and not nullptr. ASSERT_NE(..., nullptr) correctly verifies that the controller loaded successfully. The CI will confirm this.

[saikishor]

That's weird. How come the tests were passing till now? 🤔

[thedevmystic]

That's weird. How come the tests were passing till now?

I think the controller must be failing to load (returning nullptr). It's the only way to pass this. That means the there is also a bug.

[SouriRishik]

If the previous ASSERT_EQ(..., nullptr) was passing, that suggests load_controller() might actually be returning nullptr, meaning the controller isn't loading successfully.

I'll verify the actual return value and investigate whether this is due to a configuration/plugin issue or something in the test setup itself, rather than just the assertion.

I'll update the PR once I confirm the behavior.

[SouriRishik]

Verified that load_controller() returns nullptr, so the controller is not loading in this setup.

Reverted the assertion and added an invalid controller test to ensure failure cases are properly covered.

[thedevmystic]

The ASSERT_NE() in first test is correct as per of logic. So, reverting is not right as it is a bug.

[SouriRishik]

okay I'll change that again but what exactly do i follow up, I added additional test function for an invalid controller configuration to explicitly verify failure cases, so both success and failure paths are covered. What do you think of that?

[thedevmystic]

From what I see, we have a bug somewhere. Since ASSERT_EQ(cm.load_controller("load_admittance_controller"), nullptr); was passing, ASSERT_NE() will fail. But the catch is ASSERT_NE() is logically correct.

Let's see what maintainers has to say :)

[kamal2730]

Add missing parameters and test locally by running:

colcon test --packages-select admittance_controller --event-handlers console_direct+

[SouriRishik]

Hello @christophfroehlich, I have made the required changes. Could you please review?

[kamal2730]

LGTM!