xds/client: cleanup Dump to remove unnecessary version field #4978
Conversation
xds/internal/xdsclient/dump_test.go
Outdated
| @@ -16,7 +16,7 @@ | |||
| * | |||
| */ | |||
|
|
|||
| package xdsclient_test | |||
| package xdsclient | |||
There was a problem hiding this comment.
Cleanup.
I believe it was put in a different package because of a circular dependency. That's resolved by the xdsclient refactoring.
There was a problem hiding this comment.
Oh, and, this will need to access some unexported testing helper functions, in a future change.
There was a problem hiding this comment.
I believe it was put in a different package because of a circular dependency
Maybe so, but IMO all our tests should be in a separate package unless they are unit tests of unexported functions or need to access internals of the package being tested (which we should be trying to minimize). This forces the tests to be written at the API layer, which is more representative of actual usage.
Can we leave this as xdsclient_test, and add the things that access unexported fields to a different test file?
There was a problem hiding this comment.
all our tests should be in a separate package
I think this is an overkill. There's a trade-off between unit tests and e2e tests. The complexity added by this requirement is not worth the effort.
Accessing unexported fields doesn't mean to check the field values. It can be to use the unexport update handler to send an update.
For example, in xdsclient, the way to receive an update is to have a real control plane, and use that to send an xDS resp. (There are currently exported functions on the xdsclient to trigger updates, but I think that's not in the right place, and will be unexported)
Requiring a control plane for all the watcher tests is too much work. Or you are arguing that we should move most (all?) the xdsclient tests to e2e.
I changed this back to _test. It's not required in this PR.
There was a problem hiding this comment.
I think this is an overkill.
It's overkill to say it must be done that way, but not that we should try to do it that way as much as is reasonable.
Obviously if you need to mock out something to test it more easily (e.g. time things), then you'll put your tests in the same package.
you are arguing that we should move most (all?) the xdsclient tests to e2e.
I didn't mean all our tests need to be at the gRPC API level, but at the API level of the package being tested. A fake management server might be too difficult to be worth doing (maybe?), but we definitely don't want every test of "update XYZ happened, what did the xdsclient see?" to be done all the way up at the E2E/application level.
Version is not used by anyone (not even in the CSDS responses).
And version doesn't make sense in federation (when there are multiple control plane servers)
RELEASE NOTES: N/A