Skip to content
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

Add overloads of TestStore.send that accept CaseKeyPaths #2681

Merged
merged 2 commits into from Feb 28, 2024

Conversation

scogeo
Copy link
Contributor

@scogeo scogeo commented Dec 23, 2023

Recently the ability to receive actions in a TestStore was updated to support CasePaths. This was a great improvement in ergonomics, but unfortunately the ability to send actions into the store using CasePaths is missing. This leads to a bit of a dichotomy when writing test cases when using deeply nested enums in actions.

For example, this snippet from a test case in the VoiceMemoTests reads a little weird with a mixture of CasePath syntax and standard enum construction syntax.

await store.send(.recordingMemo(.presented(.onTask)))
await store.send(.recordingMemo(.presented(.stopButtonTapped))) {
  $0.recordingMemo?.mode = .encoding
}
await store.receive(\.recordingMemo.finalRecordingTime) {
  $0.recordingMemo?.duration = 2.5
}

This PR allows this now to be written as:

await store.send(\.recordingMemo.presented.onTask)
await store.send(\.recordingMemo.presented.stopButtonTapped) {
  $0.recordingMemo?.mode = .encoding
}
await store.receive(\.recordingMemo.finalRecordingTime) {
  $0.recordingMemo?.duration = 2.5
}

This significantly improves the ergonomics of writing test cases, and also improves the readability of tests cases.

While not shown in this example, the PR also supports the ability to send associated values of enums, and is included in the test case for this feature.

@stephencelis
Copy link
Member

stephencelis commented Dec 23, 2023

@scogeo Thanks for taking the time to PR this! It's actually something we talked about, but we felt that having two ways of doing the same thing was enough reason to avoid the extra API. In the case of receive, there are payloads you can ignore:

store.receive(.response(.success(MyResponse(/* complicated payload */))))
// vs.
store.receive(\.response.success)  // no payload

Meanwhile, this is not the case with send

store.send(.textFieldChanged("Hello"))
// vs.
store.send(\.textFieldChanged)  // 🛑 can't send

Even if we allow payloads as a secondary argument, the receive has the benefit of asserting equality on just the payload while saving you from having to mark every enum Equatable, which is not always easy to do:

// Need to manipulate the `Result`'s error type to be `Equatable` and not `any Error`:
store.receive(.response(.success(MyResponse(/* complicated payload */))))

// Can simply equate on the `MyResponse` type:
store.receive(\.response.success, MyResponse(/* complicated payload */))

The send is still just an alias, since payloads aren't asserted against:

// 2 ways of doing the exact same thing? Is there a difference other than syntax?

store.send(.textFieldChanged("Hello"))

store.send(\.textFieldChanged, "Hello")

Does our line of thought make sense? Is there something we're missing?

If I really stretch my thinking, losing the nested parentheses is nice, but then would we extend this to syntax to sending actions to normal Stores? Going down that path seems a little muddy to me.

@scogeo
Copy link
Contributor Author

scogeo commented Dec 23, 2023

@stephencelis You make some good points about the differences in motivation between send and receive. This is definitely syntactic sugar for the send case.

My motivation for submitting the PR came from refactoring a large Reducer and adding additional nested enums. It is much easier to do such refactoring when the code is written using CasePaths, rather than actual nested enums which require the nested parentheses.

And in some cases, it is not just nested parentheses, it is a completely differently style of writing the code.

For example, this snippet also from VoiceMemoTests which sends an IdentifiedAction:

await store.send(.voiceMemos(.element(id: deadbeefURL, action: .playButtonTapped))) {
  $0.voiceMemos[id: deadbeefURL]?.mode = .playing(progress: 0)
}
await store.receive(\.voiceMemos[id:deadbeefURL].delegate.playbackStarted)
await self.clock.run()

With the PR, this could then be rewritten as:

await store.send(\.voiceMemos[id:deadbeefURL].playButtonTapped) {
 $0.voiceMemos[id: deadbeefURL]?.mode = .playing(progress: 0)
}
await store.receive(\.voiceMemos[id:deadbeefURL].delegate.playbackStarted)
await self.clock.run()

This looks much more symmetric and is easier for my mind to process. So while this is definitely syntactic sugar, it adds symmetry to send and receive that is currently absent, and makes test cases easier to comprehend (and maintain) especially for deeply nested enums which are very common in test cases.

With regards to Store, I don't see the need to add the equivalent overloads. Invoking send on Store typically only happens in the context of a View which sends view specific actions which rarely consist of nested enums. The common exception being the ViewAction design pattern, which I use extensively. But the observation-beta branch has officially codified support of the ViewAction design pattern and added a @ViewAction macro to assist in embedding view actions with a send method. So even this common case will be handled in the near future.

In the end, this may just be a personal coding style preference. But I do find the asymmetry between send and receive on TestStore to be a bit jarring in actual use when writing test cases. Since Store does not have the equivalent receive method, this asymmetry will never be seen in code when using a Store so there is no need to add the equivalent functionality.

@stephencelis
Copy link
Member

@scogeo We're definitely not opposed to the idea. Let us just think on it a bit longer.

@scogeo
Copy link
Contributor Author

scogeo commented Jan 2, 2024

@stephencelis Sounds good.

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.

Sorry for the hold up, but after some time to think it over we think that it makes sense to have these overloads. Thanks for taking the time to PR!

@stephencelis stephencelis merged commit b4a3d04 into pointfreeco:main Feb 28, 2024
7 checks passed
@scogeo scogeo deleted the teststore-send-casekeypath branch February 29, 2024 00:00
cgrindel-self-hosted-renovate bot added a commit to cgrindel/rules_swift_package_manager that referenced this pull request Feb 29, 2024
…ure to from: "1.9.0" (#940)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[pointfreeco/swift-composable-architecture](https://togithub.com/pointfreeco/swift-composable-architecture)
| minor | `from: "1.8.2"` -> `from: "1.9.0"` |

---

### Release Notes

<details>
<summary>pointfreeco/swift-composable-architecture
(pointfreeco/swift-composable-architecture)</summary>

###
[`v1.9.0`](https://togithub.com/pointfreeco/swift-composable-architecture/releases/tag/1.9.0)

[Compare
Source](https://togithub.com/pointfreeco/swift-composable-architecture/compare/1.8.2...1.9.0)

#### What's Changed

See [Migrating to
1.9](https://togithub.com/pointfreeco/swift-composable-architecture/blob/main/Sources/ComposableArchitecture/Documentation.docc/Articles/MigrationGuides/MigratingTo1.9.md)
for more details.

- Added: New versions of `TestStore.send` that accept case key paths
(thanks [@&#8203;scogeo](https://togithub.com/scogeo),
[pointfreeco/swift-composable-architecture#2681;
[pointfreeco/swift-composable-architecture#2868).
- Added `Reducer.dependency(value)`, for overriding a reducer's
dependency using a singleton value of a type
([pointfreeco/swift-composable-architecture#2863).
- Fixed: Improve `Store` diagnostics for deriving bindings
([pointfreeco/swift-composable-architecture#2793).
- Fixed: Avoid erroneous perception checks when `ViewStore`s are
initialized in a view that doesn't use `WithPerceptionTracking`
([pointfreeco/swift-composable-architecture#2849).
- Fixed: Support `#if` branching in `@ObservableState` and enum
`@Reducer`s
([pointfreeco/swift-composable-architecture#2800).
- Infrastructure: Tree navigation documentation fixes (thanks
[@&#8203;imjn](https://togithub.com/imjn),
[pointfreeco/swift-composable-architecture#2837);
presentation reducer documentation fixes (thanks
[@&#8203;ozumin](https://togithub.com/ozumin),
[pointfreeco/swift-composable-architecture#2853).
- Infrastructure: Improve tutorial diffing (thanks
[@&#8203;oka-yuji](https://togithub.com/oka-yuji),
[pointfreeco/swift-composable-architecture#2844).
- Infrastructure: Expand release build test coverage
([pointfreeco/swift-composable-architecture#2856).
- Infrastructure: Document gotcha with macros and previews
([pointfreeco/swift-composable-architecture#2855).

#### New Contributors

- [@&#8203;imjn](https://togithub.com/imjn) made their first
contribution in
[pointfreeco/swift-composable-architecture#2837
- [@&#8203;oka-yuji](https://togithub.com/oka-yuji) made their first
contribution in
[pointfreeco/swift-composable-architecture#2844
- [@&#8203;ozumin](https://togithub.com/ozumin) made their first
contribution in
[pointfreeco/swift-composable-architecture#2853

**Full Changelog**:
pointfreeco/swift-composable-architecture@1.8.2...1.9.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjM2LjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
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.

None yet

2 participants