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: CLI integration tests do not use correct credentials and broker port #582

Conversation

ludovic-boutros
Copy link
Contributor

…port

  • Please check if the PR fulfills these requirements
  • The commit messages are descriptive
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • An issue has been created for the pull requests. Some issues might require previous discussion.
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    This PR fixes the CLI integration tests, broken since the quotas fix Fix quota manager integration #581.

  • What is the current behavior? (You can also link to an open issue here)
    Timeout during integration tests.

  • What is the new behavior (if this is a feature change)?
    Tests should pass now.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

  • Other information:

IMPORTANT: Please review the CONTRIBUTING.md file for detailed contributing guidelines.
IMPORTANT: Your pull request MUST target master.

PLEASE REMOVE THIS TEMPLATE BEFORE SUBMITTING

@sverrehu
Copy link
Contributor

sverrehu commented Feb 21, 2024

This still breaks the unit tests. There is no broker at localhost:909x when unit tests are run. Maybe you could disable your functionality during dry-run, if that makes any sense? The failing tests have dry-run enabled.

Other Managers have logic for not querying the cluster in certain cases, and this is used during unit testing. See e.g. ArtefactManager and TopicManager, which both have loadActualClusterStateIfAvailable methods. On the other hand, QuotasManager will always query the cluster.

@ludovic-boutros ludovic-boutros force-pushed the fix/cli-integration-tests-broken-since-quotas-fix branch from 1b3cb19 to a803ade Compare April 3, 2024 13:28
@ludovic-boutros
Copy link
Contributor Author

I needed to add the quota manager mocks in the cli unit tests.
I hope it should be good now.

@purbon purbon merged commit 40d70a6 into kafka-ops:master Apr 3, 2024
2 checks passed
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.

3 participants