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

Fix test settings for rosbridge_library #643

Merged
merged 5 commits into from
Oct 7, 2021

Conversation

kenji-miyake
Copy link
Contributor

@kenji-miyake kenji-miyake commented Sep 24, 2021

Public Changes

None

Description

To keep the quality of libraries, it's important to re-enable tests as written in a TODO comment.
As a first step, I'd like to fix some errors to minimize the diff of the following PRs.

Also, I'll send some other PRs to fix each test.

Related: #644, #645, #646, #653

@kenji-miyake
Copy link
Contributor Author

@jtbandes @amacneil @cclauss Hi, thank you for maintaining this great project! I saw the commit history and mentioned you.
Could you review this and related PRs, please? 🙏

@kenji-miyake
Copy link
Contributor Author

I know some tests might require launch_testing like #585, but I think it's worth cleaning these up first.

@amacneil
Copy link
Member

Agree with this direction, but could you make a PR (or update this one) that runs black on the whole codebase (and add to CI if possible)? That way we will be consistent everywhere.

It also looks like there might be other changes in this PR (cmake file?), it would be best if we do the formatting changes as one atomic unit separate from any other code changes.

@kenji-miyake
Copy link
Contributor Author

kenji-miyake commented Sep 24, 2021

could you make a PR (or update this one) that runs black on the whole codebase (and add to CI if possible)?

Yes, it's possible. I think pre-commit is a good way to do that as moveit teams do.
https://github.com/ros-planning/moveit2/blob/d184714751b347a2389cf1e8742b1be94eed63b8/.pre-commit-config.yaml#L35-L38

-> Added in #648

It also looks like there might be other changes in this PR (cmake file?), it would be best if we do the formatting changes as one atomic unit separate from any other code changes.

Yes, it's for fixing tests. I can split that into another PR.

This was referenced Sep 24, 2021
Kenji Miyake added 2 commits September 28, 2021 14:55
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
@kenji-miyake kenji-miyake changed the title Format test files Fix test settings for rosbridge_library Sep 28, 2021
@kenji-miyake
Copy link
Contributor Author

@amacneil Thank you for merging #648. I rebased and changed the title of this PR!

@amacneil
Copy link
Member

Thanks! Can you update the PR description too?

Tagging @jtbandes for a review

@kenji-miyake
Copy link
Contributor Author

@amacneil Done! Removed format-related words and add a related PR.

Kenji Miyake added 3 commits September 28, 2021 19:53
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Copy link
Member

@amacneil amacneil left a comment

Choose a reason for hiding this comment

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

thanks!

@amacneil amacneil merged commit b918f4a into RobotWebTools:ros2 Oct 7, 2021
@kenji-miyake kenji-miyake deleted the format-test-files branch October 8, 2021 08:15
jtbandes pushed a commit that referenced this pull request Dec 10, 2021
**Public Changes**
None

**Description**

Fixed test_message_conversion.py.
Since it has some bugs in the application code, I'll fix them in another PR #646.

Related: #643

Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
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.

None yet

2 participants