Fixed runSideEffect key in WorkflowConcurrency Worker #202
Conversation
Changed WorkerWorkflow to use it’s state as the key for the runSideEffect call so that if the state changes the worker is re-run. Updated unit tests to test for this issue of the side effect not running. Added unit tests for testing that updating the key in the render call causes the worker to run.
mjohnson12
changed the title
| // Wait for the workflow to render after being updated. | ||
| wait(for: [expectation], timeout: 1.0) | ||
| // Test to make sure the rendering matches the initial state. | ||
| XCTAssertEqual(1, host.rendering.value) |
There was a problem hiding this comment.
i think the most recent expectation enforces this, but since the last thing asserted on the rendering prior to this was that it was also equal to 1, perhaps we should use a different initial state value to make it more obvious what the expected behavior is here. IIUC, we could pass in a state of like 7 on line 75, and then assert it's still the same here, and that it is incremented by 1 on line 93. is that right?
There was a problem hiding this comment.
Yes. Updated the test to use a more obvious changed value.
| } | ||
|
|
||
| // Update the workflow to a new key which should force the worker to run. | ||
| host.update(workflow: TaskTestWorkerWorkflow(key: "key", initialState: 0)) |
There was a problem hiding this comment.
if this line were to be removed, what happens on line 136? does the assertion fail b/c the rendering is 2?
There was a problem hiding this comment.
If you remove the host.update call then line 133 times out because we didn't do anything to cause another render.
| @@ -38,8 +38,120 @@ class WorkerTests: XCTestCase { | |||
| disposable?.dispose() | |||
| } | |||
|
|
|||
| func testWorkflowUpdate() { | |||
| // Create the workflow which causes the TaskTestWorker to run. | |||
| let host = WorkflowHost( | |||
There was a problem hiding this comment.
to clarify: is this expected to enter into a rendering loop given the behavior of the workflow/worker?
There was a problem hiding this comment.
The init for WorkflowHost calls render so it's expected that the render loop start the worker after the host is created.
Per CR changed intial state of the updated workflow to be a more obvious state change
Changed WorkerWorkflow to use it’s state as the key for the runSideEffect call so that if the state changes the worker is re-run.
Updated unit tests to test for this issue of the side effect not running.
Added unit tests for testing that updating the key in the render call causes the worker to run.
Checklist