Unit tests for DSL workflow #87
Conversation
vishwa-uber
requested review from
Groxx,
ibarrajo,
taylanisikdemir and
davidporter-id-au
|
do you mind adding a description for the diff? |
| env.RegisterActivityWithOptions(sampleActivity, activity.RegisterOptions{ | ||
| Name: "sampleActivity", | ||
| }) | ||
| env.ExecuteWorkflow(func(ctx workflow.Context) error { |
There was a problem hiding this comment.
Just trying to keep up, I think this is basically creating an inline simple workflow, I guess for the intention of testing the different parts of the DSL example
Was this your intention to create a mini-workflow in this callback? I might need to zoom briefly to just understand the intent.
This is fine from a code perspective and it increases coverage, but I'm just not sure if its testing the intended code-path.
There was a problem hiding this comment.
ty, explained offline, I didn't read the code properly, the tt.fields.execute is a workflow function. All good then
There was a problem hiding this comment.
yea... without some further assertions in here (e.g. "called X", "took less than X+Y time", etc) these aren't testing much of anything except "doesn't return error" which could be true for many, many reasons.
short-cutting the real workflow for a simpler one: generally seems fine, but I don't think it's necessary here, the real workflow looks essentially identical. it's just saving adding a layer or two of boilerplate structure (root.sequence), by adding a layer of boilerplate structure (separate bindings arg, custom workflow).
There was a problem hiding this comment.
Unlike other workflows that return some value DSL doesn't return any value other than an error. So could you elaborate more on what assertions I need to do here?
My intent of writting these unit tests was to get better understanding of how this workflow works. Even if any user wants to understand DSL unlike other workflows this doesn't have any unit tests; by writing a few I thought would help
There was a problem hiding this comment.
there are a few options:
env.OnActivity(...)to mock out the activities and assert that they are called, chaining args / mapped variables correctly (because that's an important part of what this workflow does)env.SetOnActivityCompletedListenerto check the return values of the activities, because they're actually being run currently and you can make sure the output matches what it should (even if the workflow doesn't return it)env.OnActivity(...).After(time.Minute)and make sure that the parallel execution'senv.Now()change before/afterenv.ExecuteWorkflowis less than all the activities running serially (to show it runs them concurrently)
probably others too, but those are OTOH and would assert meaningful things.
I'd also probably just define the tests in yaml, rather than using a format that doesn't match how the workflow's actual runs work. then you can also use the workflow as-is rather than writing a custom one, and you can use the pre-defined ones and assert that they do what is expected.
cmd/samples/dsl/workflow_test.go
Outdated
| env.SetOnActivityStartedListener(func(activityInfo *activity.Info, ctx context.Context, args encoded.Values) { | ||
| activityType := activityInfo.ActivityType.Name | ||
| activityCalled = append(activityCalled, activityType) | ||
| require.Equal(t, "sampleActivity", activityType) |
There was a problem hiding this comment.
I'm pretty sure these callbacks occur on a different goroutine, so require isn't safe to use - it'll short-circuit execution of that goroutine (... the workflow's? not sure tbh), not the test.
in some ways that's fine, and it should still fail the test, but it's fairly likely to cause other kinds of issues in general. best to avoid unless it's known to be safe.
There was a problem hiding this comment.
makes sense @Groxx, updated the PR with changes. Lmk if it looks good now!
This is to write unit tests for following workflows in DSL, that tests execution with Single Activity, Multiple Activities and execution with Error for the following Statements:
This change also includes unit test for SimpleDSLWorkflow which tests the activityCalled i.e., Test_SimpleDSLWorkflow()