add updatable timer sample #120
Conversation
There was a problem hiding this comment.
Looks great! One request - can we add a test for this workflow? I know we don't technically have them for all samples, but we do for most. May be able to leverage time skipping and even check history events if you'd like. Let us know if we can help w/ the test.
There was a problem hiding this comment.
@cretz Added tests, let me know if there's anything you'd change, thanks!
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
Mostly LGTM, only thing really needed IMO is to add an event check on the test that does the update. Everything else optional.
| (MyWorkflow wf) => wf.RunAsync(wakeUpTime), | ||
| new(id: $"workflow-{Guid.NewGuid()}", taskQueue: worker.Options.TaskQueue!)); | ||
|
|
||
| await AssertMore.EventuallyAsync(async () => |
There was a problem hiding this comment.
Would move this to after GetResultAsync and not have it be an EventuallyAsync but rather just check the history inline
| // Sleep for 30 days | ||
| await env.DelayAsync(TimeSpan.FromDays(30)); |
There was a problem hiding this comment.
This is not needed, GetResultAsync automatically skips time
| var wakeUpTime2 = await handle.QueryAsync(workflow => workflow.GetWakeUpTime); | ||
| Assert.Equal(inAnHour, wakeUpTime2, precision: TimeSpan.FromSeconds(5)); | ||
|
|
||
| await handle.GetResultAsync(); |
There was a problem hiding this comment.
Would suggest checking history after this one, including confirming that a timer canceled event is present for that first timer
|
|
||
| namespace TemporalioSamples.UpdatableTimer; | ||
|
|
||
| public class UpdatableTimer(DateTimeOffset wakeUpTime) |
There was a problem hiding this comment.
Optional neat improvement, support providing a summary for the timer that appears in the UI (there is a WaitConditionAsync overload that accepts options now which includes a summary)
| [Fact] | ||
| public async Task SimpleRun_Succeeds() | ||
| { | ||
| await using var env = await WorkflowEnvironment.StartLocalAsync(); |
There was a problem hiding this comment.
Technically "start local" is a tad heavy so we prefer reusing it in tests by having classes extend WorkflowEnvironmentTestBase and use a unique task queue (though not all tests seem to have done that). But I didn't catch it before and it's not a big deal.
| var handle = await env.Client.StartWorkflowAsync( | ||
| (MyWorkflow wf) => wf.RunAsync(wakeUpTime), | ||
| new(id: $"workflow-{Guid.NewGuid()}", taskQueue: worker.Options.TaskQueue!)); | ||
| await handle.GetResultAsync(); |
There was a problem hiding this comment.
Ok, I see the problem. This hangs in CI for 30 days heh, may want start time skipping instead.
There was a problem hiding this comment.
No probs, have updated to use skipping.
|
There is a slight .NET whitespace formatting issue that showed up in CI for another sample, I am going to fix in your branch then merge. |
2 participants
What was changed
Add an updatable timer sample
Closes Sample request: Updatable timer #96 and Sample request: Updateable timer #59
How was this tested:
Tested this manually by running it a few times.
Any docs updates needed?
Updated the main readme