fix(tests): testautorelay #1121
Conversation
|
|
||
| check addresses != switchClient.peerInfo.addrs | ||
| await allFutures(switchClient.stop(), switchRelay.stop()) | ||
|
|
||
| asyncTest "Three relays connections": |
There was a problem hiding this comment.
@lchenut what is this test checking exactly?
There was a problem hiding this comment.
I'll answer every question about this test here.
So. This badly named "Three relays connections" tests multiple things. The two big things tested is:
- the maximum number of relay line 119:
let autorelay = AutoRelayService.new(2, relayClient, checkMA, newRng())(here it's a limit of 2) - what happens in case of a disconnection of a remote peer line 127
await rel2.stop()
What does this test do in details:
- It creates 4 switches. switchClient is the "autorelay switch", the other three (rel1, rel2, rel3) are standard relay server switches.
- State == 0: checkMA is called for the first time because switchClient connects (and reserves) to rel1
- State == 1: switchClient connects (and reserves) to rel2 (line 124). You can notice that switchClient connects to rel3 aswell but it doesn't reserve, due to the maximum number of relay set to 2.
- State == 2: rel2 disconnect, we return to the "first state" with only rel1 reserved by switchClient
- State == 3: now that rel2 is disconnected, there's room for another relay, and rel3 takes this place.
There was a problem hiding this comment.
Thanks a lot.
tests/testautorelay.nim
Outdated
| @@ -95,21 +95,25 @@ suite "Autorelay": | |||
| relayClient = RelayClient.new() | |||
| proc checkMA(addresses: seq[MultiAddress]) = | |||
| if state == 0 or state == 2: | |||
There was a problem hiding this comment.
Why do we need this state variable and what's the reasoning behind the conditions?
| buildRelayMA(rel1, switchClient) in addresses | ||
| buildRelayMA(rel2, switchClient) in addresses | ||
| addresses.len() == 2 | ||
| let relay1MAs = buildRelayMA(rel1, switchClient) |
There was a problem hiding this comment.
Why do we use relays 1 and 2 here?
| buildRelayMA(rel1, switchClient) in addresses | ||
| buildRelayMA(rel3, switchClient) in addresses | ||
| addresses.len() == 2 | ||
| let relay1MAs = buildRelayMA(rel1, switchClient) |
There was a problem hiding this comment.
Why do we use relays 1 and 3 here?
tests/testautorelay.nim
Outdated
| buildRelayMA(rel1, switchClient) in addresses | ||
| buildRelayMA(rel3, switchClient) in addresses | ||
| addresses.len() == 2 | ||
| let relay1MAs = buildRelayMA(rel1, switchClient) |
There was a problem hiding this comment.
why do we test this again here?
There was a problem hiding this comment.
It's indeed a bit overkill. Not mandatory.
tests/testautonatservice.nim
Outdated
| @@ -23,7 +23,7 @@ import stubs/autonatclientstub | |||
| proc createSwitch(autonatSvc: Service = nil, withAutonat = true, maxConnsPerPeer = 1, maxConns = 100, nameResolver: NameResolver = nil): Switch = | |||
| var builder = SwitchBuilder.new() | |||
| .withRng(newRng()) | |||
| .withAddresses(@[ MultiAddress.init("/ip4/0.0.0.0/tcp/0").tryGet() ], false) | |||
| .withAddresses(@[ MultiAddress.init("/ip4/0.0.0.0/tcp/0").tryGet() ]) | |||
There was a problem hiding this comment.
This wasn't necessary, this test wasn't failing.
| await rel2.stop() | ||
| await fut.wait(1.seconds) |
There was a problem hiding this comment.
I would leave this. Without this await, the test could finish before all the check are done.
There was a problem hiding this comment.
Actually, all awaits are necessary. It was "working" cause I removed await fut.wait(1.seconds) so no check was being done as you said. But I used futures instead, let me know what you think.
Fix
testautorelaydue to wildcard resolver and improve its readability.Related to #1099