[fix]: improve optional handling in TestApplyContext #361
Conversation
jamieQ
force-pushed
the
jquadri/apply-context-test-optional-fix
branch
from
ada9392 to
65d5322
Compare
| // 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.") |
There was a problem hiding this comment.
Why not crash like you do below?
There was a problem hiding this comment.
this way we can just fail the test and allow other tests to proceed rather than halting the entire process
There was a problem hiding this comment.
Is that desirable? I worry that this will hide bugs if you don't happen to notice the log amidst log noise.
There was a problem hiding this comment.
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 RenderTester handles certain cases IIRC.
There was a problem hiding this comment.
i guess another benefit of this approach is it allows unit tests in this repo to exercise that path somewhat
There was a problem hiding this comment.
oh, if reportIssue fails the test then it's fine I suppose
| @@ -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 { | |||
There was a problem hiding this comment.
without separating the dictionary lookup from the cast, if you didn't pass in a Workflow instance to the .tester() API and the apply() implementation was reading a value of optional type, this line would always 'pass' and return nil when we really want it to fail with the informative error messages below.
| // We have an expected value | ||
| let value = expectedValues.removeValue(forKey: keyPath), | ||
| // And it's the right type | ||
| let value = value as? Value |
There was a problem hiding this comment.
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 guard we're still lumping those two conditions together - "did I get a value" vs. "is it the right type".
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
this change makes two improvements to the current implementation of
TestApplyContext:TestApplyContextin favor of an alternate case denoting the fact that client code provided no mechanism to resolveWorkflowvalues at runtimeTestApplyContextso tests can continue running rather than resulting in a fatal error