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: making filter admin data test order independent #2355

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

gabrielmer
Copy link
Contributor

Description

Generalized test to not rely on order of neither peers subscriptions nor of subscription criteria within each peer.

@gabrielmer gabrielmer marked this pull request as ready for review January 15, 2024 15:52
@gabrielmer gabrielmer self-assigned this Jan 15, 2024
Copy link

github-actions bot commented Jan 15, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2355

Built from 06d3741

Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Thanks!

LGTM

tests/wakunode_rest/test_rest_admin.nim Show resolved Hide resolved
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

LGTM with only small remarks.
We may think of how to compare resultset unordered.
Also now this change is in contradict with my PR.. but that can be solved as soon as this one gets merged.

@gabrielmer
Copy link
Contributor Author

We may think of how to compare resultset unordered.

Yes! I changed the case to check for the number of content topics subscribed and not their values to avoid complexity, as I think the check is strong enough in this case.

The problem here why I didn't use a set is that there's two layers of order: there's the list of peers, and within each peer there's a list of content topics. So we would have to use a set of sets, or something like that, and might overcomplicate things.

There might be a more elegant solution, we can think it through :). Sorting is also a option but kind of tedious in this case, for the same reason.

Also now this change is in contradict with my PR.. but that can be solved as soon as this one gets merged.

Yes! Once this is merged we'll probably have to rebase your PR to include this fix :))

@gabrielmer gabrielmer force-pushed the fix-imporving-filter-admin-test branch from 82e75fd to 11a5ce4 Compare January 16, 2024 17:30
@gabrielmer gabrielmer merged commit 8a9fad2 into master Jan 17, 2024
9 of 10 checks passed
@gabrielmer gabrielmer deleted the fix-imporving-filter-admin-test branch January 17, 2024 08:42
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.

3 participants