-
Notifications
You must be signed in to change notification settings - Fork 55
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
IWF-385: return channel sizes for RPC #502
Conversation
"testing" | ||
) | ||
|
||
func TestSignalWorkflowNoWorkflowId(t *testing.T) { |
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.
Moved from another file as it's a new test and the file was too big
integ/util.go
Outdated
@@ -178,6 +178,12 @@ func doStartIwfServiceWithClient(config IwfServiceTestConfig) (uclient uclient.U | |||
} | |||
} | |||
|
|||
func panicAError(err error) { |
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.
panicAtError
?
integ/workflow/locking/routers.go
Outdated
@@ -107,6 +111,19 @@ func (h *handler) ApiV1WorkflowWorkerRpc(c *gin.Context) { | |||
}) | |||
return | |||
} | |||
|
|||
signalChannelInfo := (*req.SignalChannelInfos)[UnusedSignalChannelName] | |||
if signalChannelInfo.GetSize() != 4 { |
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.
I am not following this part. What is this check? Why the size is expected to be 4?
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.
I just added a few comments to the test.
The signals are sent in a different file locking_test.go
.
Changed to a constant to be more clear Lol.
infos := map[string]iwfidl.ChannelInfo{} | ||
for name, l := range i.receivedData { | ||
infos[name] = iwfidl.ChannelInfo{ | ||
Size: ptr.Any(int32(len(l))), |
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.
Size is the number of messages, not actual data size in bytes, right?
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.
Yeah, l is the list of all the messages. So len(l) is the number of the messages.
receivedData
is a map of list.
@@ -94,6 +94,7 @@ func doTestLockingWorkflow(t *testing.T, backendType service.BackendType, config | |||
panicAtHttpError(err, httpResp) | |||
|
|||
for i := 0; i < 4; i++ { | |||
// send 4 unused signals at the beginning to validate the ChannelInfo feature |
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.
👍
@@ -23,6 +26,16 @@ func (i *InternalChannel) GetAllReceived() map[string][]*iwfidl.EncodedObject { | |||
return i.receivedData | |||
} | |||
|
|||
func (i *InternalChannel) GetInfos() map[string]iwfidl.ChannelInfo { | |||
infos := map[string]iwfidl.ChannelInfo{} |
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.
Since we know the size, can we not create a map with a preallocated size here?
infos := map[string]iwfidl.ChannelInfo{} | |
infos := make(map[string]iwfidl.ChannelInfo, len(i.receivedData)) |
@@ -237,6 +238,16 @@ func (sr *SignalReceiver) GetAllReceived() map[string][]*iwfidl.EncodedObject { | |||
return sr.receivedSignals | |||
} | |||
|
|||
func (sr *SignalReceiver) GetInfos() map[string]iwfidl.ChannelInfo { | |||
infos := map[string]iwfidl.ChannelInfo{} |
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.
Same as #502 (comment)
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.
added. Good catch :D
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.
Left a small suggestion, but overall looks good! 🙌
Description
Checklist
Related Issue
Closes #issue_number