uber-go / cadence-client

Framework for authoring workflows and activities running on top of the Cadence orchestration engine.
https://cadenceworkflow.io
MIT License
339 stars 128 forks source link

Add the change in branch number case(test) to replayersuite #1236

Closed agautam478 closed 1 year ago

agautam478 commented 1 year ago

What changed?

Why?

How did you test it? yes, locally

Potential risks NA

Groxx commented 1 year ago

Since these don't have branches, nor do they have arguments that change the number of branches: what does this PR do?

agautam478 commented 1 year ago

@Groxx

If the number of expected branches is changed the replayer should catch it as a non deterministic error. TestBranchWorkflowWithExtraBranch tests this case.

sampleBranchWorkflow2 has the logic where an extra branch in added in :

for i := 1; i <= 4; i++ { activityInput := fmt.Sprintf("branch %d of 4", i) future := workflow.ExecuteActivity(ctx, sampleActivity, activityInput) futures = append(futures, future) }

The value was 3 when the history was recorded. Refer to https://docs.google.com/document/d/1lwADEFAkaxOiu0bbNXeMmdrPJYCnOBXyw8VJ-aJNqzM/edit point 1.

Groxx commented 1 year ago

ah, right, this is the "branch" workflow from samples, which... also doesn't really have branches 🤦

If we wanted to make this one clearer about the difference, we could register a more dynamic workflow instead. e.g.

func makeworkflow(count) func(workflow.Context) error {
  return func(workflow.Context) error {
    // ...
    for i := 0; i < count; i++ {
    // ...
  }
}

// and in the test:
replayer.RegisterWorkflowWithOptions(makeworkflow(3), ...) // this one should work
replayer.RegisterWorkflowWithOptions(makeworkflow(4), ...) // this one should fail

since that way it's clear what changed, which is the only relevant part of the test.

Groxx commented 1 year ago

alrighty. given the similarity to the sample terminology and code, how about:

otherwise looks good, it's pretty straightforward and the tests are descriptive and correct 👍