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.

Sign up for GitHub

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

Jump to bottom

test(waku-filter): Subscribe tests batch (4/4) #2057

Merged
merged 6 commits into from
Oct 12, 2023
Merged

test(waku-filter): Subscribe tests batch (4/4) #2057

merged 6 commits into from
Oct 12, 2023

Conversation

AlejandroCabeza
Copy link
Contributor

Description

Implement fourth (and last) batch of subscribe tests for waku filter, the service to service ones.

Changes

  • Implement waku filter service to service tests.
  • Remove legacy waku filter tests.

Dependencies

#2046

@AlejandroCabeza AlejandroCabeza changed the base branch from master to test-waku-filter-subscribe-3 September 20, 2023 21:01
@github-actions
Copy link

github-actions bot commented Sep 20, 2023 •
edited

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2057

Built from beafee4

@AlejandroCabeza AlejandroCabeza force-pushed the test-waku-filter-subscribe-4 branch 2 times, most recently from ac02255 to b354235 Compare September 22, 2023 15:10
@AlejandroCabeza AlejandroCabeza self-assigned this Sep 25, 2023
gabrielmer
gabrielmer reviewed Sep 26, 2023
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Commented some questions, mostly regarding style and not the logic itself.

For the rest I still have some hw to do, need to first get more familiar with the filter protocol. Working on it and hopefully I'll be able to add more feedback on it soon :)

I think it's best to add one or two more reviewers to the PR, ideally including someone that has already worked with filter.

tests/node/test_wakunode_filter.nim Outdated
Comment on lines 3 to 18
import
std/sequtils,
std/options,
stew/shims/net as stewNet,
testutils/unittests,
chronicles,
chronos,
libp2p/crypto/crypto

import
std/[options, tables, sequtils],
testutils/unittests,
chronos,
chronicles,
os,
libp2p/peerstore
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for having two separate import statements and with some repeated modules?
I think it's better to combine them into one import statement without duplicates

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 followed the style I saw in sibling files. I think the reasoning is to separate builtin and third-party imports from local ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but in this case there's 3 import statements instead of 2 and chronos, chronicles and some other modules are being imported twice:

nwaku/tests/node/test_wakunode_filter.nim

Lines 3 to 31 in b354235

import
std/sequtils,
std/options,
stew/shims/net as stewNet,
testutils/unittests,
chronicles,
chronos,
libp2p/crypto/crypto
import
std/[options, tables, sequtils],
testutils/unittests,
chronos,
chronicles,
os,
libp2p/peerstore
import
../../../waku/waku_core,
../../../waku/node/peer_manager,
../../../waku/node/waku_node,
../../../waku/waku_filter_v2,
../../../waku/waku_filter_v2/client,
../../../waku/waku_filter_v2/subscriptions,
../testlib/common,
../testlib/wakucore,
../testlib/wakunode,
../testlib/testasync,
../testlib/futures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that. It seems my brain just read the first half and assummed the second part. You're completely right, changing that.

tests/testlib/futures.nim Outdated
Comment on lines 1 to 8
import
# std/sequtils,
# std/options,
# stew/shims/net as stewNet,
# testutils/unittests,
chronicles,
chronos
# libp2p/crypto/crypto
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would delete the lines with unused modules instead of commenting them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. Oversight when testing. Thanks!

tests/waku_filter_v2/test_waku_client.nim
@@ -18,6 +18,7 @@ import
../testlib/wakucore,
../testlib/testasync,
../testlib/testutils,
../testlib/futures,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a new import required if no code is being added to this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I moved the newPushHandlerFuture in waku_filter_utils (already imported) into the new futures module, as that function will also be used outside waku filter tests.

gabrielmer
gabrielmer approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM! Added one comment with a small nitpick - tests looks great 🔥

Thank you!

tests/node/test_wakunode_filter.nim
var messagePushHandler {.threadvar.}: FilterPushHandler

asyncSetup:
pushHandlerFuture = newFuture[(string, WakuMessage)]()
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a super nit but for consistency I would use here newPushHandlerFuture() like in the unsubscribed test case

@AlejandroCabeza AlejandroCabeza removed the request for review from jm-clius September 27, 2023 16:48
@AlejandroCabeza AlejandroCabeza mentioned this pull request Oct 3, 2023
Merged
2 tasks
Ivansete-status
Ivansete-status approved these changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Great PR! LGTM! Just added a tiny comment

tests/node/test_wakunode_filter.nim
Comment on lines +76 to +78
check:
subscribeResponse.isOk()
server.wakuFilter.subscriptions.len == 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
check:
subscribeResponse.isOk()
server.wakuFilter.subscriptions.len == 1
assert subscribeResponse.isOk(), $subscribeResponse.error
check server.wakuFilter.subscriptions.len == 1

If possible, I'd apply that elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! Must've slipped my mind. I'm planning to do a "standardising" PR after I finish the current one I'm working on to mend all places I skipped.

@AlejandroCabeza AlejandroCabeza merged commit 70e8b7b into test-waku-filter-subscribe-3 Oct 12, 2023
8 checks passed
@AlejandroCabeza AlejandroCabeza deleted the test-waku-filter-subscribe-4 branch October 12, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabrielmer gabrielmer gabrielmer approved these changes

@Ivansete-status Ivansete-status Ivansete-status approved these changes

Assignees

@AlejandroCabeza AlejandroCabeza

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AlejandroCabeza @gabrielmer @Ivansete-status