Send ProtocolMessageCommands where appropriate #1107
Conversation
Adds ProtocolMessageCommands to outgoing commands for a WFT response for update accepted and completed messages but not rejections as rejections are not written as events. Entries in the message outbox now contain the message itself as well as a predicate to test history events that may have been created in response to this message. In this way the messages and the event expectations are fully defined in internal_update.go and the event handling and replay code can remain ignorant of the mapping between message types and the resulting event types.
| "update_with_protocol_message_command", | ||
| } { | ||
| s.Run(name, func() { | ||
| err := replayer.ReplayWorkflowHistoryFromJSONFile(ilog.NewDefaultLogger(), name+".json") |
There was a problem hiding this comment.
can we add some tests for failure cases? Another integration test case I would love to see is accepting, completing and accepting+completing an update in the last workflow task of a history and verifying the commands are present in history. Currently all those situations are broken and I was expecting ProtocolMessageCommands to fix them.
There was a problem hiding this comment.
I can do that. FWIW that was fixed even before ProtocoMessageCommand went in on the server. There was a time when it was broken though - you didn't hallucinate that.
There was a problem hiding this comment.
I added replay tests for complete and accept+complete in the last WFT and for a non-deterministic update implementation ... can you think of anything else apart from very obviously broken things like events in the wrong place that would get caught in features?
There was a problem hiding this comment.
can you think of anything else apart from very obviously broken things like events in the wrong place that would get caught in features
not sure I understand the question? could you rephrase it?
There was a problem hiding this comment.
Basically just trying to brainstorm negative test scenarios beyond non-determinism.
There was a problem hiding this comment.
No other then trying different events in the update handler to cause non determinism
For workflows that were started before the new ProtocolMessageCommand was in use, replay them to completion without that feature rather than enabling it within the run. New workflow runs will always have that feature flag set as part of handling the WorkflowExecutionStarted event.
Testing situations where - update acceptance _and_ completion, and - just update completion occur and are sent to the server on the WFT completion request that _also_ carries the command indicating that the workflow itself has completed.
| } | ||
|
|
||
| func (wc *workflowEnvironmentImpl) ScheduleUpdate(name string, args *commonpb.Payloads, hdr *commonpb.Header, callbacks UpdateCallbacks) { | ||
| wc.updateHandler(name, args, hdr, callbacks) | ||
| } | ||
|
|
||
| func (wc *workflowEnvironmentImpl) Send(msg *protocolpb.Message) { | ||
| wc.outbox = append(wc.outbox, msg) | ||
| func withExpectedEventPredicate(pred func(*historypb.HistoryEvent) bool) msgSendOpt { |
There was a problem hiding this comment.
If this is the only msgSendOpt, I think it's premature to event have a msgSendOpt abstraction. Just take the optional predicate on Send until such abstraction is needed. Or if it's really needed, just use a struct for options instead of this options pattern we don't really use much in the SDK (though I know the server does a lot).
(sorry this sat pending a while, this is definitely optional and can ignore if you'd like)
What was changed
Adds ProtocolMessageCommands to outgoing commands for a WFT response for update accepted and completed messages but not rejections as rejections are not written as events. Entries in the message outbox now contain the message itself as well as a predicate to test history events that may have been created in response to this message. In this way the messages and the event expectations are fully defined in internal_update.go and the event handling and replay code can remain ignorant of the mapping between message types and the resulting event types.
Why?
Allows for update messages to be sequence with respect to commands.
Checklist
Closes
How was this tested:
New and existing unit tests. Features tests all still pass.
No