Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Fix child_frame in controller_state_msg #1601

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix child_frame in controller_state_msg #1601

wants to merge 5 commits into from
+43 −1

Conversation

rehanshah17
Copy link

@rehanshah17 rehanshah17 commented Mar 20, 2025 •
edited by christophfroehlich
Loading

Closing #504

Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:

  1. Limited scope. Your PR should do one thing or one set of things. Avoid adding “random fixes” to PRs. Put those on separate PRs.
  2. Give your PR a descriptive title. Add a short summary, if required.
  3. Make sure the pipeline is green.
  4. Don’t be afraid to request reviews from maintainers.
  5. New code = new tests. If you are adding new functionality, always make sure to add some tests exercising the code and serving as live documentation of your original intention.

To send us a pull request, please:

  • Fork the repository.
  • Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
  • Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
  • Commit to your fork using clear commit messages.
  • Send a pull request, answering any default questions in the pull request interface.
  • Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

@christophfroehlich christophfroehlich changed the title fixed the copy and paste #504 issue, added the test check_frame_ids_in_controller_state Fix child_frame in controller_state_msg Mar 20, 2025
@christophfroehlich christophfroehlich linked an issue Mar 20, 2025 that may be closed by this pull request
[AdmittanceController] It looks like an unintentional copy-paste has happened #504
Open
9 tasks
christophfroehlich
christophfroehlich requested changes Mar 22, 2025
View reviewed changes
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

please have a look at the failing CI jobs

@rehanshah17
Copy link
Author

Pretty sure I just fixed the child_frame issue + the failing Cl jobs in the newest commit "fixing spaces".

@rehanshah17
Copy link
Author

This may be something very small that I am overlooking, but all the tests are failing from the first version of my code, instead of the newest commit. Is there something else that I have to do than pushing changes to the feature branch?

1 similar comment
@rehanshah17
Copy link
Author

This may be something very small that I am overlooking, but all the tests are failing from the first version of my code, instead of the newest commit. Is there something else that I have to do than pushing changes to the feature branch?

saikishor
saikishor requested changes Mar 23, 2025
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

This PR doesn't even build. Have you tested it locally before?

saikishor
saikishor reviewed Mar 23, 2025
View reviewed changes
admittance_controller/test/test_admittance_controller.cpp
@@ -350,6 +370,28 @@ TEST_F(AdmittanceControllerTest, receive_message_and_publish_updated_status)
subscribe_and_get_messages(msg);
}

TEST_F(AdmittanceControllerTest, check_frame_ids_in_controller_state)
Copy link
Member

@saikishor saikishor Mar 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@rehanshah17 may be this should be a friend test

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I am not familiar with what a fried test is

Copy link
Member

Choose a reason for hiding this comment

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

Sorry FRIEND_TEST

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@saikishor saikishor saikishor requested changes

@christophfroehlich christophfroehlich christophfroehlich requested changes

@anfemosa anfemosa Awaiting requested review from anfemosa

@ARK3r ARK3r Awaiting requested review from ARK3r

@bmagyar bmagyar Awaiting requested review from bmagyar

@homalozoa homalozoa Awaiting requested review from homalozoa

@progtologist progtologist Awaiting requested review from progtologist

@rosterloh rosterloh Awaiting requested review from rosterloh

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AdmittanceController] It looks like an unintentional copy-paste has happened
3 participants
@rehanshah17 @christophfroehlich @saikishor