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

map tests queues and refactor #1559

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

smagnani96
Copy link
Contributor

This PR introduces a new test for MapIterator as well as small refactoring to some other tests.
With this, the aim is primarily to ensure that MapIterator fails when called on keyless maps such as queues/stacks: this is because the current implementation relies on a key (nextKey) lookup from the map, which is then used to retrieve its respective value.
Commit messages should contain each the explanation, feel free to reach me for further q&a 😃

@smagnani96 smagnani96 requested a review from a team as a code owner September 3, 2024 17:52
@smagnani96 smagnani96 changed the title Pr/map tests queues and refactor map tests queues and refactor Sep 3, 2024
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Changes all look good to me

Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks for splitting this up into smaller bits. Please try not to move stuff (like the variable declarations) around without a clear benefit. It causes churn and makes things more time-consuming to review.

map_test.go Show resolved Hide resolved
map_test.go Show resolved Hide resolved
map_test.go Outdated Show resolved Hide resolved
map_test.go Show resolved Hide resolved
This commit introduces in the test sections for the maps the peek operation
performed on a queue. It verifies that the 1st value is correctly returned
and not removed from the data structure (lookup operation), and also that
it returns the error `ErrKeyNotExist` on an empty queue.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit performs a small refactoring to some of the map tests,
trying to shorten a bit, while also cleaning unnecessary code.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
This commit introduces the `TestIterateWrongMap` test, which should
assert that the `MapIterator` fails while called on keyless maps such as
queues. We do not expect different behaviours, as the `MapIterator.Next`
relies on the presence of a key for which it lookups the value.
A different method can be later introduces to also traverse a queue, but
in that case we'd need to additionally remove the value.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/map-tests-queues-and-refactor branch from 2613a2c to fbc3585 Compare September 12, 2024 10:48
@smagnani96 smagnani96 requested a review from ti-mo September 12, 2024 11:09
@ti-mo ti-mo merged commit 88736f4 into cilium:main Sep 12, 2024
17 checks passed
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.

4 participants