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

Test/peer connection management #45

Merged
merged 27 commits into from
Jun 21, 2024
Merged

Conversation

romanzac
Copy link
Collaborator

@romanzac romanzac commented Jun 4, 2024

PR Details

Tests to cover peer & connection management basic functionality and related known issues

Issues reported:

@romanzac romanzac marked this pull request as ready for review June 8, 2024 08:03
@romanzac romanzac requested a review from fbarbu15 June 8, 2024 08:03
Copy link
Collaborator

@fbarbu15 fbarbu15 left a comment

Choose a reason for hiding this comment

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

Approved with one small comment
Thanks!

@@ -38,3 +38,18 @@ def delay(num_seconds):

def gen_step_id():
return f"{datetime.now().strftime('%Y-%m-%d_%H-%M-%S')}__{str(uuid.uuid4())}"


def peer_info2id(peer, is_nwaku=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think those methods should be part of the waku_node as they seem to be waku specific.
Here, the common.py file contains helper methods that are unrelated to waku

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Methods moved at 63d7c36

Let's have a discussion tomorrow about doing more in this PR. If I have time budget for that, it is interesting test topic to cover.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have ideas for new interesting tests then it makes sense to keep this open and add new tests next week let's say.
But this week you should focus on Nomos as we discussed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fbarbu15 I've processed all remaining issues on my list, and I think we can close the PR. Please let me know what you think.

Copy link

@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.

LGTM except that I don't see any waiting.

From what I understand of nwaku peer management, peer info is going to be populated only after some time has passed (full connection cycle).

I'm afraid that without waiting for some unknown time (1s?) the tests will always fail.

@romanzac romanzac requested a review from fbarbu15 June 20, 2024 19:48
src/env_vars.py Outdated
@@ -14,9 +14,10 @@ def get_env_var(var_name, default=None):


# Configuration constants. Need to be upercase to appear in reports
DEFAULT_NWAKU = "harbor.status.im/wakuorg/nwaku:latest"
# DEFAULT_NWAKU = "harbor.status.im/wakuorg/nwaku:latest"
DEFAULT_NWAKU = "wakuorg/nwaku:wakunode2-v0.28.0-peerdb"
Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert this change

Copy link
Collaborator Author

@romanzac romanzac Jun 21, 2024

Choose a reason for hiding this comment

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

Fixed at e081390 and d77636d

src/node/waku_node.py Outdated Show resolved Hide resolved
src/steps/peer_store.py Outdated Show resolved Hide resolved
src/steps/peer_store.py Outdated Show resolved Hide resolved
src/steps/peer_store.py Outdated Show resolved Hide resolved
@romanzac
Copy link
Collaborator Author

fix: added delays to let nodes finish discovery

Delays added at db5a76b

@romanzac romanzac requested a review from fbarbu15 June 21, 2024 13:27
Copy link
Collaborator

@fbarbu15 fbarbu15 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@romanzac romanzac merged commit ccad2a1 into master Jun 21, 2024
1 check passed
@romanzac romanzac deleted the test-peer-connection-management branch June 21, 2024 15:40
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

3 participants