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

Clarify purpose of handler_stream_test.go #472

Merged

Conversation

Hirochon
Copy link
Contributor

@Hirochon Hirochon commented Mar 5, 2023

Clarify that the sole purpose of the test in handler_stream_test.go is to verify a safety property of the ClientStream iterator. A similar testing strategy isn't worthwhile for ServerStream or BidiStream, because they're not iterators.

Hirochon and others added 7 commits March 5, 2023 21:48
This reverts commit 578709b.
Clarify that the purpose of the test in handler_stream_test.go is to
verify safety properties of our iterator-style implementation. There's
no need for similar tests for the other two stream types.
@akshayjshah akshayjshah changed the title add tests for ServerStream and BidiStream Clarify purpose of handler_stream_test.go Mar 6, 2023
@akshayjshah
Copy link
Member

Hi @Hirochon - thank you for opening this PR! Unfortunately, I'd rather not merge the extra tests you've added. The existing test verifies that the ClientStream iterator allocates a new message for each iteration. (It's a regression test for a fix we made last year.) There's no need for similar tests of ServerStream or BidiStream, because they're not iterators. All three stream types are tested much more extensively in connect_ext_test.go.

However, this PR did highlight that the existing test isn't well-documented. I've pushed some commits to your branch to remove the extra tests and add better documentation. I'm going to amend the PR title and description and merge this in, so that you're still credited in the next set of release notes.

@akshayjshah
Copy link
Member

@Hirochon's original PR description, for posterity:

This pull request adds tests for ServerStream and BidiStream, in addition to the existing tests for ClientStream.

Specifically, the tests cover the following cases:

  • TestServerStream: Tests the Send method of ServerStream.
  • TestBidiStream: Tests the Receive and Send methods of BidiStream.

I have implemented the tests using the nopServerStreamingHandlerConn and nopBidiStreamingHandlerConn structs, which are similar to the nopStreamingHandlerConn struct used in the existing tests for ClientStream. I have also made sure that the new tests are consistent with the existing test code, so that the overall test suite feels consistent and cohesive.

@akshayjshah akshayjshah merged commit 4711715 into connectrpc:main Mar 6, 2023
@Hirochon
Copy link
Contributor Author

Hirochon commented Mar 6, 2023

Hi @akshayjshah
I understand your concerns regarding the extra tests I added. Thank you for explaining the purpose of the existing test and the extensive testing of the other stream types. I appreciate your efforts to improve the documentation and am glad that I could help in this regard.

renovate bot referenced this pull request in open-feature/flagd Apr 14, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[github.com/bufbuild/connect-go](https://togithub.com/bufbuild/connect-go)
| require | minor | `v1.5.2` -> `v1.6.0` |

---

### Release Notes

<details>
<summary>bufbuild/connect-go</summary>

###
[`v1.6.0`](https://togithub.com/bufbuild/connect-go/releases/tag/v1.6.0)

[Compare
Source](https://togithub.com/bufbuild/connect-go/compare/v1.5.2...v1.6.0)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### Enhancements

- Improve comments & add procedure consts to generated code by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/480](https://togithub.com/bufbuild/connect-go/pull/480)
- Reduce per-call URL parsing cost by
[@&#8203;mattrobenolt](https://togithub.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/467](https://togithub.com/bufbuild/connect-go/pull/467)
- Improve errors for outdated protobuf runtimes by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/465](https://togithub.com/bufbuild/connect-go/pull/465)
- Switch README to use `buf curl` by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/474](https://togithub.com/bufbuild/connect-go/pull/474)

##### Bugfixes

- Clarify purpose of handler_stream_test.go by
[@&#8203;Hirochon](https://togithub.com/Hirochon) in
[https://github.com/bufbuild/connect-go/pull/472](https://togithub.com/bufbuild/connect-go/pull/472)
- Make StreamType constants typed numerics by
[@&#8203;jhump](https://togithub.com/jhump) in
[https://github.com/bufbuild/connect-go/pull/486](https://togithub.com/bufbuild/connect-go/pull/486)
- Populate Spec and Peer in Client.CallServerStream by
[@&#8203;jhump](https://togithub.com/jhump) in
[https://github.com/bufbuild/connect-go/pull/487](https://togithub.com/bufbuild/connect-go/pull/487)

#### New Contributors

- [@&#8203;Hirochon](https://togithub.com/Hirochon) made their first
contribution in
[https://github.com/bufbuild/connect-go/pull/472](https://togithub.com/bufbuild/connect-go/pull/472)

**Full Changelog**:
bufbuild/connect-go@v1.5.2...v1.6.0

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

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

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/open-feature/flagd).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4zMS40IiwidXBkYXRlZEluVmVyIjoiMzUuMzEuNCJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
akshayjshah added a commit that referenced this pull request Jul 26, 2023
Clarify that the sole purpose of the test in `handler_stream_test.go` is
to verify a safety property of the `ClientStream` iterator. A similar
testing strategy isn't worthwhile for `ServerStream` or `BidiStream`,
because they're not iterators.

---------

Co-authored-by: Akshay Shah <[email protected]>
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.

2 participants