-
Notifications
You must be signed in to change notification settings - Fork 33
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
More spdlog tests #40
Conversation
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
} else if (severity == 0 && level == 10) { | ||
// This is a special case - not sure what the right behavior is | ||
expected_log << ss.str() << std::endl; |
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 called this out on Slack - not sure what the expected behavior should be here, so for now I'm just asserting the current behavior.
rcl_logging_spdlog/test/fixtures.hpp
Outdated
std::stringstream dir_command; | ||
dir_command << "dir"; | ||
#ifdef _WIN32 | ||
dir_command << " /B"; | ||
#endif | ||
dir_command << " " << (log_dir / get_expected_log_prefix()).string() << "*"; | ||
|
||
FILE * fp = popen(dir_command.str().c_str(), "r"); | ||
if (nullptr == fp) { | ||
throw std::runtime_error("Failed to glob for log files"); | ||
} | ||
|
||
char raw_line[2048]; | ||
while (fgets(raw_line, sizeof(raw_line), fp) != NULL) { | ||
pclose(fp); | ||
|
||
std::string line(raw_line); | ||
fs::path line_path(line.substr(0, line.find_last_not_of(" \t\r\n") + 1)); | ||
// This should be changed once ros2/rcpputils#68 is resolved | ||
return line_path.is_absolute() ? line_path : log_dir / line_path; | ||
} | ||
|
||
pclose(fp); |
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.
Pretty much all of this should be replaced with properly iterating the directory content to find the log file, but that functionality isn't available in the filesystem helper. So either we need to implement it, or we need to bump to C++17, because this is kinda gross. (Post-Foxy)
rcl_logging_spdlog/test/fixtures.hpp
Outdated
char * exe_name = rcutils_get_executable_name(allocator); | ||
if (nullptr == exe_name) { | ||
throw std::runtime_error("Failed to determine executable name"); | ||
} | ||
std::stringstream prefix; | ||
prefix << exe_name << "_" << | ||
rcutils_get_pid() << "_"; | ||
allocator.deallocate(exe_name, allocator.state); |
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.
We should support get_executable_name
in rcpputils to avoid the need for an allocator here. Post-Foxy, of course.
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
I haven't reviewed the changes closely, but you have my blessing to merge pending approval. Since it's only test code, I think it's okay delay it's release into Foxy. |
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
31f5f67
to
a68f7ae
Compare
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.
LGTM
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
As suggested in #36 (comment), some of this approach takes after #37.