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

core/tests/connection_limit: Fix flake in max_established_incoming #2295

Merged
merged 3 commits into from
Oct 20, 2021

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Oct 15, 2021

The test max_established_incoming starts two networks with a
configured connection limit, spawns up to limit + 1 connections and
expects the last connection to be closed due to being over the limit.

The previous test implementation depended on both networks to handle
each connection in sequence, which is not always the case.

E.g. while network 2 might expect the last connection to close, network
1 might finish upgrading the last connection before the second to last
connection, thus expecting the second to last connection to close.

This commit drives network 1 and 2 in sequence and ensures both networks
are finished upgrading a connection before starting a new connection. In
addition it upgrade the test to use quickcheck.

The test `max_established_incoming` starts two networks with a
configured connection limit, spawns up to `limit + 1` connections and
expects the last connection to be closed due to being over the limit.

The previous test implementation depended on both networks to handle
each connection in sequence, which is not always the case.

E.g. while network 2 might expect the last connection to close, network
1 might finish upgrading the last connection before the second to last
  connection, thus expecting the second to last connection to close.

This commit drives network 1 and 2 in sequence and ensures both networks
are finished upgrading a connection before starting a new connection. In
addition it upgrade the test to use `quickcheck`.
@mxinden
Copy link
Member Author

mxinden commented Oct 19, 2021

@thomaseizinger can you take a look? It is not ideal (mostly due to the conflicts resulting from NetworkEvent having a lifetime), still, especially since the test is flaky on our CI, I think it is a step forward.

@thomaseizinger
Copy link
Contributor

@thomaseizinger can you take a look? It is not ideal (mostly due to the conflicts resulting from NetworkEvent having a lifetime), still, especially since the test is flaky on our CI, I think it is a step forward.

Will have a look tomorrow!

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Looks sound to me!

@mxinden mxinden merged commit cd2588e into libp2p:master Oct 20, 2021
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.

2 participants