Exhaustive RenderTester state assertion #238
Conversation
- Before: To perform exhaustive state assertions with a RenderTester you have to manually keep track of the initial state. - After: Added RenderTesterResult/assertState which makes it easy to perform exhaustive state assertions by mutating the initial state
|
|
| file: StaticString = #file, | ||
| line: UInt = #line, | ||
| changes: (inout WorkflowType.State) throws -> Void | ||
| ) rethrows -> RenderTesterResult<WorkflowType> where WorkflowType.State: Equatable { |
There was a problem hiding this comment.
nit: there is an extension below (~L130) where the utilities for equatable states should probably all go
| ) rethrows -> RenderTesterResult<WorkflowType> where WorkflowType.State: Equatable { | ||
| var initialState = self.initialState | ||
| try changes(&initialState) | ||
| XCTAssertEqual(initialState, state, "Initial state did not match new state", file: file, line: line) |
There was a problem hiding this comment.
this error message seems a little confusing to me. we don't actually expect the 'initial state' to match the new state right? we expect the explicitly transformed state to match. not sure what the best wording to communicate this is though...
There was a problem hiding this comment.
hmm yeah this doesn't make sense... perhaps something more vague like
Expected state does not match
There was a problem hiding this comment.
also perhaps I should reverse the order here
XCTAssertEqual(state, initialState
| /// - changes: A function that receives the initial state | ||
| /// and is expected to mutate it to match the new state. | ||
| @discardableResult | ||
| public func assertState( |
There was a problem hiding this comment.
i wonder if naming like assertStateModifications, assertModifiedState, assertTransformedState, or something along those lines might be a bit more clear. since there's already an assert(state:) method, might be good to make the difference a bit more obvious.
There was a problem hiding this comment.
assertStateModifications is pretty close to what I have now if you read the argument as part of the name
assertState(changes:)
There was a problem hiding this comment.
come to think of it assert(stateChanges:) or assert(stateModifications:) might be a closer match to the existing pattern. wdyt?
There was a problem hiding this comment.
i guess my personal preference is just to try and avoid having a number of different methods that end up reading like assert { ... } with different closure types, as i find that harder to search for, and harder to understand when visually scanning.
There was a problem hiding this comment.
Thanks for this improvement. Looks good to me, and I agree with the naming/phrasing tweaks that you and @jamieQ are discussing.
This reminded me of a post I read arguing against exhaustive testing in TCA. But re-reading the post, it really only makes a good case against mandating all tests be exhaustive over state and effects.
| let result = TestWorkflow() | ||
| .renderTester(initialState: .idle) | ||
| .render { _ in } | ||
|
|
There was a problem hiding this comment.
Shall we test that assertState can succeed as well?
I'm not sure why we haven't done so for the other, existing assert/verify methods 🤔
There was a problem hiding this comment.
We are already doing that in WorkflowRenderTesterTests/test_childWorkflowOutput for both this new method and verifyState
|
|
||
| /// Exhaustive state testing against the initial state. | ||
| /// - Parameters: | ||
| /// - changes: A function that receives the initial state |
There was a problem hiding this comment.
nit: param name doesn't match
Checklist