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 alloc tests for alternative stream creation API #451

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

gjcairo
Copy link
Contributor

@gjcairo gjcairo commented Jul 15, 2024

Allocation tests currently use a func that takes a promise to create the stream multiplexer.
However, we also provide a different API that doesn't require a promise and instead returns an EventLoopFuture<Channel>.
We don't currently have allocation tests for the latter, so this PR duplicates the current H2 tests but uses the non-promise API to create the multiplexer.
I also had to add a new method on the old H2 multiplexer to conform to a test protocol.

@gjcairo gjcairo requested a review from glbrntt July 15, 2024 16:27
@gjcairo gjcairo marked this pull request as ready for review July 15, 2024 16:27
@gjcairo gjcairo requested a review from glbrntt July 16, 2024 10:11
Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Instead of adding more of those "old" tests can we please adopt the new benchmarking. We have to migrate over anyways so we should use chances like this to start the move.

@glbrntt
Copy link
Contributor

glbrntt commented Jul 16, 2024

Instead of adding more of those "old" tests can we please adopt the new benchmarking. We have to migrate over anyways so we should use chances like this to start the move.

I generally agree but in this instance we're just modifying an existing test so the overhead is low.

I think it makes more sense to get the scaffolding for new benchmarks done across all of our repos in one go and then migrate the tests over time. Bootstrapping the setup is always more painful than you think it's going to be...

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Jul 16, 2024
@glbrntt glbrntt merged commit aab0f12 into apple:main Jul 16, 2024
6 of 7 checks passed
@gjcairo gjcairo deleted the alloc-tests branch July 16, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants