mdns: don't discover ourselves #1661
Conversation
|
Looks like we had a test case |
|
Thank you for your reply. |
|
Because after this code modification, both "TestSelfDiscovery" and "TestOtherDiscovery" failed. I have a bold guess. Before we modify the code, whether these two tests passed because peer found itself. But this is just my assumption. Looking forward to your reply. |
I have no idea... |
|
Then let's remove it. I don't see a use case for self-discovery. @MGMCN, do you want to modify the test cases? |
|
Thank you for your reply. I have removed this test function(TestSelfDiscovery). |
|
Looks like there's another test that's failing as well. |
|
Thank you for your reply. Yes, I've seen it. I'll check it tomorrow. |
|
Hello, I think I probably know why our TestOtherDiscovery test failed. for _, id := range hostIDs { ... )Because we traverse all hostids in line 82 of the mdns_test.go, but because we have modified the mDNS code, we make the current host unable to find itself. In other words, when we test the file, if we find that the ith host is checking whether we have found peers other than ourselves, we should skip checking whether this host is in our discovery list. But if we follow the original logic, we will also find out whether ith host are in this ith host's discovery list. After I modify the logic of the test code, the test case passes. |
|
Sorry, is there anything else I need to correct about this PR? |
p2p/discovery/mdns/mdns_test.go
Outdated
| ids := make([]peer.ID, 0, len(infos)) | ||
| for _, info := range infos { | ||
| ids = append(ids, info.ID) | ||
| } | ||
| if !containsAllHostIDs(ids) { | ||
| if !containsAllHostIDs(ids,i) { |
There was a problem hiding this comment.
Why not just pass the peer ID into this function?
There was a problem hiding this comment.
Thank you for your suggestion.
I've revised it.
Do you think it's ok?
Please contact me if there is anything else that needs to be modified.
|
Hahaha. |
|
Thank you @MGMCN! |
No description provided.