-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(i): Fix the nodes ID/index bug in tests #3162
test(i): Fix the nodes ID/index bug in tests #3162
Conversation
9ca479a
to
fb558ab
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3162 +/- ##
===========================================
- Coverage 78.10% 78.01% -0.10%
===========================================
Files 354 354
Lines 34660 34660
===========================================
- Hits 27071 27038 -33
- Misses 5972 5996 +24
- Partials 1617 1626 +9
Flags with carried forward coverage won't be shown. Click here to find out more. see 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// greater than 0. For example if requesting a node with nodeID=2 then the resulting output will contain only | ||
// one element (at index 0) caller might accidentally assume that this node belongs to node 0. Therefore, the | ||
// caller should always use the returned IDs, instead of guessing the IDs based on node indexes. | ||
func getNodesWithIDs(nodeID immutable.Option[int], nodes []clients.Client) ([]int, []clients.Client) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: this could be a custom iterator https://pkg.go.dev/iter that returns nodeID and node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do so in future if others feel strongly for it.
## Relevant issue(s) Resolves sourcenetwork#3076 ## Description - Fix the `getNodes` bug - Fix the temporary duplications in some actions - Remove the `getNodesCollection`
Relevant issue(s)
Resolves #3076
Description
getNodes
buggetNodesCollection