-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
net/mock: support ConnectionGater in MockNet #2297
Conversation
I should note as well - the local and remote checks are run on the same thread as part of connecting which I believe matches the real behaviour where if the security handshake or protocol negotiation fails |
f235c5c
to
9370213
Compare
Updated the public interface to support passing in
|
What's the motivation behind this PR? mocknet is deprecated and will be replaced with an in-memory-transport soon. |
We're using mocknet in our testing but ran into limitations in our ability to test things because the connection gater wasn't in use - in particular we're unable to test that peers that a downscored past a certain point are then disconnected and banned from reconnecting. A more realistic in-memory-transport sounds excellent but if that's not likely to be ready in the next release it would be nice to get this in if possible to unblock our testing until the in memory transport is available. Long term it sounds like in-memory transport would let our test setup be much closer to the production setup which would be a big win so I'd definitely be keen to switch over to it. |
There's a flaky mocknet test:
Can you please debug and fix? |
Yep I'll see what I can find. |
I made that test and can help debugging it |
0341f79
to
6b5ceee
Compare
@Wondertan some debugging help would be useful. Even on the current master branch I get failures when running the test multiple times. I added:
to It looks like when it fails it's because it gets an additional connected event when it's expecting disconnected events. Throwing a bunch of printlns and a
|
6b5ceee
to
9370213
Compare
@ajsutton, I just tried and cannot reproduce this flake on master, however, I am able to reproduce this on mocknet-gater branch with GOMAXPOCS set to 1,2,3 |
9370213
to
2691caf
Compare
ah, looks like it got fixed on master at some point since I branched this off.
but on current master it passes consistently for me. So I've rebased this on master and now it's passing reliably on this branch too. |
ok, something different this time:
If I'm reading that test right its using a real swarm host, not the mock net though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this looks good. Could you try rerunning CI again? I think you hit a flaky test not related to these changes. (I would, but GH doesn't let me)
Apologies for the delay to review
2691caf
to
ad86cc3
Compare
I couldn't rerun the test in GH either but have rebased this on top of latest master which got them to run again. |
@MarcoPolo Looks happy now. |
Great! Thanks for contribution! |
Adds support for enforcing
ConnectionGater
checks toMockNet
. This enables integration tests to test connection gating policies such as peer blocking policies.MockNet doesn't actually use security handshakes or a connection upgrade process, so the whole cycle is simulated with calls to
InterceptSecured
andInterceptUpgraded
being done when the connection is opened.