Skip to content

Cleaner logging tests #11

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

Merged
merged 41 commits into from
Nov 18, 2024
Merged

Conversation

mxgrey
Copy link

@mxgrey mxgrey commented Nov 5, 2024

This PR takes advantage of rcutils_logging_set_output_handler to override the way log outputs are published within the logging test function. We prevent the messages from being printed and instead collect them in way that the test can access them and validate what is being produced.

This has two big advantages:

  • We no longer get false "Error" and "Warning" messages printing out to the terminal when running these tests
  • We can explicitly validate that the logger is behaving the way we expect

This branch builds directly off of #7, so that should be merged before this is reviewed.

mxgrey added 27 commits October 30, 2024 12:59
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Copy link
Author

mxgrey commented Nov 5, 2024

I just realized a major issue with some of the changes in this PR.

When you specify a custom log output handler function to rcutils, your handler function will be given the format string ("%s") along with the variadic arguments that the user passed in to fill into the format string.

The variadic arguments are pretty much indecipherable from the Rust side, so in order to make sense of the log messages on the rclrs side, I skipped passing ("%s", msg) to rcutils and instead simplified it to just passing in msg with no variadic arguments.

This is fine in our tests since we can choose to just look at the message and ignore the variadic arguments. However this will be a problem when we're not testing because it's possible for someone to pass a string with a % character into their log message, and when rcl processes that according to its normal formatting rules, it can result in a segmentation fault or other undefined behavior.

I'll need to tweak the PR to have slightly different behavior when compiled for tests vs compiled for real. A better solution would be to find a way to format a string from Rust given C variadic arguments, but I expect that to be an unreasonably difficult thing to do.

@mxgrey
Copy link
Author

mxgrey commented Nov 6, 2024

The safety issue is fixed by 89809d1, so this PR should be good to go.

Copy link
Owner

@geoff-imdex geoff-imdex left a comment

Choose a reason for hiding this comment

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

As always, lots of great stuff (definitely helping my Rust learnings!). One extremely pedantic comment for adding in the standard safety stuff.

mxgrey and others added 4 commits November 12, 2024 11:10
Signed-off-by: Michael X. Grey <[email protected]>
* Use latest stable Rust CI

Signed-off-by: Michael X. Grey <[email protected]>

* Fix quotation in doc

Signed-off-by: Michael X. Grey <[email protected]>

* Fix serde feature for vendored messages

Signed-off-by: Michael X. Grey <[email protected]>

* Fix formatting of lists in docs

Signed-off-by: Michael X. Grey <[email protected]>

* Update minimum supported Rust version based on the currently used language features

Signed-off-by: Michael X. Grey <[email protected]>

* Conform to clippy style guide

Signed-off-by: Michael X. Grey <[email protected]>

* Add rust toolchain as a matrix dimension

Signed-off-by: Michael X. Grey <[email protected]>

* Bump declaration of minimum supported rust version

Signed-off-by: Michael X. Grey <[email protected]>

* Limit the scheduled runs to once a week

Signed-off-by: Michael X. Grey <[email protected]>

* Define separate stable and minimal workflows because matrix does not work with reusable actions

Signed-off-by: Michael X. Grey <[email protected]>

* Reduce minimum version to 1.75 to target Noble

Signed-off-by: Michael X. Grey <[email protected]>

---------

Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
* Update rclrs vendor_interfaces.py script

This updates the vendor_interfaces.py script to also vendor in the
action_msgs and unique_identifier_msgs packages. The script is modified
to always use a list of package names rather than hard-coding the
package names everywhere.

* Silence certain clippy lints on generated code

In case a user enforces clippy linting on these generated packages,
silence expected warnings. This is already the case in rclrs, but should
be applied directly to the generated packages for the sake of downstream
users.

The `clippy::derive_partial_eq_without_eq` lint was already being
disabled for the packages vendored by rclrs, but is now moved to the
rosidl_generator_rs template instead. This is necessary since we always
derive the PartialEq trait, but can't necessary derive Eq, and so don't.

The `clippy::upper_case_acronyms` is new and was added to account for
message type names being upper-case acrynyms, like
unique_identifier_msgs::msg::UUID.

* Update vendored message packages

This updates the message packages vendored under the rclrs `vendor`
module. The vendor_interfaces.py script was used for reproducibility.

The existing packages – rcl_interfaces, rosgraph_msgs, and
builtin_interfaces – are only modified in that they now use the
std::ffi::c_void naming for c_void instead of the std::os::raw::c_void
alias and disable certain clippy lints.

The action_msgs and unique_identifier_msgs packages are newly added to
enable adding action support to rclrs. action_msgs is needed for
interaction with action goal info and statuses, and has a dependency on
unique_identifier_msgs.
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Copy link
Author

mxgrey commented Nov 18, 2024

Based on some discussions in the working group, I've updated interval to be called throttle instead, aligning the terminology with the other ROS client libraries.

@mxgrey
Copy link
Author

mxgrey commented Nov 18, 2024

I've also merged in the latest main branch from upstream. I think this PR should be good to go. Hopefully this will be the last one needed for the maintainer review.

@geoff-imdex geoff-imdex merged commit 196569c into geoff-imdex:main Nov 18, 2024
6 checks passed
@mxgrey mxgrey deleted the clean_logging_tests branch November 18, 2024 06:55
@mxgrey mxgrey mentioned this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants