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

improve handling of encoding #181

Merged
merged 1 commit into from
Jul 31, 2019
Merged

Conversation

dirk-thomas
Copy link
Contributor

Related to ros2/rosidl#388.

@dirk-thomas dirk-thomas added bug Something isn't working in review Waiting for review (Kanban column) labels Jul 19, 2019
@dirk-thomas dirk-thomas self-assigned this Jul 19, 2019
@dirk-thomas
Copy link
Contributor Author

With this change the failing test (outputting non-ASCII characters in the error message) is reported correctly: https://ci.ros2.org/job/ci_windows/7467/testReport/

The actual test is being fixed in a second commit in ros2/rosidl#391.

print(decoded_line, end='')
output += decoded_line
if output_handle:
output_handle.write(line)
output_handle.write(decoded_line.encode())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change necessary, isn't line already encoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using line directly it is not ensured that the characters in that string can be encoded into the encoding which is used for stdout.

By first successfully decoding it (with one of the two possible encodings) and then re-encoding it is ensure that the string can be written to stdout without an exception.

Copy link
Contributor

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Other than one question, LGTM

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas force-pushed the dirk-thomas/bom-generated-code branch from 2af1579 to 84f395b Compare July 25, 2019 19:17
@dirk-thomas dirk-thomas merged commit be483fd into master Jul 31, 2019
@delete-merged-branch delete-merged-branch bot deleted the dirk-thomas/bom-generated-code branch July 31, 2019 14:39
nuclearsandwich pushed a commit that referenced this pull request Mar 20, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
nuclearsandwich pushed a commit that referenced this pull request Mar 23, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
nuclearsandwich added a commit that referenced this pull request Apr 11, 2020
* Add runner option to ament_add_test (#174)

* ament_cmake allow speficiation of a different test runner

  - By default, still uses run_test.py
  - Example use case: ament_cmake_ros can use a test runner that
    sets a ROS_DOMAIN_ID

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

* ament_cmake move run_test.py to a python module

  - This should let us see the history

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

* ament_cmake refactor run_test.py into an importable python module

  - Adds an ament_cmake_test python package

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* improve handling of encoding (#181)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* use deterministic order for updated env vars (#196)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Run tests in current binary directory, not global source directory (#206)

Switch to CMAKE_CURRENT_BINARY_DIR for consistency with CTest

Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* remove status attribute from result XML, add skipped tag instead (#218)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Declare AMENT_TEST_RESULTS_DIR as a PATH (#221)

Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Generate xunit files valid for the junit10.xsd

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Revert "Run tests in current binary directory, not global source directory (#206)"

This reverts commit 4354d62.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

Co-authored-by: Peter Baughman <dblue135@yahoo.com>
Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Co-authored-by: Dan Rose <rotu@users.noreply.github.com>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants