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

Reformat rmw_impl_id_check to call a testable function #725

Merged
merged 9 commits into from
Jul 27, 2020
35 changes: 35 additions & 0 deletions rcl/include/rcl/rmw_implementation_identifier_check.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2020 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef RCL__RMW_IMPLEMENTATION_IDENTIFIER_CHECK_H_
#define RCL__RMW_IMPLEMENTATION_IDENTIFIER_CHECK_H_

#ifdef __cplusplus
extern "C"
{
#endif

#include "rcl/visibility_control.h"

#define RMW_IMPLEMENTATION_ENV_VAR_NAME "RMW_IMPLEMENTATION"
#define RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME "RCL_ASSERT_RMW_ID_MATCHES"

RCL_PUBLIC
rcl_ret_t rcl_rmw_implementation_identifier_check(void);

#ifdef __cplusplus
}
#endif

#endif // RCL__RMW_IMPLEMENTATION_IDENTIFIER_CHECK_H_
53 changes: 29 additions & 24 deletions rcl/src/rcl/rmw_implementation_identifier_check.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ extern "C"

#include "rcl/types.h"

#define RMW_IMPLEMENTATION_ENV_VAR_NAME "RMW_IMPLEMENTATION"
#define RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME "RCL_ASSERT_RMW_ID_MATCHES"
#include "rcl/rmw_implementation_identifier_check.h"

// Extracted this portable method of doing a "shared library constructor" from SO:
// http://stackoverflow.com/a/2390626/671658
Expand All @@ -55,7 +54,8 @@ extern "C"
static void f(void)
#endif

INITIALIZER(initialize) {
rcl_ret_t rcl_rmw_implementation_identifier_check(void)
{
// If the environment variable RMW_IMPLEMENTATION is set, or
// the environment variable RCL_ASSERT_RMW_ID_MATCHES is set,
// check that the result of `rmw_get_implementation_identifier` matches.
Expand All @@ -66,18 +66,17 @@ INITIALIZER(initialize) {
RMW_IMPLEMENTATION_ENV_VAR_NAME,
&expected_rmw_impl_env);
if (NULL != get_env_error_str) {
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME,
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
"Error getting env var '" RCUTILS_STRINGIFY(RMW_IMPLEMENTATION_ENV_VAR_NAME) "': %s\n",
get_env_error_str);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Blast545 meta: why not using RCL_SET_ERROR_MSG_WITH_FORMAT and logging in the caller? I'd think that'd make the test less noisy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean noisy in the context? isn't the check logging only in the error condition? (if any)

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, error conditions that we will test for. It's even easier to assert on error messages if we defer logging.

Copy link
Contributor Author

@Blast545 Blast545 Jul 23, 2020

Choose a reason for hiding this comment

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

Addressed with 9931857

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved already

exit(RCL_RET_ERROR);
return RCL_RET_ERROR;
}
if (strlen(expected_rmw_impl_env) > 0) {
// Copy the environment variable so it doesn't get over-written by the next getenv call.
expected_rmw_impl = rcutils_strdup(expected_rmw_impl_env, allocator);
if (!expected_rmw_impl) {
RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "allocation failed");
exit(RCL_RET_BAD_ALLOC);
RCL_SET_ERROR_MSG("allocation failed");
return RCL_RET_BAD_ALLOC;
}
}

Expand All @@ -86,31 +85,29 @@ INITIALIZER(initialize) {
get_env_error_str = rcutils_get_env(
RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, &asserted_rmw_impl_env);
if (NULL != get_env_error_str) {
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME,
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
"Error getting env var '"
RCUTILS_STRINGIFY(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME) "': %s\n",
get_env_error_str);
exit(RCL_RET_ERROR);
return RCL_RET_ERROR;
}
if (strlen(asserted_rmw_impl_env) > 0) {
// Copy the environment variable so it doesn't get over-written by the next getenv call.
asserted_rmw_impl = rcutils_strdup(asserted_rmw_impl_env, allocator);
if (!asserted_rmw_impl) {
RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "allocation failed");
exit(RCL_RET_BAD_ALLOC);
RCL_SET_ERROR_MSG("allocation failed");
return RCL_RET_BAD_ALLOC;
}
}

// If both environment variables are set, and they do not match, print an error and exit.
if (expected_rmw_impl && asserted_rmw_impl && strcmp(expected_rmw_impl, asserted_rmw_impl) != 0) {
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME,
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
"Values of RMW_IMPLEMENTATION ('%s') and RCL_ASSERT_RMW_ID_MATCHES ('%s') environment "
"variables do not match, exiting with %d.",
expected_rmw_impl, asserted_rmw_impl, RCL_RET_ERROR
);
exit(RCL_RET_ERROR);
return RCL_RET_ERROR;
}

// Collapse the expected_rmw_impl and asserted_rmw_impl variables so only expected_rmw_impl needs
Expand All @@ -130,31 +127,39 @@ INITIALIZER(initialize) {
// If either environment variable is set, and it does not match, print an error and exit.
if (expected_rmw_impl) {
const char * actual_rmw_impl_id = rmw_get_implementation_identifier();
const rcutils_error_string_t rmw_error_msg = rcl_get_error_string();
rcl_reset_error();
if (!actual_rmw_impl_id) {
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME,
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
"Error getting RMW implementation identifier / RMW implementation not installed "
"(expected identifier of '%s'), with error message '%s', exiting with %d.",
expected_rmw_impl,
rcl_get_error_string().str,
rmw_error_msg.str,
RCL_RET_ERROR
);
rcl_reset_error();
Blast545 marked this conversation as resolved.
Show resolved Hide resolved
exit(RCL_RET_ERROR);
return RCL_RET_ERROR;
}
if (strcmp(actual_rmw_impl_id, expected_rmw_impl) != 0) {
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME,
RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
"Expected RMW implementation identifier of '%s' but instead found '%s', exiting with %d.",
expected_rmw_impl,
actual_rmw_impl_id,
RCL_RET_MISMATCHED_RMW_ID
);
exit(RCL_RET_MISMATCHED_RMW_ID);
return RCL_RET_MISMATCHED_RMW_ID;
}
// Free the memory now that all checking has passed.
allocator.deallocate((char *)expected_rmw_impl, allocator.state);
}
return RCL_RET_OK;
}

INITIALIZER(initialize) {
rcl_ret_t ret = rcl_rmw_implementation_identifier_check();
if (ret != RCL_RET_OK) {
RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "%s\n", rcl_get_error_string().str);
exit(ret);
}
}

#ifdef __cplusplus
Expand Down
8 changes: 8 additions & 0 deletions rcl/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,14 @@ function(test_target_function)
AMENT_DEPENDENCIES ${rmw_implementation} "osrf_testing_tools_cpp" "test_msgs"
)

rcl_add_custom_gtest(test_rmw_impl_id_check_func${target_suffix}
SRCS rcl/test_rmw_impl_id_check_func.cpp
ENV ${rmw_implementation_env_var}
APPEND_LIBRARY_DIRS ${extra_lib_dirs}
LIBRARIES ${PROJECT_NAME}
AMENT_DEPENDENCIES ${rmw_implementation}
)

# Launch tests

rcl_add_custom_executable(service_fixture${target_suffix}
Expand Down
70 changes: 70 additions & 0 deletions rcl/test/rcl/test_rmw_impl_id_check_func.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2020 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <gtest/gtest.h>

#include "rcutils/env.h"

#include "rcl/error_handling.h"
#include "rcl/rcl.h"
#include "rcl/rmw_implementation_identifier_check.h"

TEST(TestRmwCheck, test_rmw_check_id_impl) {
EXPECT_EQ(RCL_RET_OK, rcl_rmw_implementation_identifier_check());
}

TEST(TestRmwCheck, test_failing_configuration) {
const char * expected_rmw_impl_env = NULL;
const char * expected_rmw_id_matches = NULL;

const char * get_env_var_name = rcutils_get_env(
RMW_IMPLEMENTATION_ENV_VAR_NAME,
&expected_rmw_impl_env);

const char * get_env_id_matches_name = rcutils_get_env(
RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME,
&expected_rmw_id_matches);

// Fail test case, reason: RMW_IMPLEMENTATION_ENV_VAR_NAME set, not matching rmw impl
EXPECT_TRUE(rcutils_set_env(RMW_IMPLEMENTATION_ENV_VAR_NAME, "some_random_name"));
EXPECT_TRUE(rcutils_set_env(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, ""));
EXPECT_EQ(RCL_RET_MISMATCHED_RMW_ID, rcl_rmw_implementation_identifier_check());
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();

// Fail test case, reason: RMW_IMPLEMENTATION_ENV_VAR_NAME set, not matching rmw impl
EXPECT_TRUE(rcutils_set_env(RMW_IMPLEMENTATION_ENV_VAR_NAME, ""));
EXPECT_TRUE(rcutils_set_env(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, "some_random_name"));
EXPECT_EQ(RCL_RET_MISMATCHED_RMW_ID, rcl_rmw_implementation_identifier_check());
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();

// Fail test case, reason: env variables not equal
EXPECT_TRUE(rcutils_set_env(RMW_IMPLEMENTATION_ENV_VAR_NAME, "some_random_name"));
EXPECT_TRUE(rcutils_set_env(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, "diff_random"));
EXPECT_EQ(RCL_RET_ERROR, rcl_rmw_implementation_identifier_check());
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();

// Fail test case, reason: equal env variables do not match rmw impl
EXPECT_TRUE(rcutils_set_env(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, "some_random_name"));
EXPECT_TRUE(rcutils_set_env(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, "some_random_name"));
EXPECT_EQ(RCL_RET_MISMATCHED_RMW_ID, rcl_rmw_implementation_identifier_check());
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();

// Restore env variables set in the test
EXPECT_TRUE(rcutils_set_env(RMW_IMPLEMENTATION_ENV_VAR_NAME, get_env_var_name));
EXPECT_TRUE(rcutils_set_env(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, get_env_id_matches_name));
}