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

AlejandroCabeza · 2023-09-14T11:02:17Z

Description

Implement second batch of subscribe tests for waku filter.

Changes

Implement subscribe tests.

Dependencies

#2034

github-actions · 2023-09-14T11:07:55Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2035

Built from 7d23fff

NagyZoltanPeter · 2023-09-26T14:24:18Z

tests/waku_filter_v2/test_waku_client.nim

-      # Given
-      let 
-        subscribeResponse = await wakuFilterClient.subscribe(
+        assert subscribeResponse.isOk(), $subscribeResponse.error


Isn't it be unsubscribeResponse here? Also maybe instead of assert formulize a check?

Indeed it is. Fixed, along the other typos you mentioned.

The assert is a suggestion from @Ivansete-status. What this does is, if the check fails it prints the value on the right, namely $subscribeResponse.error, which is useful for debugging.

NagyZoltanPeter · 2023-09-26T14:31:57Z

tests/waku_filter_v2/test_waku_client.nim

+          unsubscribeResponse.isOk()
+          wakuFilter.subscriptions.len == 0
+
+        # When sending a message to the previously subscribed content topic


Just a little typo here.

Suggested change

# When sending a message to the previously subscribed content topic

# When sending a message to the previously unsubscribed content topic

NagyZoltanPeter · 2023-09-26T14:59:43Z

tests/waku_filter_v2/test_waku_client.nim

+          unsubscribeResponse.isOk()
+          wakuFilter.subscriptions.len == 0
+
+        # When sending a message to the previously subscribed content topic


Suggested change

# When sending a message to the previously subscribed content topic

# When sending a message to the previously unsubscribed content topic

NagyZoltanPeter · 2023-09-26T15:00:05Z

tests/waku_filter_v2/test_waku_client.nim

+        check:
+          not await pushHandlerFuture.withTimeout(FUTURE_TIMEOUT)
+
+        # When sending a message to the other previously subscribed content topic


Suggested change

# When sending a message to the other previously subscribed content topic

# When sending a message to the other previously unsubscribed content topic

NagyZoltanPeter · 2023-09-26T15:03:12Z

tests/waku_filter_v2/test_waku_client.nim

+          wakuFilter.subscriptions.hasKey(clientPeerId)
+          wakuFilter.getSubscribedContentTopics(clientPeerId) == otherContentTopicSeq
+
+        # When sending a message to the previously subscribed content topic


Suggested change

# When sending a message to the previously subscribed content topic

# When sending a message to the previously unsubscribed content topic

NagyZoltanPeter · 2023-09-26T15:03:40Z

tests/waku_filter_v2/test_waku_client.nim

+          unsubscribeResponse2.isOk()
+          wakuFilter.subscriptions.len == 0
+
+        # When sending a message to the previously subscribed content topic


Suggested change

# When sending a message to the previously subscribed content topic

# When sending a message to the previously unsubscribed content topic

NagyZoltanPeter

I think it is all good! Thank you for the great work!
I found only some typos and maybe one missed check.

NagyZoltanPeter

LGTM!
Its a good idea having that assert like this. I did not know.

Ivansete-status

LGTM! Thanks for it! 💯

Ivansete-status · 2023-09-28T13:43:06Z

tests/waku_filter_v2/test_waku_client.nim

+      pushHandlerFuture = newPushHandlerFuture()
+      messagePushHandler = proc(
+        pubsubTopic: PubsubTopic, message: WakuMessage
+      ): Future[void] {.async, closure, gcsafe.} =


Small nitpick:
I think is not needed to use Future[void]. This is done implicitly by the async pragma.

Good to know, thanks!

Ivansete-status · 2023-09-28T13:46:21Z

tests/waku_filter_v2/test_waku_client.nim

+            serverRemotePeerInfo, pubsubTopic, contentTopicSeq
+          )
+        require:
+          subscribeResponse.isOk()


For that kind of validation, I think it is better to do the next so that we have a better detail in case of failure:
assert subscribeResponse.isOk(), $subscribeResponse.error

That applies elsewhere a Result is being checked.

Agree, must've slipped by when changing.
I'm actually moving away from require, as if a require doesn't work, it makes the whole test suite fail, so I'd rather use checks or asserts.

Ivansete-status · 2023-09-28T13:48:05Z

tests/waku_filter_v2/test_waku_client.nim

-      # Given
-      let 
-        subscribeResponse = await wakuFilterClient.subscribe(
+  suite "MessagePushHandler - Void":


For me to understand better. What does "Void" mean in this context?

Just a (poor) naming I used to differentiate the two different push handlers I used: One containing future (as a way to confirm a message arrived), and the other also adding the messages to a list.

I'm planning to remove this classification because the cost of making it understandable is not worth the time improvement it provides. Better to have a single push handler that does both things (have a future, and add to list), even if the list is not used in some tests.

* Implement waku filter client subscribe tests. * Remove legacy filter tests. * Fix constant for max criteria per subscription limit.

AlejandroCabeza self-assigned this Sep 14, 2023

AlejandroCabeza changed the base branch from master to test-waku-filter-subscribe September 14, 2023 11:02

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch from 69f7d66 to 070a72a Compare September 15, 2023 17:36

AlejandroCabeza mentioned this pull request Sep 18, 2023

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

Merged

2 tasks

AlejandroCabeza changed the title ~~test(waku-filter): Subscribe tests batch 2~~ test(waku-filter): Subscribe tests batch (2/3) Sep 19, 2023

AlejandroCabeza changed the title ~~test(waku-filter): Subscribe tests batch (2/3)~~ test(waku-filter): Subscribe tests batch (3/3) Sep 19, 2023

AlejandroCabeza changed the title ~~test(waku-filter): Subscribe tests batch (3/3)~~ test(waku-filter): Subscribe tests batch (2/3) Sep 19, 2023

AlejandroCabeza force-pushed the test-waku-filter-subscribe branch from b54e77a to e7bca65 Compare September 20, 2023 14:37

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch 2 times, most recently from 44fef54 to 5063ab4 Compare September 20, 2023 17:34

AlejandroCabeza changed the title ~~test(waku-filter): Subscribe tests batch (2/3)~~ test(waku-filter): Subscribe tests batch (2/4) Sep 20, 2023

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch from 5063ab4 to f4b924f Compare September 21, 2023 19:25

AlejandroCabeza force-pushed the test-waku-filter-subscribe branch from 4f0326a to 6605abf Compare September 21, 2023 21:03

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch from f4b924f to 29d4837 Compare September 22, 2023 08:11

AlejandroCabeza marked this pull request as ready for review September 25, 2023 13:01

AlejandroCabeza requested a review from NagyZoltanPeter September 25, 2023 13:02

NagyZoltanPeter reviewed Sep 26, 2023

View reviewed changes

NagyZoltanPeter requested changes Sep 26, 2023

View reviewed changes

NagyZoltanPeter approved these changes Sep 26, 2023

View reviewed changes

AlejandroCabeza force-pushed the test-waku-filter-subscribe branch from 6605abf to 798c7f6 Compare September 27, 2023 16:43

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch from 30c78cf to ebf4a50 Compare September 27, 2023 16:44

Ivansete-status approved these changes Sep 28, 2023

View reviewed changes

AlejandroCabeza force-pushed the test-waku-filter-subscribe branch from 798c7f6 to dc47a6f Compare October 9, 2023 14:39

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch from ebf4a50 to 225da69 Compare October 9, 2023 15:05

AlejandroCabeza force-pushed the test-waku-filter-subscribe branch from dc47a6f to 76be3d1 Compare October 9, 2023 16:10

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch from 225da69 to a6ca5be Compare October 9, 2023 16:11

AlejandroCabeza added 4 commits October 12, 2023 20:50

Improve test check readability.

4cd95a9

Implement batch of waku filter client subscribe tests.

19a23a3

Fix typos.

4c1f4d2

Minor PR comment improvements.

f4ea8e3

AlejandroCabeza force-pushed the test-waku-filter-subscribe branch from 76be3d1 to c0f218b Compare October 12, 2023 18:50

AlejandroCabeza force-pushed the test-waku-filter-subscribe-2 branch from a6ca5be to f4ea8e3 Compare October 12, 2023 18:51

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

bf83a72

* Implement waku filter client subscribe tests. * Remove legacy filter tests. * Fix constant for max criteria per subscription limit.

AlejandroCabeza merged commit e1f470d into test-waku-filter-subscribe Oct 12, 2023
2 checks passed

AlejandroCabeza deleted the test-waku-filter-subscribe-2 branch October 12, 2023 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AlejandroCabeza commented Sep 14, 2023 •

edited

github-actions bot commented Sep 14, 2023 •

edited

NagyZoltanPeter Sep 26, 2023 •

edited

AlejandroCabeza Sep 26, 2023

NagyZoltanPeter Sep 26, 2023

NagyZoltanPeter Sep 26, 2023

NagyZoltanPeter Sep 26, 2023

NagyZoltanPeter Sep 26, 2023

NagyZoltanPeter Sep 26, 2023

NagyZoltanPeter left a comment

NagyZoltanPeter left a comment

Ivansete-status left a comment

Ivansete-status Sep 28, 2023

AlejandroCabeza Oct 9, 2023

Ivansete-status Sep 28, 2023

AlejandroCabeza Oct 9, 2023

Ivansete-status Sep 28, 2023

AlejandroCabeza Oct 9, 2023 •

edited

	# When sending a message to the previously subscribed content topic
	# When sending a message to the previously unsubscribed content topic

	# When sending a message to the other previously subscribed content topic
	# When sending a message to the other previously unsubscribed content topic

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

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

Conversation

AlejandroCabeza commented Sep 14, 2023 • edited

Description

Changes

Dependencies

github-actions bot commented Sep 14, 2023 • edited

NagyZoltanPeter Sep 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlejandroCabeza Oct 9, 2023 • edited

Choose a reason for hiding this comment

AlejandroCabeza commented Sep 14, 2023 •

edited

github-actions bot commented Sep 14, 2023 •

edited

NagyZoltanPeter Sep 26, 2023 •

edited

AlejandroCabeza Oct 9, 2023 •

edited