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

Rabbit sync new #120

Open
wants to merge 18 commits into
base: rabbit-sync
Choose a base branch
from
Open

Conversation

tatzati
Copy link
Contributor

@tatzati tatzati commented Aug 1, 2023

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Documentation (typos, code examples or any documentation update)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project (scripts/lint.sh has no errors)
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I added a code examples to illustrate the changes

@Lancetnik Lancetnik changed the base branch from main to rabbit-sync August 1, 2023 17:44
def test_init_connect_by_url(self, settings):
args, kwargs = self.get_broker_args(settings)
broker = self.broker(*args, **kwargs)
assert broker.connect()
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please refactor all the following testcases to use a mocked _connect method instead? We can check incoming *args, **kwargs arguments by assert_called_with method, so we need no more connect to the real broker to test correct arguments merging.

Copy link
Contributor Author

@tatzati tatzati Aug 2, 2023

Choose a reason for hiding this comment

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

Is this ok? Or do you have something else in mind?

 @patch("pika.BlockingConnection")
   def test_init_connect(self, mock, settings: Settings):
    mock_conn = MagicMock()
    mock.return_value = mock_conn
    args, kwargs = self.get_broker_args(settings)
    broker = self.broker(*args, **kwargs)
    broker.connect()
    mock.assert_called_once_with(
        pika.URLParameters(settings.url),
    )
    broker.close()
    mock_conn.close.assert_called_once()

Copy link
Owner

Choose a reason for hiding this comment

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

I thought about replacing the original broker _connect method to check incoming arguments. It will be suitable for all brokers, not RMQ only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give me an example?

Copy link
Owner

Choose a reason for hiding this comment

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

Tomorrow

Copy link
Owner

Choose a reason for hiding this comment

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

By words: you need to patch this one https://github.com/Lancetnik/Propan/blob/main/propan/brokers/rabbit/rabbit_broker.py#L108 the way you patching in the TestBroker
'broker._connect = MethodType(async_mock, broker)' and check the mock call arguments

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