[fix]: improve optional handling in TestApplyContext (#361)

this change makes two improvements to the current implementation of
`TestApplyContext`:

1. it removes the partially-implemented 'expectations dictionary'
functionality from the `TestApplyContext` in favor of an alternate case
denoting the fact that client code provided no mechanism to resolve
`Workflow` values at runtime
2. it allows failures in such a mode to resolve optional workflow
property values from the `TestApplyContext` so tests can continue
running rather than resulting in a fatal error

Author: jamieQ

WorkflowTesting/Sources/WorkflowActionTester.swift

@@ -21,15 +21,20 @@ import XCTest
 @testable import Workflow
 
 extension WorkflowAction {
-    /// Returns a state tester containing `self`.
+    /// Returns a `WorkflowActionTester` with the given state before the `WorkflowAction` has been applied to it.
+    ///
+    /// - Parameters:
+    ///   - state: The `WorkflowType.State` instance that specifies the state before the `WorkflowAction` has been applied.
+    ///   - workflow: An optional `WorkflowType` instance to be used if the `WorkflowAction` needs to read workflow properties off of the `ApplyContext` parameter during action application. If this parameter is unspecified, attempts to access the `WorkflowType`'s properties will error in the testing runtime.
+    /// - Returns: An appropriately-configured `WorkflowActionTester`.
     public static func tester(
         withState state: WorkflowType.State,
         workflow: WorkflowType? = nil
     ) -> WorkflowActionTester<WorkflowType, Self> {
         WorkflowActionTester(
             state: state,
             context: TestApplyContext(
-                kind: workflow.map { .workflow($0) } ?? .expectations([:])
+                kind: workflow.map { .workflow($0) } ?? .unsupported
             )
         )
     }
@@ -70,6 +75,19 @@ extension WorkflowAction {
 ///     .assert(output: .finished)
 ///     .assert(state: .differentState)
 /// ```
+///
+/// If the `Action` under test uses the runtime's `ApplyContext` to read values from the
+/// current `Workflow` instance, then an instance of the `Workflow` with the expected
+/// properties that will be read during `send(action:)` must be supplied like:
+/// ```
+/// MyWorkflow.Action
+///     .tester(
+///         withState: .firstState,
+///         workflow: MyWorkflow(prop: 42)
+///     )
+///     .send(action: .exampleActionThatReadsWorkflowProp)
+///     .assert(...)
+/// ```
 public struct WorkflowActionTester<WorkflowType, Action: WorkflowAction> where Action.WorkflowType == WorkflowType {
     /// The current state
     let state: WorkflowType.State
@@ -193,7 +211,10 @@ struct TestApplyContext<Wrapped: Workflow>: ApplyContextType {
         // FIXME: flesh this out to support 'just in time' values
         // rather than requiring a full Workflow instance to be provided
         // https://github.com/square/workflow-swift/issues/351
-        case expectations([AnyKeyPath: Any])
+        // case expectations([AnyKeyPath: Any])
+
+        /// The client provided no means to resolve values at runtime, and lookups will fail with a runtime error
+        case unsupported
     }
 
     var storage: TestContextKind
@@ -208,11 +229,21 @@ struct TestApplyContext<Wrapped: Workflow>: ApplyContextType {
         switch storage {
         case .workflow(let workflow):
             return workflow[keyPath: keyPath]
-        case .expectations(var expectedValues):
-            guard let value = expectedValues.removeValue(forKey: keyPath) as? Value else {
+        // TODO: implement https://github.com/square/workflow-swift/issues/351
+//        case .expectations:
+//            fatalError("Not yet supported")
+        case .unsupported:
+            // If we're expecting a value of optional type. Error, but don't crash
+            // since we can just return nil.
+            if Value.self is OptionalProtocol.Type {
+                reportIssue("Attempted to read value \(keyPath as AnyKeyPath), when applying an action, but no value was present. Pass an instance of the Workflow to the ActionTester to enable this functionality.")
+                return Any?.none as! Value
+            } else {
                 fatalError("Attempted to read value \(keyPath as AnyKeyPath), when applying an action, but no value was present. Pass an instance of the Workflow to the ActionTester to enable this functionality.")
             }
-            return value
         }
     }
 }
+
+private protocol OptionalProtocol {}
+extension Optional: OptionalProtocol {}

WorkflowTesting/Tests/WorkflowActionTesterTests.swift

@@ -14,8 +14,10 @@
  * limitations under the License.
  */
 
+import IssueReporting
 import Workflow
 import XCTest
+
 @testable import WorkflowTesting
 
 final class WorkflowActionTesterTests: XCTestCase {
@@ -112,14 +114,34 @@ extension WorkflowActionTesterTests {
                 withState: true,
                 workflow: TestWorkflow(prop: 42)
             )
-            .send(action: .readProps)
+            .send(action: .readProp)
+            .assert(state: true)
+            .assert(output: .value("read prop: 42"))
+    }
+
+    func test_new_api_works_with_optional_props() {
+        TestActionWithProps
+            .tester(
+                withState: true,
+                workflow: TestWorkflow(prop: 42, optionalProp: 22)
+            )
+            .send(action: .readOptionalProp)
+            .assert(state: true)
+            .assert(output: .value("read optional prop: 22"))
+
+        TestActionWithProps
+            .tester(
+                withState: true,
+                workflow: TestWorkflow(prop: 42, optionalProp: nil)
+            )
+            .send(action: .readOptionalProp)
             .assert(state: true)
-            .assert(output: .value("read props: 42"))
+            .assert(output: .value("read optional prop: <nil>"))
     }
 
     // FIXME: ideally an 'exit/death test' would somehow be used for this...
     /*
-     func test_old_api_explodes_if_you_use_props() {
+     func test_old_api_explodes_if_accessing_through_apply_context() {
          XCTExpectFailure("This test should fail")
 
          TestActionWithProps
@@ -128,14 +150,24 @@ extension WorkflowActionTesterTests {
              .assert(state: true)
      }
       */
+
+    func test_old_api_errors_accessing_optional_through_apply_context_without_proper_setup() {
+        withExpectedIssue("reading optional value through context without workflow should fail but not crash") {
+            TestActionWithProps
+                .tester(withState: true)
+                .send(action: .readOptionalProp)
+                .assert(state: true)
+        }
+    }
 }
 
 // MARK: -
 
 private enum TestActionWithProps: WorkflowAction {
     typealias WorkflowType = TestWorkflow
 
-    case readProps
+    case readProp
+    case readOptionalProp
     case dontReadProps
 
     func apply(
@@ -146,9 +178,13 @@ private enum TestActionWithProps: WorkflowAction {
         case .dontReadProps:
             return .value("did not read props")
 
-        case .readProps:
+        case .readProp:
             let prop = context[workflowValue: \.prop]
-            return .value("read props: \(prop)")
+            return .value("read prop: \(prop)")
+
+        case .readOptionalProp:
+            let optionalProp = context[workflowValue: \.optionalProp]
+            return .value("read optional prop: \(optionalProp?.description ?? "<nil>")")
         }
     }
 }
@@ -179,6 +215,7 @@ private struct TestWorkflow: Workflow {
     }
 
     var prop = 0
+    var optionalProp: Int? = 42
 
     func makeInitialState() -> Bool {
         true