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

fix(test): disable encoded response/request check when map contains multiple elements #2013

Merged

Conversation

faillefer
Copy link
Contributor

@faillefer faillefer commented Sep 9, 2021

Fix #2012
Disable encoded response/request check when map contains multiple elements. In that situation the encoded/request response varies because of unpredictable map traversal order

@faillefer faillefer force-pushed the fix-2012-reproducible-delete-offset-response branch from f05103e to 6d470cc Compare September 9, 2021 12:19
@faillefer faillefer marked this pull request as ready for review September 9, 2021 12:26
@faillefer faillefer requested a review from bai as a code owner September 9, 2021 12:26
@faillefer faillefer changed the title [#2012] Sort partitions when encoding DeleteOffsetResponse [#2012] Sort by partition when encoding DeleteOffsetResponse Sep 9, 2021
@bai bai requested a review from dnwe September 9, 2021 12:29
@dnwe
Copy link
Collaborator

dnwe commented Sep 9, 2021

Thanks for looking at this. The change here does fix the test, although if we do want the result to always be reproducible you'd want to sort the parent topic names as well as that is also a map isn't it?

However, it does also raise the question of: "do we want to be sorting/ordering the protocol responses to make testing easier, when the clients themselves don't care?" — I wonder if we should change our comparison method in our tests

@faillefer
Copy link
Contributor Author

faillefer commented Sep 9, 2021

Since iteration order on map is randomized the encoded response is variable and cannot be checked against a fixed byte array.
Should we disable this check as in offset_commit_response_test.go ?

@faillefer
Copy link
Contributor Author

If we do as in offset_commit_response_test.go the result would be :

faillefer@d3b16f1

@dnwe
Copy link
Collaborator

dnwe commented Sep 10, 2021

@faillefer yeah I think that’s the best option for now

…ments. In that situation the encoded/request response varies because of unpredictable map traversal order
@faillefer faillefer force-pushed the fix-2012-reproducible-delete-offset-response branch from 6d470cc to da3178f Compare September 10, 2021 12:57
@faillefer faillefer changed the title [#2012] Sort by partition when encoding DeleteOffsetResponse [#2012] Disable encoded response/request check when map contains multiple elements Sep 10, 2021
@faillefer
Copy link
Contributor Author

faillefer commented Sep 10, 2021

@dnwe i've updated the branch associated to this merge request.
Check of encoded response (or request) is not performed when it contains map with multiple elements
(The sort on partition is removed)
I've updated the title and description of the MR

@dnwe dnwe changed the title [#2012] Disable encoded response/request check when map contains multiple elements fix(test): disable encoded response/request check when map contains multiple elements Sep 13, 2021
@dnwe dnwe merged commit 5f7f224 into IBM:master Sep 13, 2021
dnwe added a commit that referenced this pull request Sep 13, 2021
…offset-response

fix(test): disable encoded response/request check when map contains multiple elements
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.

test: TestDeleteOffsetsResponse is brittle and fails periodically
2 participants