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

Adding tests for unicode support in message comments. #720

Merged
merged 3 commits into from
Jan 17, 2023
Merged

Adding tests for unicode support in message comments. #720

merged 3 commits into from
Jan 17, 2023

Conversation

guichristmann
Copy link
Contributor

This PR is in reference to issue #713.

Adds a new test file test_parse_unicode.py in rosidl_adapter to ensure parsing of messages with Unicode comments works as expected.

The characters and symbols used in the comments were semi-randomly picked from this document: https://www.cogsci.ed.ac.uk/~richard/unicode-sample.html

Signed-off-by: guichristmann <guichristmann@gmail.com>
@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

@guichristmann Can you take a look at the flake8 warnings and fix those? I think that import pytest needs to go after the import os block, and that from rosidl_adapter.parser import parse_message_file needs to be before from rosidl_adapter.parser import parse_message_string. Once those are done, I'll run CI again.

Signed-off-by: guichristmann <guichristmann@gmail.com>
@guichristmann
Copy link
Contributor Author

Oops, seems like I didn't have the flake8 plugin for import order and missed those. I've verified it locally and pushed a new commit with the changes. Hopefully it goes through now.

@clalancette
Copy link
Contributor

Oops, seems like I didn't have the flake8 plugin for import order and missed those. I've verified it locally and pushed a new commit with the changes. Hopefully it goes through now.

Thanks for taking a look. It looks good now; I'll run CI again.

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

Thanks for taking a look. It looks good now; I'll run CI again.

Ah, so now we run into a real bug on Windows (it is always Windows). It looks like it is upset about the \u2200 character in one of the tests. If you click on https://ci.ros2.org/job/ci_windows/18571/consoleFull , you should be able to find the error. Can you please take a look?

Signed-off-by: guichristmann <guichristmann@gmail.com>
@guichristmann
Copy link
Contributor Author

Apparently for Windows it is required to explicitly pass an encoding=utf-8 parameter when writing to a file (per https://stackoverflow.com/questions/27092833/unicodeencodeerror-charmap-codec-cant-encode-characters).

I pushed the small changes that are supposed to fix this, but as I don't have a Windows machine right now I only tested it in Ubuntu. (Is there an alternative way for me to test/run the CI locally?) I'll wait for the new CI run and check again in case new problems pop up.

@clalancette
Copy link
Contributor

I pushed the small changes that are supposed to fix this, but as I don't have a Windows machine right now I only tested it in Ubuntu.

Thanks!

(Is there an alternative way for me to test/run the CI locally?)

Not right now, unfortunately. I'll kick off another round of CI here.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for iterating!

@clalancette clalancette merged commit a46056c into ros2:rolling Jan 17, 2023
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.

Add tests parsing/generating msg/srv/action comments with non-ASCII characters
2 participants