Skip to content

Test store dependency binding #1620

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

Merged
merged 4 commits into from
Nov 4, 2022
Merged

Conversation

mbrandonw
Copy link
Member

This makes it so that the test context is set when constructing the TestStore and in the modify closures of send/receive, and provides a trailing closure initializer for TestStore for customizing dependencies.

This idea came from @crayment in #1613.

reducer: Reducer,
updateDependencies: (inout DependencyValues) -> Void = { _ in },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this'll be a trailing closure without argument labels most of the time, but what do you think of:

Suggested change
updateDependencies: (inout DependencyValues) -> Void = { _ in },
prepareDependencies: (inout DependencyValues) -> Void = { _ in },

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that works for me. will update

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes!

@crayment
Copy link
Contributor

crayment commented Nov 4, 2022

Love it, thanks!!

@stephencelis stephencelis merged commit 9a22c5a into main Nov 4, 2022
@stephencelis stephencelis deleted the test-store-dependency-binding branch November 4, 2022 17:43
@crayment
Copy link
Contributor

crayment commented Nov 9, 2022

I just realized receive should probably be updated as well to have the test stores dependencies for updateExpectingResult. Totally overlooked that in my first discussion.

@mbrandonw
Copy link
Member Author

@crayment That should be the case by virtue of the fact that we updated the private function expectedStateShouldMatch, and both send and receive make use of it. Are you seeing otherwise?

We probably should update the test to get coverage on this fact though.

@crayment
Copy link
Contributor

crayment commented Nov 9, 2022

I haven't verified on this branch yet. Still using my helpers. If you give me a bit I can verify, and maybe even PR an update to the test 😄

@mbrandonw
Copy link
Member Author

I did just verify locally with a test and it seems OK. But yes please feel free to PR a test to get coverage.

@crayment
Copy link
Contributor

crayment commented Nov 9, 2022

Done: #1644

Feel free to just close if you like your test updates better though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants