-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(policy): Reduce duplication in outbound policy API tests #13543
Conversation
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great simplification!
policy-test/src/outbound_api.rs
Outdated
@@ -418,7 +389,7 @@ pub fn assert_tls_backend_matches_parent( | |||
} | |||
} | |||
|
|||
assert_resource_meta(&backend.metadata, parent, port) | |||
//assert_resource_meta(&backend.metadata, parent, port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be reverted?
policy-test/src/outbound_api.rs
Outdated
@@ -464,7 +435,7 @@ pub fn assert_tcp_backend_matches_parent( | |||
} | |||
} | |||
|
|||
assert_resource_meta(&backend.metadata, parent, port) | |||
//assert_resource_meta(&backend.metadata, parent, port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes here
Signed-off-by: Alex Leong <alex@buoyant.io>
Good catch, @zaharidichev, those lines were actually in helper methods that are no longer used and can be removed entirely. |
This change refactors the outbound policy API integration tests to reduce duplication and to be more principled about what combinations of routes and parents are tested. The outbound policy API supports many route types (Linkerd HTTPRoute, gateway HTTPRoute, GRPCRoute, TCPRoute, and TLSRoute) and each of these routes may be attached to up to two parent types (Service and EgressNetwork). Many behaviors of the outbound policy API are shared between these route and parent types and in order to test these behaviors, many tests were duplicated for each combination of types. This resulted in a large amount of duplication and made it difficult to determine exactly which combinations were being tested and if any combinations were missed. It also required that the duplicated tests be kept in sync and allowed the possibility that the tests could accidentally drift apart over time resulting in inconsistent test coverage.
We introduce the TestRoute and TestParent traits which capture the common properties of routes and parents respectively and refactor the outbound policy API tests to be generic over these types and then explicitly invoke the test with the combinations of types to which the tested behavior applies. In cases where the tested behavior is specific to a particular type rather than being common to all types, a type specific test remains.