Hi, I was wondering if the nodes returned by GetNodes should be ordered in some way. I couldn't find anything in the readme or handout but the tests seem to indicate that they should appear from "oldest to most recently discovered". I wanted to know if it was important as right now my implementation doesn't return them in a deterministic order, so I had to change the tests to check for membership instead of checking if they're at a given index, for example I replaced:
require.Equal(t, "127.0.0.1:2001", g2.GetNodes()[1])
by
require.Contains(t, g2.GetNodes(), "127.0.0.1:2001")
Hi,
Strictly speaking, you're right that the handouts don't specify the order, and thus the order should be irrelevant for the test. If you could verify that your test passes after you made the change you suggested, then don't worry, you'll get the points for the test. There's no need to change your implementation to preserve the order.
In the interest of not changing the tests, we won't push a change for this assignment. However, we will keep this in mind for the next assignments.
Thanks a lot for reporting the issue.
Cristina
Great, thanks a lot !
Hello,
I have checked as you asked if sorting the nodes in ascending order would fix this problem but it doesn't. My solution returns the nodes in an unpredictable order for performance reasons so it would be great if the tests could be upgraded to take into account this fact.
It thinks that the tests assumes that the order correspond to the order of addition of those nodes.
Best regards,
Bastien
Hi,
The order of nodes is irrelevant and the change your colleague suggested fixes the issue, i.e., replacing "require.Equal(t, "127.0.0.1:2001", g2.GetNodes()[1])" with "require.Contains(t, g2.GetNodes(), "127.0.0.1:2001")". As I mentioned above, if you pass the test with this change, it's totally fine.
Perhaps I misunderstood your question?
Cristina
Sorry, my question wasn't clear.
What I wanted to ask is wether you will you do a patch for the tests or not ?
Bastien
