Replay testing #300
Replay testing #300
Conversation
jeffschoner
force-pushed
the
replay-testing
branch
from
790166f to
a506122
Compare
jeffschoner
force-pushed
the
replay-testing
branch
from
a506122 to
1889fb0
Compare
| end | ||
|
|
||
| # Runs a replay test on a JSON string directly. See comment on replay_history_json_file more details. | ||
| def replay_history_json(workflow_class, json) |
There was a problem hiding this comment.
Feels a bit more composable to have just one typesafe replay_history method which takes a History and then have some helper methods to convert from proto/json/files to History
Was perusing the python version, might be worth looking for insights there as well.
There was a problem hiding this comment.
It's not as close as it could be because temporal-ruby already has its own representation of workflow histories that I didn't want to refactor, but I was able to bring its more composable API shape over here
| attempt: 1, | ||
| workflow_run_id: 'run_id', | ||
| workflow_id: 'workflow_id', | ||
| workflow_name: history.find_event_by_id(1).attributes.workflow_type.name |
There was a problem hiding this comment.
nit: Could probably use a comment or assertion that this is the event you expect.
| # | ||
| # If the pretty_print optional parameter is set to true, it outputs in a more human | ||
| # readable form on output. | ||
| def self.correct_event_types(text, pretty_print: true) |
There was a problem hiding this comment.
Is this because Stripe is on an old version of the Java SDK? Don't see this function used anywhere. Should it be tested? Or maybe just done on the stripe fork?
There was a problem hiding this comment.
I believe this originally started with older versions of tctl. Unfortunately, we standardized our internal tooling on this camel case enum style rather than the standard screaming snake case version. It probably does make more sense to leave it out of here, as any new histories collected using current Temporal UI/CLI or these new APIs won't need to rely on this. I agree if it's kept, it should be tested.
| # Duplicate the configuration so that this doesn't interfere with other tests in the | ||
| # same process that are not replay tests | ||
| @config = config.dup.tap do |c| | ||
| # Otherwise, replay tests will produce no logs. This helps with test debugging. |
There was a problem hiding this comment.
Interesting idea. I like it, and I couldn't find evidence that the temporal SDKs do this.
I think people would want this in tests, but perhaps not if doing safe-rollout deploys (might be scary to see redundant logs during production rollouts). I wonder if this should be configurable.
There was a problem hiding this comment.
Could be a nice option to have during testing, certainly, but yeah I'd make it configurable defaulting to off.
| @@ -33,7 +33,7 @@ def initialize(workflow_class, history, task_metadata, config, track_stack_trace | |||
|
|
|||
| def run | |||
| dispatcher.register_handler( | |||
| History::EventTarget.workflow, | |||
| History::EventTarget.start_workflow, | |||
There was a problem hiding this comment.
Curious what's going on here. I ask because it's pretty central code.
There was a problem hiding this comment.
The non-determinism checking that happens in discard_command only considers the "target" of a command/event and only includes it and the event ID in its error messages. Prior to this change, any operation affecting a workflow had the same target and therefore the same target in any error messages. I've specialized these targets to individual ones for starting, completing, failing and continuing as new. This comes with two major benefits.
First, the non-determinism checking now applies between these different cases. This means a history that succeeded but on replay tries to fail or continue as new will now be considered non-deterministic while previously this would have been acceptable
Second, the error message will now differentiate between these cases rather than a generic "workflow" error that may be non-obvious to the user. E.g., this error message:
Unexpected command. The replaying code is issuing: (23, workflow) but the history of previous executions recorded: (23, activity)
becomes something like,
Unexpected command. The replaying code is issuing: (23, fail_workflow) but the history of previous executions recorded: (23, activity)
On top of that, we now call discard_command in the handlers for these workflow ending events, which means we do non-determinism checking on workflow end. This is important in catching cases where a workflow history ends but on replay the code keeps running.
This .particular change is simply a consequence of that specialization of event targets. I could have left it in place, but felt it would be more confusing to see a workflow target only for starting workflows, while the other cases all have their own targets.
|
I still plan to make some of the suggested improvements, but marking this as "ready to review" to make clear it's in a reviewable state. |
| ) | ||
| end | ||
|
|
||
| it 'replay continues an new when history succeeded' do |
There was a problem hiding this comment.
*as new
s/succeeded/completed/
jeffschoner
force-pushed
the
replay-testing
branch
from
592b738 to
cab8e1a
Compare
|
@drewhoskins-temporal @Sushisource Thanks for the feedback. I've incorporated your suggestions @DeRauk Ready for your review too |
|
This is really great, thank you @jeffschoner! |
Summary
Adds a
ReplayTesterlike the replayer abstractions in official SDKs. This allows workflow histories to be replayed against code for testing purposes to detect non-determinism before a change goes to production.This consists of a set of smaller changes. These can be reviewed commit by commit:
ReplayTesterabstraction itselfSignalWithStartWorkflowand a script to update its history that demonstrates collecting histories in JSON and protobuf binaryTesting