identify: Fix flaky tests

[MarcoPolo]

Depends on the updated version of libp2p/go-libp2p-peerstore#199. Tests will fail until that gets updated.

@marten-seemann do you have any tips on avoiding the WaitForDisconnectNotification hack? We need someway of knowing when things got the disconnected notification so we know when we can actually probe the address book for these tests.

In draft until libp2p/go-libp2p-peerstore#199 gets merged.

[marten-seemann]

@marten-seemann do you have any tips on avoiding the WaitForDisconnectNotification hack? We need someway of knowing when things got the disconnected notification so we know when we can actually probe the address book for these tests.

Haven't done a careful review yet, but 2 things come to mind:

1. We had this long-standing PR to send swarm notifications synchronously: send notifications synchronously go-libp2p-swarm#295. We blocked this change on some (suspected) problems with bitswap, but we can merge this for the next release. Would that be helpful to you?
2. Is using the PeerConnectednessChanged event an option?

[MarcoPolo]

1. We had this long-standing PR to send swarm notifications synchronously: send notifications synchronously go-libp2p-swarm#295. We blocked this change on some (suspected) problems with bitswap, but we can merge this for the next release. Would that be helpful to you?

This should obviate the need for this hack as long as identify doesn't handle this notification async

2. Is using the PeerConnectednessChanged event an option?

No, we need to know when identify finished processing the disconnect notification

[MarcoPolo]

This is basically ready, but I'm going to hold for a bit to see where #1562 ends up. If that gets merged I can remove my hack :)

[MarcoPolo]

With the sync notifs change #1562 I could get rid of the old hack. Thanks!

[marten-seemann]

There's a failing identify tests in this run: https://github.com/libp2p/go-libp2p/runs/6696246165?check_suite_focus=true.
Is that a separate issue, or should that one have been fixed by this PR?

[MarcoPolo]

There's a failing identify tests in this run: https://github.com/libp2p/go-libp2p/runs/6696246165?check_suite_focus=true. Is that a separate issue, or should that one have been fixed by this PR?

I don't think those are fixed, but I'll add some logging around them since it seems the only way they fail is if identifyConn fails.

[MarcoPolo]

I'm not really sure how something like https://github.com/libp2p/go-libp2p/runs/6696246165?check_suite_focus=true#step:9:923 fails since it presumably waited for identify to happen. And IdentifyConn blocks until identify finishes or errors. If it finished it should have the new addrs since that blocks. If it errored I don't really know why. So I added some logging to the tests and a warn if the identify conn results in an error.

[MarcoPolo]

@marten-seemann I'm trying to reproduce the issues here in this branch (by rerunning ci manually), but I think if this looks good we can also merge and fix up the other flakes in another PR. This PR should help debug those flakey tests with the new logging.