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

Filter Subscribe #6

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Filter Subscribe #6

merged 3 commits into from
Dec 11, 2023

Conversation

fbarbu15
Copy link
Collaborator

@fbarbu15 fbarbu15 commented Nov 29, 2023

  • Added filter subscription creation and update tests
  • reached 67 tests
  • Small adjustments to the framework
  • Rename other subscription tests with relay so it's clear what each test does
  • Issues reported:
  1. gowaku:
    encoding/hex: odd length hex string error when subscribing
  2. nwaku:
    pubsubTopic not required as described in the specs

@fbarbu15 fbarbu15 marked this pull request as ready for review November 29, 2023 13:48
Copy link
Collaborator

@AlbertoSoutullo AlbertoSoutullo left a comment

Choose a reason for hiding this comment

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

LGTM.

Also, before I forget, regarding relay (and community messages, that I don't think you are doing right now), looks like they are facing message loss issues. This was commented in a call yesterday, and also in a comment today.

Maybe it gives you more ideas about test cases, maybe not. I simply wanted you to be aware of it, just in case..

set_subscriptions_response = self.rpc_call(
"post_waku_v2_filter_v1_subscription",
[
subscription["contentFilters"] if "contentFilters" in subscription else [],

Choose a reason for hiding this comment

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

These can be shorthanded to

subscription.get("contentFilters", [])
# and
subscription.get("pubsubTopic")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, it's more easy to read, thanks


@pytest.fixture(scope="function")
def relay_warm_up(self):
try:
self.wait_for_published_message_to_reach_peer()
self.wait_for_published_message_to_reach_relay_peer()
logger.info("WARM UP successful!!")
except Exception as ex:
raise TimeoutError(f"WARM UP FAILED WITH: {ex}")

# this method should be used only for the tests that use the warm_up fixture

Choose a reason for hiding this comment

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

You mean the relay_warm_up fixture?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, fixing :)


def test_filter_update_subscription_with_invalid_pubsub_topic_format(self, subscribe_main_nodes):
try:
self.update_filter_subscription({"requestId": "1", "contentFilters": [self.test_content_topic], "pubsubTopic": [self.test_pubsub_topic]})

Choose a reason for hiding this comment

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

With "invalid pubsub topic form" we're talking about "type" or "string formatting"?
I mean: "list of strings" (which is what seems to happen) vs the "specified rfc string format"?

I don't know if I'm making sense 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, in this test I'm passing a list of strings instead of string. I will update the test name to be more suggestive

Copy link
Collaborator

@romanzac romanzac left a comment

Choose a reason for hiding this comment

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

LGTM.
waku-org/go-waku#928 shows how important interop tests are. I won't get to these kind of errors just by using Go functions from codebase.

@fbarbu15 fbarbu15 merged commit 7f91bd7 into master Dec 11, 2023
1 of 2 checks passed
@fbarbu15 fbarbu15 deleted the tests/filter-subscribe branch December 11, 2023 12:02
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.

None yet

4 participants