-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Use LinkedHashMap for deterministic iterations #1165
Conversation
3454156
to
04338e7
Compare
I guess to change implementation for tests is not a good approach. |
@kortov Thanks for the feedback. I guess another approach is to assert for both orders. It gets messy when there are more but in this test, there are only two possible cases. |
Maybe you're right with your implementation because in
LinkedHashMap . And more than that, your PR is to feign.mock package, so I guess performance is not bigger issue than determenistic results.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good, will merge as soon build passes
04338e7
to
f67316d
Compare
@velo I fixed the format issue. |
Test
shouldPrintHeaders()
asserts the string version ofRequestHeader
.RequestHeader
class uses a HashMap to store the headers. However, HashMap does not guarantee any specific order of entries. Test can fail if the iteration order changes.This PR proposes to fix the test by changing HashMap to LinkedHashMap that makes the iteration order deterministic.