Fix: Increment stats.MessagesSent in msgToStream() function #441
Conversation
Extract the code that is common in TestMessageResendAfterError, TestMessageSendTimeout and TestMessageSendNotSupportedResponse to a separate function.
Let prepareNetwork() make simmetric setup with two `ErrHost`s with two `impl` networks to be sure we test `impl` instances on both ends.
The test shows we have a problem with `MessagesSent` counter.
Fixes the bug with incrementing `MessagesSent` counter only in `SendMessage()` method if `impl`. Now it works for `MessageSender` too.
wolneykien
changed the title
|
When working on the branch I've once got the following error on CircleCI: Locally, I've got the same error 6-9 times out of 16 runs in a row and for the master branch too. So I think that the test is unstable. |
|
That test is flaky, created an issue: #442 |
network/ipfs_impl_test.go
Outdated
| // Wait a little for MessagesRecvd counters to be updated | ||
| // after receiver.ReceiveMessage() returns. FIXME: Can we | ||
| // use some network event instead of a timer? | ||
| ctxwait, cancelwait := context.WithTimeout(ctx, 100*time.Millisecond) |
There was a problem hiding this comment.
It may be possible to use bsnet.host.Network().Notify(listener) to listen for ClosedStream(n network.Network, v network.Stream).
Could you give that a shot? If it's more than half an hour of work no worries I think this is good enough.
There was a problem hiding this comment.
That was interesting enough. Now the receiver has listener network.Notifiee field that is used by prepareNetwork() to set up the listener before the network becomes active (that's important). Also I've found that every MessageSender needs to be closed ;) and added .Close() calls to all tests in the file.
|
Also thanks for refactoring to clean up the ipfs impl tests 💯 |
Added `listener network.Notifiee` to the `receiver` structure. If a listener is specified then `prepareNetwork()` connects it to the mock network it builds before making any connections.
Wait for all network streams are closed instead of just using a timeout. The timeout of 5 s is still used as a deadline (it makes the test to fail).
The `MessageSender` needs to be closed if we want all streams in the network to be closed.
|
However, it seems there is another unstable test out there: |
|
Added an issue for the flaky test: ipfs/boxo#88 |
See #440.