node: Fix disabled node tests#4824
Conversation
Eric-Warehime
requested review from
algonautshant,
algorandskiy,
brianolson and
winder
Codecov Report
@@ Coverage Diff @@
## master #4824 +/- ##
==========================================
- Coverage 54.64% 54.18% -0.46%
==========================================
Files 416 416
Lines 53710 53710
==========================================
- Hits 29350 29105 -245
- Misses 21928 22159 +231
- Partials 2432 2446 +14
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
algonautshant
left a comment
algonautshant
left a comment
There was a problem hiding this comment.
Great to have these tests come back online.
Some comments about the sad return of ExtedPeerList.
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
This looks great! Please fix MakeFull and this good to go.
| require.NoError(t, err) | ||
|
|
||
| node, err := MakeFull(logging.Base().With("source", t.Name()+strconv.Itoa(i)), rootDirectory, cfg, []string{}, g) | ||
| node, err := MakeFull(logging.Base().With("source", t.Name()+strconv.Itoa(i)), rootDirectory, cfg, append(neighbors[:i], neighbors[i+1:]...), g) |
There was a problem hiding this comment.
I do not think this works as intended - it keeps copying into the same slice:
func main() {
a := []int{1, 2, 3, 4, 5, 6, 7}
b := append(a[:2], a[3:]...)
c := append(a[:3], a[4:]...)
fmt.Printf("%v\n", a)
fmt.Printf("%v\n", b)
fmt.Printf("%v\n", c)
}Output
[1 2 4 6 7 7 7]
[1 2 4 6 7 7]
[1 2 4 6 7 7]
There was a problem hiding this comment.
Fixed in the most recent patch. Thanks for pointing this out. Let me know if you know of a better way to do this than what I've done which is just copy and append.
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
Please remove redundant copy. LGTM
|
|
||
| for i := range nodes { | ||
| var nodeNeighbors []string | ||
| copy(nodeNeighbors, neighbors) |
There was a problem hiding this comment.
nodeNeighbors has length 0 so nothing copied.
copied := copy(nodeNeighbors, neighbors)
fmt.Printf("copied = %d\n", copied)
copied = 0so simple appending as below is fine
3 participants
Summary
These node tests look like they've been disabled for a very long time--some of them since the initial commit.
I've fixed these tests with some caveats.
-shortExtendPeerListmethod to theGossipNode. The tests originally required this which wasn't an issue because the phonebook was located in the mode, but when it got moved out we lost the ability to update the phonebook peers manually.Test Plan
Unit tests.