-
Notifications
You must be signed in to change notification settings - Fork 45
[fix]: improve optional handling in TestApplyContext #361
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,12 @@ 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 | ||
|
|
@@ -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 | ||
|
|
@@ -209,10 +227,25 @@ struct TestApplyContext<Wrapped: Workflow>: ApplyContextType { | |
| case .workflow(let workflow): | ||
| return workflow[keyPath: keyPath] | ||
| case .expectations(var expectedValues): | ||
| guard let value = expectedValues.removeValue(forKey: keyPath) 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.") | ||
| guard | ||
| // We have an expected value | ||
| let value = expectedValues.removeValue(forKey: keyPath), | ||
| // And it's the right type | ||
| let value = value as? Value | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a case where a completely different type could be stored here, not related to the optionality? since these are both in the same
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's a good point... currently i think it's impossible since this path isn't fully implemented to support actually putting things into the expectations dictionary, but i think you're right the cases should be handled more granularly. thanks!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated to just mark this path as currently unsupported rather than piggy-backing on the unimplemented expectations dictionary feature. now we don't care about the actual type, just the optionality and will error in all cases if there's no workflow provided to resolve the values from. |
||
| else { | ||
| // 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.") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not crash like you do below?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this way we can just fail the test and allow other tests to proceed rather than halting the entire process
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that desirable? I worry that this will hide bugs if you don't happen to notice the log amidst log noise.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the expectation is that this would be called from a test, so failing the test vs terminating the process seems marginally better to me. i don't feel particularly strongly about it though, so happy to leave it as it was. there is some precedent for this in the way
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i guess another benefit of this approach is it allows unit tests in this repo to exercise that path somewhat
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, if |
||
| 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 {} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without separating the dictionary lookup from the cast, if you didn't pass in a
Workflowinstance to the.tester()API and theapply()implementation was reading a value of optional type, this line would always 'pass' and returnnilwhen we really want it to fail with the informative error messages below.