Introduce SideEffect #1021 #1174

dhavalshreyas · 2020-05-19T23:18:26Z

Introduces a SideEffect hook that will be used to perform the functionality currently provided by Workers.

This is a subset of @bencochran's implementation in the bc/guwt-prototype branch.

swift/Workflow/Sources/Lifetime.swift

swift/Workflow/Sources/SubtreeManager.swift

swift/Workflow/Tests/SubtreeManagerTests.swift

bencochran · 2020-05-27T02:49:44Z

swift/WorkflowTesting/Sources/RenderExpectations.swift

@@ -99,6 +102,14 @@ public struct ExpectedWorker {
    }
 }

+public struct ExpectedSideEffect {


I think right now we don’t have a way to just send a WorkflowAction to RenderTester. So we can’t write tests that act like a side-effect did something (and we couldn’t convert ExpectedWorker to be built on ExpectedSideEffect. We should consider adding an action to ExpectedSideEffect or a way to send an action to RenderTester directly.

Good point @bencochran .

or a way to send an action to RenderTester directly.

Any ideas on the best way to do that?

Pass in context, so that we can makeSink() and send event.

Pass in inout state so that we can just modify the state directly.

Add a generic ActionType to ExpectedSideEffect and pass in a sink of ActionType.

@bencochran, @AquaGeek: Please take a look at ExpectedSideEffect when you get a chance. I've made it take in a closure which accepts the context.

Currently, the only init takes in key and action, however, I wanted to make sure we can pass in arbitrary block of code in the future to ensure it's in line with the runSideEffect api.

bencochran · 2020-05-27T02:51:09Z

swift/WorkflowTesting/Sources/WorkflowRenderTester.swift

@@ -230,7 +232,7 @@
    fileprivate final class RenderTestContext<T: Workflow>: RenderContextType {
        typealias WorkflowType = T

-        private var (lifetime, token) = Lifetime.make()
+        private var (lifetime, token) = ReactiveSwift.Lifetime.make()


Do we dislike the need to disambiguate enough to use a different name than Lifetime? I couldn’t come up with anything better when I originally typed my version of this, but wanted to raise the question in case others had thoughts.

I don't think we need to rename just to disambiguate. As we continue to remove the ReactiveSwift dependency from the core library - this will become less of an issue.

swift/Workflow/Sources/RenderContext.swift

swift/Workflow/Tests/SubtreeManagerTests.swift

swift/WorkflowTesting/Sources/WorkflowRenderTester.swift

zach-klippenstein · 2020-06-05T18:23:41Z

Been meaning to review this, hopefully will get to it today or Monday since I want to start working on this for Kotlin soon.

AquaGeek · 2020-06-08T20:46:42Z

swift/WorkflowTesting/Sources/RenderExpectations.swift

@@ -99,6 +104,24 @@ public struct ExpectedWorker {
    }
 }

+public struct ExpectedSideEffect<WorkflowType: Workflow> {
+    let key: AnyHashable
+    let action: ((RenderContext<WorkflowType>) -> Void)?


What's the purpose of this closure?

Currently, the only init takes in key and action, however, I wanted to make sure we can pass in arbitrary block of code in the future to ensure it's in line with the runSideEffect api.

But what is the block intended to do? If you're writing a test for a workflow that directly invokes runSideEffect, what kinds of things would you actually want to assert on?

That runSideEffect was called with a given key.

That runSideEffect was not called with a given key.

Given that a side effect was ran with a given key, that the side effect lambda actually performed some side effect (e.g. called logger.log() with some message).

Given that a side effect was ran with a given key, that the side effect lambda registers the correct logic to be executed in the onEnded handler.

Given that a side effect was ran with a given key, that the side effect lambda sent some action to the event sink.

1 can definitely be done with this API, presumably 2 can as well (by not expecting the key?). I'm not sure how close the Swift render testing API is to the kotlin one yet, but if the testing infrastructure runs the action immediately, then 3 and 5 can be done with existing APIs (3 by asserting the side effect fake was invoked after the render call, 5 using verifyAction(Result)). The only special case is 4, which requires an API to end the lifetime. Some ways to do this:

Pass a flag in ExpectedSideEffect that, if set, will cause the onEnded handlers to be invoked immediately.

An additional API on RenderTestResult called something like endSideEffect(key: String): RenderTestResult that executes the end actions for the given side effect and returns a RenderTestResult that allows verifying if any actions were sent to the sink in the onEnded handler.

I think 1 is the simpler option, although both could be tricky (if the side effect immediately sends an action, and then sends another action when ending, the initial action will always be evaluated first and a second render pass would be needed to evaluate the second, and that's hard to express in the render tester API since it, by definition, tests a single render pass). Perhaps we don't actually need to need to provide the ability to test side effect implementations in this API, and can use the more general "integration" testing API?

In any case, this lambda parameter doesn't seem to address any of these use cases so I'm also not sure what purpose it serves.

This is in response to @bencochran 's feedback here: #1174 (comment)

ExpectedWorker allows you to send an Output back to the RenderContext when executed. Trying to provide the same ability here.

The action is actually not public, it's not meant to be used directly. The two public initializers are:

public init(key: AnyHashable) public init<ActionType: WorkflowAction>(key: AnyHashable, action: ActionType) where ActionType.WorkflowType == WorkflowType

I wanted to make sure ExpectedSideEffect is implemented similarly to how SideEffects are implemented and hence in the future we can provide additional ways to pass WorkflowActions to the RenderContext.

I'm worried we never quite converged our testing APIs, and now they're diverging more. Need to dig into this more to understand where the differences are. I could have sworn we had an issue to track testing APIs but I can't find it.

I could have sworn we had an issue to track testing APIs but I can't find it.

#702?

@zach-klippenstein: #702 is slated for v1.0. Happy to look into it after the GUWT changes.

I think the public parts of ExpectedSideEffect as written here fits with the overall current shape of our RenderTester. Though, yeah, I’d prefer we converged in the direction of #702 and what Kotlin has.

I’m not sure of the need internally to hold a ((RenderContext<WorkflowType>) -> Void)? vs an AnyWorkflowAction<WorkflowType>? but that’s implementation detail that I’m less concerned with, I think?

My plan is to work on the Kotlin side of this once I've split the repo out, this week. If yall are happy with this then SGTM, I'll not look too hard, see what I come up with in Kotlin, hopefully it's the same! And if not, I'll be in a much better position to debate 😉

AquaGeek · 2020-06-08T20:48:52Z

The only part unclear to me is the purpose of the closure on ExpectedSideEffect. What is the use case for that? Are engineers supposed to use that to e.g. fulfill an XCTestExpectation?

AquaGeek

Discussed offline. I'm fine with where this is.

zach-klippenstein

Shouldn't this PR close one or more of the issues from the guwt label? https://github.com/square/workflow/issues?q=is%3Aissue+is%3Aopen+label%3Aguwt

swift/Workflow/Sources/Lifetime.swift

zach-klippenstein · 2020-06-08T23:21:53Z

swift/WorkflowTesting/Sources/RenderExpectations.swift

@@ -99,6 +104,24 @@ public struct ExpectedWorker {
    }
 }

+public struct ExpectedSideEffect<WorkflowType: Workflow> {
+    let key: AnyHashable
+    let action: ((RenderContext<WorkflowType>) -> Void)?


But what is the block intended to do? If you're writing a test for a workflow that directly invokes runSideEffect, what kinds of things would you actually want to assert on?

That runSideEffect was called with a given key.

That runSideEffect was not called with a given key.

Given that a side effect was ran with a given key, that the side effect lambda actually performed some side effect (e.g. called logger.log() with some message).

Given that a side effect was ran with a given key, that the side effect lambda registers the correct logic to be executed in the onEnded handler.

Given that a side effect was ran with a given key, that the side effect lambda sent some action to the event sink.

1 can definitely be done with this API, presumably 2 can as well (by not expecting the key?). I'm not sure how close the Swift render testing API is to the kotlin one yet, but if the testing infrastructure runs the action immediately, then 3 and 5 can be done with existing APIs (3 by asserting the side effect fake was invoked after the render call, 5 using verifyAction(Result)). The only special case is 4, which requires an API to end the lifetime. Some ways to do this:

Pass a flag in ExpectedSideEffect that, if set, will cause the onEnded handlers to be invoked immediately.

An additional API on RenderTestResult called something like endSideEffect(key: String): RenderTestResult that executes the end actions for the given side effect and returns a RenderTestResult that allows verifying if any actions were sent to the sink in the onEnded handler.

I think 1 is the simpler option, although both could be tricky (if the side effect immediately sends an action, and then sends another action when ending, the initial action will always be evaluated first and a second render pass would be needed to evaluate the second, and that's hard to express in the render tester API since it, by definition, tests a single render pass). Perhaps we don't actually need to need to provide the ability to test side effect implementations in this API, and can use the more general "integration" testing API?

In any case, this lambda parameter doesn't seem to address any of these use cases so I'm also not sure what purpose it serves.

zach-klippenstein · 2020-06-08T23:23:47Z

swift/Workflow/Sources/RenderContext.swift

@@ -75,6 +75,10 @@ public class RenderContext<WorkflowType: Workflow>: RenderContextType {
        fatalError()
    }

+    public func runSideEffect(key: AnyHashable, action: (Lifetime) -> Void) {


This needs documentation, especially discussing that the side effect is allowed to send actions to the context's Sink, that the side effect is expected to clean itself up when the Lifetime is ended, that the Lifetime won't end until a render pass in which this function is not called with the given key, etc.

Also worth noting that the given closure will not be executed on subsequent runs of the same side-effect key.

bencochran

Agree with the calls for documentation (sorry, didn’t do any of that in my prototype 🙈), but overall looks good to me

bencochran · 2020-06-10T23:03:01Z

swift/Workflow/Sources/RenderContext.swift

@@ -75,6 +75,10 @@ public class RenderContext<WorkflowType: Workflow>: RenderContextType {
        fatalError()
    }

+    public func runSideEffect(key: AnyHashable, action: (Lifetime) -> Void) {


Also worth noting that the given closure will not be executed on subsequent runs of the same side-effect key.

bencochran · 2020-06-10T23:04:20Z

swift/Workflow/Sources/SubtreeManager.swift

@@ -419,7 +445,7 @@ extension WorkflowNode.SubtreeManager {
                .observe(on: QueueScheduler.workflowExecution)
                .start { [weak self] event in
                    switch event {
-                    case .value(let output):
+                    case let .value(output):


nit: I prefer the let inside the case for the reason outlined in the Google Swift style guide

This will require a swiftformat rules change, will do it in a subsequent PR.

bencochran · 2020-06-10T23:18:35Z

swift/WorkflowTesting/Sources/RenderExpectations.swift

@@ -99,6 +104,24 @@ public struct ExpectedWorker {
    }
 }

+public struct ExpectedSideEffect<WorkflowType: Workflow> {
+    let key: AnyHashable
+    let action: ((RenderContext<WorkflowType>) -> Void)?


I think the public parts of ExpectedSideEffect as written here fits with the overall current shape of our RenderTester. Though, yeah, I’d prefer we converged in the direction of #702 and what Kotlin has.

I’m not sure of the need internally to hold a ((RenderContext<WorkflowType>) -> Void)? vs an AnyWorkflowAction<WorkflowType>? but that’s implementation detail that I’m less concerned with, I think?

First part of Kotlin implementation of square/workflow#1021. Kotlin counterpart to square/workflow#1174. This implementation intentionally does not run the side effect coroutine on the `workerContext` `CoroutineContext` that is threaded through the runtime for testing infrastructure. Initially, workers ran in the same context as the workflow runtime. The behavior of running workers on a different dispatcher by default (`Unconfined`) was introduced in square/workflow#851 as an optimization to reduce the overhead for running workers that only perform wiring tasks with other async libraries. This was a theoretical optimization, since running on the `Unconfined` dispatcher inherently involves less dispatching work, but the overhead of dispatching wiring coroutines was never actually shown to be a problem. Additionally, because tests often need to be in full control of dispatchers, the ability to override the context used for workers was introduced in square/workflow#940, which introduced `workerContext`. I am dropping that complexity here because it adds a decent amount of complexity to worker/side effect machinery without any proven value. It is also complexity that needs to be documented, and is probably just more confusing than anything. The old behavior for workers is maintained for now to reduce the risk of this change, but side effects will always run in the workflow runtime's context. This is nice and simple and unsurprising and easy to reason about.

dhavalshreyas requested review from bencochran, AquaGeek, JustinDSN and asthatrivedi May 19, 2020 23:18

JustinDSN reviewed May 20, 2020

View reviewed changes

swift/Workflow/Sources/Lifetime.swift Show resolved Hide resolved

swift/Workflow/Sources/Lifetime.swift Show resolved Hide resolved

dhavalshreyas force-pushed the dhaval/sideEffect branch from b131954 to b847a7a Compare May 21, 2020 19:53

dhavalshreyas requested review from rjrjr and zach-klippenstein May 21, 2020 22:15

dhavalshreyas force-pushed the dhaval/sideEffect branch from b847a7a to 4103c3f Compare May 27, 2020 00:06

dhavalshreyas mentioned this pull request May 27, 2020

[RFC] [DNM] SideEffect as Type #1182

Closed

bencochran reviewed May 27, 2020

View reviewed changes

dhavalshreyas mentioned this pull request May 29, 2020

Implement keyed side effects in place of / as underpinnings to Worker (Grand Unified Workflow Theory) #1021

Closed

dhavalshreyas force-pushed the dhaval/sideEffect branch from 4103c3f to 4efbd42 Compare June 5, 2020 17:15

dhavalshreyas requested a review from bencochran June 5, 2020 17:24

AquaGeek reviewed Jun 5, 2020

View reviewed changes

swift/Workflow/Sources/RenderContext.swift Show resolved Hide resolved

AquaGeek reviewed Jun 5, 2020

View reviewed changes

swift/Workflow/Tests/SubtreeManagerTests.swift Show resolved Hide resolved

AquaGeek reviewed Jun 5, 2020

View reviewed changes

swift/WorkflowTesting/Sources/WorkflowRenderTester.swift Outdated Show resolved Hide resolved

dhavalshreyas force-pushed the dhaval/sideEffect branch from 4efbd42 to c68e408 Compare June 6, 2020 00:01

dhavalshreyas requested a review from AquaGeek June 6, 2020 00:02

zach-klippenstein changed the base branch from master to trunk June 8, 2020 18:36

AquaGeek reviewed Jun 8, 2020

View reviewed changes

AquaGeek approved these changes Jun 8, 2020

View reviewed changes

zach-klippenstein reviewed Jun 8, 2020

View reviewed changes

bencochran approved these changes Jun 10, 2020

View reviewed changes

zach-klippenstein approved these changes Jun 10, 2020

View reviewed changes

dhavalshreyas force-pushed the dhaval/sideEffect branch from c68e408 to f3cc9b0 Compare June 11, 2020 00:23

dhavalshreyas added 2 commits June 10, 2020 17:30

Introduce SideEffect

857407c

Added ExpectedSideEffect to WorkflowTesting

0fa0ade

dhavalshreyas force-pushed the dhaval/sideEffect branch from f3cc9b0 to 0fa0ade Compare June 11, 2020 00:30

dhavalshreyas merged commit b852e0d into trunk Jun 11, 2020

dhavalshreyas deleted the dhaval/sideEffect branch June 11, 2020 00:39

zach-klippenstein mentioned this pull request Jun 18, 2020

Introduce runningSideEffect API square/workflow-kotlin#12

Merged

2 tasks

dhavalshreyas mentioned this pull request Jun 26, 2020

Update Swiftformat style - Inline case square/workflow-swift#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce SideEffect #1021 #1174

Introduce SideEffect #1021 #1174

dhavalshreyas commented May 19, 2020

bencochran May 27, 2020

dhavalshreyas Jun 4, 2020

dhavalshreyas Jun 5, 2020 •

edited

bencochran May 27, 2020

dhavalshreyas May 28, 2020

zach-klippenstein commented Jun 5, 2020

AquaGeek Jun 8, 2020

dhavalshreyas Jun 8, 2020

zach-klippenstein Jun 8, 2020

dhavalshreyas Jun 9, 2020

zach-klippenstein Jun 9, 2020

AquaGeek Jun 9, 2020

dhavalshreyas Jun 10, 2020

bencochran Jun 10, 2020

zach-klippenstein Jun 10, 2020

AquaGeek commented Jun 8, 2020

AquaGeek left a comment

zach-klippenstein left a comment

zach-klippenstein Jun 8, 2020

zach-klippenstein Jun 8, 2020

bencochran Jun 10, 2020

bencochran left a comment

bencochran Jun 10, 2020

bencochran Jun 10, 2020

dhavalshreyas Jun 11, 2020

bencochran Jun 10, 2020

Introduce SideEffect #1021 #1174

Introduce SideEffect #1021 #1174

Conversation

dhavalshreyas commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhavalshreyas Jun 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zach-klippenstein commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AquaGeek commented Jun 8, 2020

AquaGeek left a comment

Choose a reason for hiding this comment

zach-klippenstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bencochran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhavalshreyas Jun 5, 2020 •

edited