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

Issue #127 - Consistent Hash binding key must be integer #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RedMu
Copy link
Contributor

@RedMu RedMu commented Jul 1, 2020

See #127 - Now throws an illegal argument exception on trying to bind rather than using the Hashcode of a non integer.



@Test
void binding_with_non_integer_key_throws_exception() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test can be more expressive with a parameterized test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I miss read the message on my phone so I've also parameterised the distribution test to use a 25 tests with a random number of receivers and a random number of messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am sorry for the extra work, but to be honest I found the distribution test way less readable.

I am okay with parameterizing it, but I think a few cases with hard coded values will be easier to reason about.

I am also okay with reverting it to the first version 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I've reverted the parameterised version of the dispatch_respects_queue_weight and kept the parameterised version of the exception test.

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #128 into master will decrease coverage by 0.36%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #128      +/-   ##
============================================
- Coverage     94.20%   93.84%   -0.37%     
+ Complexity      455      452       -3     
============================================
  Files            38       38              
  Lines          1070     1072       +2     
  Branches         49       49              
============================================
- Hits           1008     1006       -2     
- Misses           45       48       +3     
- Partials         17       18       +1     
Impacted Files Coverage Δ Complexity Δ
...rabbitmq/mock/exchange/ConsistentHashExchange.java 86.36% <100.00%> (-13.64%) 6.00 <0.00> (-2.00)
.../github/fridujo/rabbitmq/mock/ReceiverPointer.java 66.66% <0.00%> (-8.34%) 3.00% <0.00%> (-1.00%)
...va/com/github/fridujo/rabbitmq/mock/MockQueue.java 95.19% <0.00%> (ø) 81.00% <0.00%> (ø%)
...dujo/rabbitmq/mock/exchange/MockTopicExchange.java 100.00% <0.00%> (ø) 2.00% <0.00%> (ø%)
.../com/github/fridujo/rabbitmq/mock/MockChannel.java 90.75% <0.00%> (+0.03%) 123.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8930b81...4592c22. Read the comment docs.

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