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

Feat: idsse-368: publish confirm tests #26

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

mackenzie-grimes-noaa
Copy link
Contributor

Linear Issue

IDSSE-368

Changes

Unit tests for PublishConfirm

Quite complicated to read unfortunately, since I ended up building a functioning mock of every pika object or method that PublishConfirm uses

  • No actual RabbitMQ server exists, messages just publish to nowhere
  • For example, if PublishConfirm passes some on_connect callback arg to some pika method, my mock method really does accept this as an argument, and invokes the callback right away.

More type hinting for PublishConfirm

Based on pika docs. For example, what args our callback methods expect to be sent by pika

Error handling for publish_message

Raise exception if someone attempts to call publish_message() before any channel has been opened.

  • Previously just returned "False" to caller, with no additional detail.
  • Now it can be distinguished from case where everything is connected and open, but pika failed for some other reason

@mackenzie-grimes-noaa mackenzie-grimes-noaa marked this pull request as ready for review October 19, 2023 20:10
Copy link
Contributor

@Geary-Layne Geary-Layne left a comment

Choose a reason for hiding this comment

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

That'a an impressive test module, thanks for taking it on.

@mackenzie-grimes-noaa
Copy link
Contributor Author

I'll probably want Paul to skim this over (when he is next available for IDSSe work), so he can double-check the minor changes I made to PublishConfirm (mainly _enable_delivery_confirmations()). I could not get some unit tests to pass without changing logic in the module, and it looked like it was an actual bug, but I could be wrong.

But in the meantime, I think these tests are better than nothing.

@mackenzie-grimes-noaa mackenzie-grimes-noaa merged commit 08bffde into main Oct 19, 2023
3 checks passed
@mackenzie-grimes-noaa mackenzie-grimes-noaa deleted the feat/idsse-368/publish-confirm-tests branch October 19, 2023 23:33
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.

2 participants