xdsclient: improve CDS watchers test #5693
Conversation
easwars
force-pushed
the
cds_watchers_test
branch
from
534eea9 to
68adeda
Compare
| // config_source specifier for the `lrs_server` field which is not set to | ||
| // `self`, and hence is expected to be NACKed by the client. |
There was a problem hiding this comment.
Nit: the lrs_server field isn't the thing being set to self. ConfigSourceSpecifier in the lrs_server field is not set to self.
There was a problem hiding this comment.
Isn't that what the comment says?
contains a config_source specifier for the `lrs_server` field which is not set to `self`
There was a problem hiding this comment.
config_source specifier this should have underscore in between source and specifier. Also, add "which" before "contains" earlier in this comment.
| if err := mgmtServer.Update(ctx, resources); err != nil { | ||
| t.Fatalf("Failed to update management server with resources: %v, err: %v", resources, err) | ||
| } | ||
| if err := verifyNoClusterUpdate(ctx, updateCh); err != nil { |
There was a problem hiding this comment.
Since we're not wrapping the error from this, rather than just saying unexpected update, can you explicitly state in the returned error (or wrap it) stating that we don't expect any update?
There was a problem hiding this comment.
Doesn't an unexpected update mean that you received an update when you did not expect one?
There was a problem hiding this comment.
To me, unexpected update also encompasses the case where you receive a bad update with fields not as expected. I feel like I've written many a if diff := cmp.Diff; diff != "" {t.Fatalf("Unexpected update (-got, +want) %v", diff} or whatever the exact syntax is. I feel strongly about this :D.
| } | ||
| if err := verifyNoClusterUpdate(ctx, updateCh1); err != nil { |
There was a problem hiding this comment.
Same comment as previously, please switch the returned error to explicitly state expecting no cluster update. This keeps it consistent to the got/want/diffs we all know and love.
There was a problem hiding this comment.
As stated in the previous comment about the same thing, unexpected update means the same as unexpected update when we don't want one. We use this pattern quite a lot in our tests. Like we don't have a want section here. We have received something when we dont expect to receive anything. So, I feel this is good enough. Let me know if you feel strongly about it.
| // config_source specifier for the `lrs_server` field which is not set to | ||
| // `self`, and hence is expected to be NACKed by the client. |
There was a problem hiding this comment.
config_source specifier this should have underscore in between source and specifier. Also, add "which" before "contains" earlier in this comment.
| if err := mgmtServer.Update(ctx, resources); err != nil { | ||
| t.Fatalf("Failed to update management server with resources: %v, err: %v", resources, err) | ||
| } | ||
| if err := verifyNoClusterUpdate(ctx, updateCh); err != nil { |
There was a problem hiding this comment.
To me, unexpected update also encompasses the case where you receive a bad update with fields not as expected. I feel like I've written many a if diff := cmp.Diff; diff != "" {t.Fatalf("Unexpected update (-got, +want) %v", diff} or whatever the exact syntax is. I feel strongly about this :D.
| // update from the management server containing both resources results in the | ||
| // invocation of all watch callbacks. | ||
| // | ||
| // The test is run with both old and new style names. |
There was a problem hiding this comment.
I'm suggesting changing docstring specifying you use old and new in same resource, rather than a different test. Just to keep explanations consistent with docstrings earlier explaining you have seperate tests for old and new style resources. Here and the other place I commented about this explicit stating of how it uses old and new style names.
| // TestCDSWatch_ResourceRemoved covers the cases where a resource being watched | ||
| // is removed from the management server. The test verifies the following | ||
| // scenarios: | ||
| // 1. Removing a resource should trigger the watch callback with a resource | ||
| // removed error. It should not trigger the watch callback for an unrelated | ||
| // resource. | ||
| // 2. An update to another resource should result in the invocation of the watch | ||
| // callback associated with that resource. It should not result in the | ||
| // invocation of the watch callback associated with the deleted resource. | ||
| // | ||
| // The test is run with both old and new style names. |
There was a problem hiding this comment.
Same as others, I'm suggesting changing docstring specifying you use old and new in same resource, rather than a different test. Just to keep explanations consistent with docstrings earlier.
| } | ||
| if err := verifyNoClusterUpdate(ctx, updateCh1); err != nil { |
| // update from the management server containing both resources results in the | ||
| // invocation of all watch callbacks. | ||
| // | ||
| // The test is run with both old and new style names. |
There was a problem hiding this comment.
Oh whoops lol on wrong partition I was thinking about.
easwars
force-pushed
the
cds_watchers_test
branch
from
d7578ee to
26ffceb
Compare
Improve the CDS 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