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

Idsse 1054 #91

Merged
merged 5 commits into from
Jan 2, 2025
Merged

Idsse 1054 #91

merged 5 commits into from
Jan 2, 2025

Conversation

paulhamer-noaa
Copy link
Contributor

Linear Issue

IDSSE-1054

Changes

  • Some updates to code for linter issues
  • Increase coverage for rabbitmq_utils (only to 88% still require additional testing)

Explanation

Created pull request to check-point work so far. Consider splitting rabbitmq_utils, i.e. Rpc, Consumer, Publisher classes as separate modules to reduce module sizes.

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.

Do I understand correctly, with these changes we are close to 90% test coverage but still under? If so, I want to make sure we create an additional task to come back and increase the coverage.
A number of these tests directly call the private _publish function. If you are able to make changes to call one of the public publish functions (that will internally call the private function) that would be better. If not, we need to make another task to revisit these test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather we not call 'private' functions directly in tests. This practice often makes it more work if/when we need to refactor our code. It is better to only call 'public' functions, though I know it can sometimes be difficult to work out the correct args to the public functions that test the underlying core logic.

@paulhamer-noaa
Copy link
Contributor Author

paulhamer-noaa commented Jan 2, 2025 via email

@paulhamer-noaa paulhamer-noaa merged commit 7fa7141 into main Jan 2, 2025
2 checks passed
@paulhamer-noaa paulhamer-noaa deleted the IDSSE-1054 branch January 2, 2025 16:04
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