Skip to content

Fixed runSideEffect key in WorkflowConcurrency Worker #202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WorkflowConcurrency/Sources/Worker.swift
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ struct WorkerWorkflow<WorkerType: Worker>: Workflow {
func render(state: State, context: RenderContext<WorkerWorkflow>) -> Rendering {
let logger = WorkerLogger<WorkerType>()
let sink = context.makeOutputSink()
context.runSideEffect(key: "") { lifetime in
context.runSideEffect(key: state) { lifetime in
let send: @MainActor(Output) -> Void = sink.send
let task = Task {
logger.logStarted()
Expand Down
137 changes: 129 additions & 8 deletions WorkflowConcurrency/Tests/WorkerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import XCTest
class WorkerTests: XCTestCase {
func testWorkerOutput() {
let host = WorkflowHost(
workflow: TaskTestWorkerWorkflow(key: "")
workflow: TaskTestWorkerWorkflow(key: "", initialState: 0)
)

let expectation = XCTestExpectation()
Expand All @@ -38,8 +38,120 @@ class WorkerTests: XCTestCase {
disposable?.dispose()
}

func testWorkflowUpdate() {
// Create the workflow which causes the TaskTestWorker to run.
let host = WorkflowHost(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify: is this expected to enter into a rendering loop given the behavior of the workflow/worker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The init for WorkflowHost calls render so it's expected that the render loop start the worker after the host is created.

workflow: TaskTestWorkerWorkflow(key: "", initialState: 0)
)

var expectation = XCTestExpectation()
// Set to observe renderings
// This expectation should be called after the TaskTestWorker runs and
// updates the state.
var disposable = host.rendering.signal.observeValues { rendering in
expectation.fulfill()
}

// Test to make sure the initial state of the workflow is correct.
XCTAssertEqual(0, host.rendering.value)

// Wait for the worker to run.
wait(for: [expectation], timeout: 1.0)
// Test to make sure the rendering after the worker runs is correct.
XCTAssertEqual(1, host.rendering.value)

disposable?.dispose()

expectation = XCTestExpectation()
// Set to observe renderings
// This expectation should be called after the workflow is updated.
// After the host is updated with a new workflow instance the
// initial state should be 1.
disposable = host.rendering.signal.observeValues { rendering in
expectation.fulfill()
}

// Updated the workflow to a new initial state.
host.update(workflow: TaskTestWorkerWorkflow(key: "", initialState: 1))

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Updated the test to use a more obvious changed value.


expectation = XCTestExpectation()
// Set to observe renderings
// This expectation should be called when the worker runs.
// The worker isEquivalent is false because we have changed the initialState.
disposable = host.rendering.signal.observeValues { rendering in
expectation.fulfill()
}

// Wait for the worker to trigger a rendering.
wait(for: [expectation], timeout: 1.0)
// Check to make sure the rendering is correct.
XCTAssertEqual(2, host.rendering.value)
}

func testWorkflowKeyChange() {
// Create the workflow which causes the TaskTestWorker to run.
let host = WorkflowHost(
workflow: TaskTestWorkerWorkflow(key: "", initialState: 0)
)

var expectation = XCTestExpectation()
// Set to observe renderings
// This expectation should be called after the TaskTestWorker runs and
// updates the state.
var disposable = host.rendering.signal.observeValues { rendering in
expectation.fulfill()
}

// Test to make sure the initial state of the workflow is correct.
XCTAssertEqual(0, host.rendering.value)

// Wait for the worker to run.
wait(for: [expectation], timeout: 1.0)
// Test to make sure the rendering after the worker runs is correct.
XCTAssertEqual(1, host.rendering.value)

disposable?.dispose()

expectation = XCTestExpectation()
// Set to observe renderings
// This expectation should be called after the workflow is updated.
// After the host is updated with a new workflow instance the
// initial state should be 1.
disposable = host.rendering.signal.observeValues { rendering in
expectation.fulfill()
}

// Update the workflow to a new key which should force the worker to run.
host.update(workflow: TaskTestWorkerWorkflow(key: "key", initialState: 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this line were to be removed, what happens on line 136? does the assertion fail b/c the rendering is 2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove the host.update call then line 133 times out because we didn't do anything to cause another render.


// Wait for the workflow to render after being updated.
wait(for: [expectation], timeout: 1.0)
// Test to make sure the rendering matches the existing state
// since the inititalState didn't change.
XCTAssertEqual(1, host.rendering.value)

expectation = XCTestExpectation()
// Set to observe renderings
// This expectation should be called when the worker runs.
// The worker should run because the key was changed for the workflow.
disposable = host.rendering.signal.observeValues { rendering in
expectation.fulfill()
}

// Wait for the worker to trigger a rendering.
wait(for: [expectation], timeout: 1.0)
// Check to make sure the rendering is correct.
// The worker adds one to the initialState so this should be 1.
XCTAssertEqual(1, host.rendering.value)
}

func testExpectedWorker() {
TaskTestWorkerWorkflow(key: "123")
TaskTestWorkerWorkflow(key: "123", initialState: 0)
.renderTester()
.expectWorkflow(
type: WorkerWorkflow<TaskTestWorker>.self,
Expand Down Expand Up @@ -127,11 +239,11 @@ private struct TaskTestWorkerWorkflow: Workflow {
typealias Rendering = Int

let key: String

func makeInitialState() -> Int { 0 }
let initialState: Int
func makeInitialState() -> Int { initialState }

func render(state: Int, context: RenderContext<TaskTestWorkerWorkflow>) -> Int {
TaskTestWorker()
TaskTestWorker(initialState: initialState)
.mapOutput { output in
AnyWorkflowAction { state in
state = output
Expand All @@ -141,18 +253,27 @@ private struct TaskTestWorkerWorkflow: Workflow {
.running(in: context, key: key)
return state
}

func workflowDidChange(from previousWorkflow: TaskTestWorkerWorkflow, state: inout Int) {
if previousWorkflow.initialState != initialState {
state = initialState
}
}
}

private struct TaskTestWorker: Worker {
typealias Output = Int

let initialState: Int

func run() async -> Int {
do {
try await Task.sleep(nanoseconds: 10000000)
} catch {}

return 1
return initialState + 1
}

func isEquivalent(to otherWorker: TaskTestWorker) -> Bool { true }
func isEquivalent(to otherWorker: TaskTestWorker) -> Bool {
return otherWorker.initialState == initialState
}
}