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

test: Cleanup outbound_balancer_waits_for_ready_endpoint #2529

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Nov 17, 2023

The outbound_balancer_waits_for_ready_endpoint test doesn't reliably demonstrate the behavior it is trying to test: we setup two identical endpoints, so our tests can't be sure that the response is coming from the correct endpoint.

This change updates this test to provide backends that return varied responses so we can ensure the desired behavior.

This change also improves the test's logging. The discovery test infrastructure is updated to allow overriding the logical service address. This aids disambiguating addresses in logs.

The outbound_balancer_waits_for_ready_endpoint test doesn't reliably
demonstrate the behavior it is trying to test: we setup two identical
endpoints, so our tests can't be sure that the response is coming from
the correct endpoint.

This change updates this test to provide backends that return varied
responses so we can ensure the desired behavior.

This change also improves the test's logging. The discovery test
infrastructure is updated to allow overriding the logical service
address. This aids disambiguating addresses in logs.
@olix0r olix0r requested a review from a team as a code owner November 17, 2023 20:14
@olix0r olix0r enabled auto-merge (squash) November 17, 2023 20:26
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me! i had one tiny suggestion

Comment on lines +484 to +493
assert_eq!(
String::from_utf8(
hyper::body::to_bytes(res.into_body())
.await
.unwrap()
.to_vec(),
)
.unwrap(),
"beta"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: there's a body_string function in the test utils that we could just use here:

Suggested change
assert_eq!(
String::from_utf8(
hyper::body::to_bytes(res.into_body())
.await
.unwrap()
.to_vec(),
)
.unwrap(),
"beta"
);
assert_eq!(http_util::body_string(res.into_body()).await, "beta");

@olix0r olix0r merged commit 9c56e91 into main Nov 20, 2023
10 checks passed
@olix0r olix0r deleted the ver/testo-disco branch November 20, 2023 21:57
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Nov 30, 2023
* Add a .codecov.yml (linkerd/linkerd2-proxy#2527)
* stack: Add BoxCloneSyncService (linkerd/linkerd2-proxy#2523)
* ci: Compute coverage over all tests (linkerd/linkerd2-proxy#2528)
* stack-metrics: Implement Clone for TrackService (linkerd/linkerd2-proxy#2524)
* ci: Fetch tarpaulin binary instead of compiling (linkerd/linkerd2-proxy#2532)
* ci: Enable coverage on main and all PRs (linkerd/linkerd2-proxy#2533)
* test: Cleanup consecutive_failures_accrue (linkerd/linkerd2-proxy#2531)
* test: Improve error reporting in gauges_endpoints (linkerd/linkerd2-proxy#2530)
* test: Cleanup outbound_balancer_waits_for_ready_endpoint (linkerd/linkerd2-proxy#2529)
* meshtls: Consolidate TLS ID verification (linkerd/linkerd2-proxy#2534)
* build(deps): bump DavidAnson/markdownlint-cli2-action (linkerd/linkerd2-proxy#2537)
* build(deps): bump tj-actions/changed-files from 40.1.1 to 40.2.0 (linkerd/linkerd2-proxy#2536)

Signed-off-by: Oliver Gould <[email protected]>
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Nov 30, 2023
* Add a .codecov.yml (linkerd/linkerd2-proxy#2527)
* stack: Add BoxCloneSyncService (linkerd/linkerd2-proxy#2523)
* ci: Compute coverage over all tests (linkerd/linkerd2-proxy#2528)
* stack-metrics: Implement Clone for TrackService (linkerd/linkerd2-proxy#2524)
* ci: Fetch tarpaulin binary instead of compiling (linkerd/linkerd2-proxy#2532)
* ci: Enable coverage on main and all PRs (linkerd/linkerd2-proxy#2533)
* test: Cleanup consecutive_failures_accrue (linkerd/linkerd2-proxy#2531)
* test: Improve error reporting in gauges_endpoints (linkerd/linkerd2-proxy#2530)
* test: Cleanup outbound_balancer_waits_for_ready_endpoint (linkerd/linkerd2-proxy#2529)
* meshtls: Consolidate TLS ID verification (linkerd/linkerd2-proxy#2534)
* build(deps): bump DavidAnson/markdownlint-cli2-action (linkerd/linkerd2-proxy#2537)
* build(deps): bump tj-actions/changed-files from 40.1.1 to 40.2.0 (linkerd/linkerd2-proxy#2536)

Signed-off-by: Oliver Gould <[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