refactor(app/test): address hyper deprecations in test helpers #3433
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this was not being flagged as an unused variable, due to the `#[instrument]` attribute. (😉 _it's used as a field in the generated span!_) `connect_timeout(..)` doesn't use its parameter. to address some deprecations, and avoid the need for polymorphism / refactoring related to http/1 and http/2 connections being represented as distinct types in the hyper 1.0 api, we remove it. Signed-off-by: katelyn martin <kate@buoyant.io>
this addresses hyper 1.0 deprecations in the server side of the inbound proxy's http unit test suite logic. see <linkerd/linkerd2#8733> for more information. the client end of this change ends up being slightly involved, due to changes that will need to be made in `linkerd_app_test::http_util`. accordingly, those deprecations will be addressed in a subsequent commit. Signed-off-by: katelyn martin <kate@buoyant.io>
this moves the public `connect_client(..)` function up to the top of the group of functions, and adds a documentation comment explaining its purpose. a todo comment is left noting that this can be refactored to use tokio's new `JoinSet` type. Signed-off-by: katelyn martin <kate@buoyant.io>
this is a small tweak, defering the call to `tokio::spawn(..)` to make using `JoinSet` easier. Signed-off-by: katelyn martin <kate@buoyant.io>
this commit changes the `connect_and_accept(..)` helper function used in the proxy's unit tests, altering its logic such that it now runs background tasks inside of a `JoinSet`. Signed-off-by: katelyn martin <kate@buoyant.io>
this commit makes two minor changes to the `run_proxy()` helper function: * remove a frivolous `.map(|_| ())` on a result that already had a tuple `Ok` type. * use `ServiceExt::oneshot` for brevity. this should have no effect on the tests, we're just golfing things down a bit. Signed-off-by: katelyn martin <kate@buoyant.io>
Signed-off-by: katelyn martin <kate@buoyant.io>
we've nudged this along well enough that this function can now reasonably be removed. Signed-off-by: katelyn martin <kate@buoyant.io>
and once more, we remove a helper function that isn't doing quite so much work, and whose signature contains deprecated hyper 1.0 types. Signed-off-by: katelyn martin <kate@buoyant.io>
we create `async move {}` blocks in multiple places, without any clear
benefit.
this commit consolidates them into one place: when we spawn a task onto
the `JoinSet`.
Signed-off-by: katelyn martin <kate@buoyant.io>
cratelyn
force-pushed
the
kate/hyper-1.x-app-inbound-test-updates.part-2
branch
from
4a06bf8 to
ae3cca2
Compare
cratelyn
changed the title
JoinSet for background tasks
cratelyn
commented
Dec 7, 2024
There was a problem hiding this comment.
the diff in this file is much more easily reviewed on a commit by commit basis.
olix0r
approved these changes
Dec 9, 2024
| bg.join_all() | ||
| .await | ||
| .into_iter() | ||
| .collect::<Result<Vec<()>, Error>>() |
There was a problem hiding this comment.
If you want, you can use core::Result so this is just .collect::<Result<Vec<()>>>()
There was a problem hiding this comment.
i took a swing at this, but i think i'll opt to elide this.
i have a personal aversion to importing Result aliases that overwrite the Result<T, E> included in the standard prelude, and found that the alternate .collect::<linkerd_app_core::Result<Vec<()>>>() form didn't really golf this down much.
| #[allow(deprecated)] // linkerd/linkerd2#8733 | ||
| async fn connect_client( | ||
| pub async fn connect_and_accept( |
There was a problem hiding this comment.
can the above deprecation be removed yet?
There was a problem hiding this comment.
not quite yet, https://github.com/linkerd/linkerd2-proxy/pull/3433/files/ae3cca22f0219533bd3b8f59b81fb0f1e852ddcf#diff-8cdd5a92a26923db556253828b546a6741d66ed6e5be3ba2043b535d8e34695dR11 the ClientBuilder parameter to this function is still a deprecated connection builder type.
this is definitely next on the chopping block though 🗡️ 😈
Base automatically changed from
kate/hyper-1.x-app-inbound-test-updates.part-1
to
main
December 9, 2024 16:55
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this commit changes the
connect_and_accept(..)helper function used inthe proxy's unit tests, altering its logic such that it now runs
background tasks inside of a
JoinSet.then, subsequent commits hoist code out of
run_proxy()andconnect_client(), and consolidate ourasync move {}blocks. this both simplifies the control flow of this test infrastructure, but also addresses some more hyper deprecations.for more information about our progressing upgrade to hyper 1.0, see linkerd/linkerd2#8733.
NB: this branch is based upon #3432.