protocols/kad: Fix tests #1320
protocols/kad: Fix tests #1320
Conversation
protocols/kad/src/behaviour/test.rs
Outdated
| Async::NotReady => break, | ||
| } | ||
| } | ||
| block_on(async move { |
There was a problem hiding this comment.
I find the test easier to understand, when we first poll all swarms except the first one, giving them a chance to make progress, and then check whether the first swarm has bootstrapped successfully.
What do you think @romanb? I would apply the same pattern to the similar tests below in case people are in favor of this style.
There was a problem hiding this comment.
I'm generally OK with it, though I don't find it simpler personally, but it seems to be semantically equivalent (i.e. each swarm is polled until pending or the expected result appears). However, I have two small comments:
- Is the
'outerloop label necessary (i.e. why notreturnas before)? - If I see it correctly, then while all swarms are "not ready" / "pending" (e.g. waiting for I/O), this variant seems to enter a "busy loop", not yielding control of the thread to do other useful things meanwhile. While this probably doesn't matter much especially since the tests use the
MemoryTransport, I still find it cleaner to yield the thread when all swarms are pending.
There was a problem hiding this comment.
Another small difference to the previous code seems to be that this version does not fail the test if any other swarm besides the first produces a BootstrapResult (which would naturally be incorrect and unexpected). Again, this may not be important, but it is a difference.
|
I'm not a fan of using |
|
@tomaka see #1320 (comment). I would like to get your input on the style before I touch all following tests and remove their |
|
I went for the
|
|
Could you rebase or merge the latest |
protocols/kad/src/jobs.rs
Outdated
| // Polling with an instant beyond the deadline for the next run | ||
| // is guaranteed to run the job, without the job needing to poll the `Delay` | ||
| // and thus without needing to run `poll` in the context of a task | ||
| // for testing purposes. |
There was a problem hiding this comment.
This comment does no longer reflect reality and should be removed, as the test is now always run in the context of a task (necessarily, because poll() now takes the task context as a parameter.
There was a problem hiding this comment.
Sorry for missing that, thanks.
protocols/kad/src/jobs.rs
Outdated
| // Polling with an instant beyond the deadline for the next run | ||
| // is guaranteed to run the job, without the job needing to poll the `Delay` | ||
| // and thus without needing to run `poll` in the context of a task | ||
| // for testing purposes. |
There was a problem hiding this comment.
Just like the other case, this comment should now be removed.
|
@romanb could you take another look? |
This pull request adjusts the Kademlia tests to the stable future refactoring.
(+ 2 minor fixes in noise and mdns)