Sort order of ports returned to fix flaky tests #1003
Conversation
Signed-off-by: Kevin Earls <kearls@redhat.com>
| @@ -60,28 +61,34 @@ func ConfigToReceiverPorts(logger logr.Logger, config map[interface{}]interface{ | |||
| return nil, ErrReceiversNotAMap | |||
| } | |||
|
|
|||
| sortedNames := make([]string, 0, len(receivers)) | |||
| for name := range receivers { | |||
| sortedNames = append(sortedNames, name.(string)) | |||
There was a problem hiding this comment.
Would it make sense to check the type just for safety here?
| sortedNames = append(sortedNames, name.(string)) | |
| v, ok := name.(string) | |
| if !ok { | |
| logger.Warn("receiver does not match type, want: %s, got: %T", "string", name) | |
| continue | |
| } | |
| sortedNames = append(sortedNames, v) |
| @@ -60,28 +61,34 @@ func ConfigToReceiverPorts(logger logr.Logger, config map[interface{}]interface{ | |||
| return nil, ErrReceiversNotAMap | |||
| } | |||
|
|
|||
| sortedNames := make([]string, 0, len(receivers)) | |||
There was a problem hiding this comment.
@kevinearls wouldn't it be cleaner to sort ports at the end of the function?
There was a problem hiding this comment.
Maybe, but I'm not sure it necessary. Originally when I saw this problem it occurred because we were getting receivers in the wrong order. Sorting them alphabetically by name is fairly simple.
The ports, on the other hand, are in an array of corev1.ServicePort. That does not implement methods needed to feed it to sort.Sort(), so sorting those would be non-trivial.
There was a problem hiding this comment.
What is the root cause of the problem? That the ports are not sorted or receivers? We should use the least error-prone approach that will guarantee always the same result.
Take a look at sort.Slice:
sort.Slice(ports, func(i, j int) bool {
return ports[i].Name < ports[j].Name
})
There was a problem hiding this comment.
The problem I had was caused by the receivers being in the wrong order, not the ports for a receiver.
There was a problem hiding this comment.
In other words, the implementation in this PR will break again if ReceiverParser.Ports() returns randomly sorted ports. In fact right now that API does not guarantee the order.
Also, there could be a unit test checking the functionality - or change the existing tests to assume the order of ports matters.
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
|
@pavolloffay please review and merge if ok |
| @@ -144,6 +144,39 @@ service: | |||
| } | |||
| } | |||
|
|
|||
| func TestReceiverPortsSortedCorrectly(t *testing.T) { | |||
There was a problem hiding this comment.
Thanks for the test!
It would be better to change the TestExtractPortsFromConfig because it does the same thing - check what ports are returned and just change how the assert is written in that function (see my other comment).
|
|
||
| ports, err := adapters.ConfigToReceiverPorts(logger, configuration) | ||
| assert.NoError(t, err) | ||
| assert.Len(t, ports, 4) |
There was a problem hiding this comment.
This PR changes the semantics of ConfigToReceiverPorts - the expected result is strictly ordered, hence we can write a nice assert and compare slices directly:
assert.Equal(error, []v1.port{{Name: "foo", Port: 1778944}{...}}, got)| continue | ||
| } | ||
|
|
||
| if len(rcvrPorts) > 0 { | ||
| sort.Slice(ports, func(i, j int) bool { |
There was a problem hiding this comment.
From all changes in this file only these 3 lines are needed 101-103 and move then right before the return.
Signed-off-by: Kevin Earls <kearls@redhat.com>
| @@ -89,5 +90,12 @@ func ConfigToReceiverPorts(logger logr.Logger, config map[interface{}]interface{ | |||
| ports = append(ports, rcvrPorts...) | |||
| } | |||
| } | |||
|
|
|||
| if len(ports) > 0 { | |||
There was a problem hiding this comment.
the check should not be needed
* Sort order of ports returned to fix flaky tests Signed-off-by: Kevin Earls <kearls@redhat.com> * Verify type of receiver name Signed-off-by: Kevin Earls <kearls@redhat.com> * Sort ports too before returning Signed-off-by: Kevin Earls <kearls@redhat.com> * Add unit test Signed-off-by: Kevin Earls <kearls@redhat.com> * respond to comments Signed-off-by: Kevin Earls <kearls@redhat.com> * fix nits Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls kearls@redhat.com
This should fix #969. If there is more than one receiver defined, ports may be returned in a different order than that in which they were created. Ideally they would always be in the same order as they are defined, but because Go does not guarantee order when iterating over keys from a map, we cannot guarantee this.