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

xdsclient: improve authority watchers test #5700

Merged
merged 2 commits into from
Nov 4, 2022

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Oct 7, 2022

Improve authority watchers test by moving to an e2e style test which verifies functionality without relying on any internal state.

This PR is similar to how the LDS watchers test was cleaned up in #5506.

#resource-agnostic-xdsclient-api

RELEASE NOTES: n/a

@easwars
Copy link
Contributor Author

easwars commented Oct 27, 2022

Ping @dfawley

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM. I'm a bit concerned about the classification of these as "end to end tests", since they don't actually test gRPC servers or clients, or anything we expose externally to the user for that matter. These are really system level tests for the xdsclient package, right? (And as such, probably belong under the xdsclient directory somewhere instead, as they were before.)

@dfawley dfawley assigned easwars and unassigned dfawley Nov 3, 2022
@easwars
Copy link
Contributor Author

easwars commented Nov 3, 2022

LGTM. I'm a bit concerned about the classification of these as "end to end tests", since they don't actually test gRPC servers or clients, or anything we expose externally to the user for that matter. These are really system level tests for the xdsclient package, right? (And as such, probably belong under the xdsclient directory somewhere instead, as they were before.)

These are e2e tests from the point of view of the xds client, since it uses only exported APIs (and creates a new client and tests the scenarios being tested). The main reason I want these tests in a separate package is because our previous tests lived in the same package and ended up relying too much on internal state and being over-specified. I could have put them in the same directory, but in an xdsclient_test package (but I thought being in a separate directory helps a little since there is a lot of code in this directory).

@easwars easwars merged commit b597a8e into grpc:master Nov 4, 2022
@easwars easwars deleted the authority_tests branch November 4, 2022 17:59
@dfawley
Copy link
Member

dfawley commented Nov 4, 2022

That's fine, but I do think a package named e2e should be reserved for gRPC end to end tests, not tests done at the API level for a single package. Using a separate directory is OK, but I think it should be a different one, ideally under the xdsclient directory, e.g. xdsclient/test.

@easwars
Copy link
Contributor Author

easwars commented Nov 4, 2022

Fair point. I can change that once the last test PR (in this round) is in.

jronak pushed a commit to jronak/grpc-go that referenced this pull request Nov 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants