Skip to content

Split controller switch test #2177

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ashwinsnambiar
Copy link

@ashwinsnambiar ashwinsnambiar commented Apr 11, 2025

Closes #521

I have split the existing test into two: lifecycle test for single controller and a switch_controller test with unknown or fake controller.

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.

@ashwinsnambiar
Copy link
Author

Could you please verify if what I have done is what was expected in the issue description? Should I do it for the rest of the tests like async_controller_lifecycle, async_controller_lifecycle_at_cm_rate etc.?

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.

Heyy @ashwinsnambiar

Can you explain the changes a bit?

All I see is that you have moved a part of the test into a different test case. Why?

The idea is to check which part of the BEST_EFFORT strictness logic is covered and add tests to cover the other parts

@ashwinsnambiar
Copy link
Author

ashwinsnambiar commented Apr 12, 2025

Hey @saikishor,

From what I understood, the BEST_EFFORT strictness logic has already been implemented.

I was just following the comment in the issue: #521 (comment).

I started out this draft PR which satisfies the first requirement of the splitting the test into lifecycle test and switching multiple controllers.

I haven't done the second part, adding a new test for activating multiple controllers when resource collision occurs.

Please let me know if I misunderstood the intent somewhere.

@ashwinsnambiar
Copy link
Author

@saikishor, did you understand what I was trying to do?

@saikishor
Copy link
Member

@saikishor, did you understand what I was trying to do?

Sorry I didn't get back to you on this. Yes, you are right. You are on the right direction :)

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 BEST EFFORT controller switch tests for Controller Manager
2 participants