refactor: Pull code to compute location into one function#1094
refactor: Pull code to compute location into one function#1094danieljbruce merged 22 commits intogoogleapis:mainfrom
Conversation
…into autoscaler-refactor-tech-debt
…nieljbruce/nodejs-bigtable into autoscaler-refactor-gather-location # Conflicts: # src/utils/cluster.ts
bcoe
left a comment
There was a problem hiding this comment.
If I understand correctly, this is a cleanup and refactor of existing behaviour, vs., a new feature?
I would title the PR in this case:
refactor: pull code to compute location into one function.
I like this cleanup 🥳 , but left a couple thoughts.
|
|
||
| it('should respect the clusters option', done => { | ||
| const fakeLocation = 'a/b/c/d'; | ||
| const fakeLocation = Cluster.getLocation_( |
There was a problem hiding this comment.
I'm surprised that tests had to be updated, since it looks like this is just a refactor. Does the value of fakeLocation actually matter?
It may be better leave the existing tests as is, since it makes it more clear that this is simply a refactor.
There was a problem hiding this comment.
Unfortunately, fakeLocation must be changed in this test and the reason is that we mock out getLocation_ in fake cluster, but now getLocation_ is called in a ClusterUtils objects which uses the real cluster. What should we do? We could do one of the following:
- Change the test as I have done. OR
- Move this functionality into a private method of cluster instead so that the test does not need to be changed.
There was a problem hiding this comment.
It looks like we decided just to get rid of the mocks that weren't being used.
| return updateMask; | ||
| } | ||
|
|
||
| static getClusterBaseConfigWithFullLocation( |
There was a problem hiding this comment.
I think we should try to add a test that specifically asserts against this expected behaviour:
it('populates full location path if location parameter provided')
What format is it expected that metadata.location is in? Is this value provided by a user.
There was a problem hiding this comment.
Do you mean add a test that calls getClusterBaseConfigWithFullLocation and then compares the output against a predetermined value?
There was a problem hiding this comment.
I would be tempted to add a test for the user behavior that would cause these different code paths to be hit.
One test for location being undefined, one test for location being set.
There was a problem hiding this comment.
The test for location is here and I added an assertion:
https://github.com/googleapis/nodejs-bigtable/blob/release-v4.0.0/test/instance.ts#L338
We can't add a test for an undefined location because the interface requires location to be defined. I believe that if location is not provided then the server will throw an error.
export interface BasicClusterConfig {
encryption?: google.bigtable.admin.v2.Cluster.IEncryptionConfig;
key?: string;
location: string;
nodes?: number;
storage?: string;
minServeNodes?: number;
maxServeNodes?: number;
cpuUtilizationPercent?: number;
}
Keep in mind that a lot of the autogenerated tests from earlier make sure these refactors don't change functionality: https://github.com/googleapis/nodejs-bigtable/pull/1092/files#diff-9db6b3eefb5be3d2bc20f1be7f6d14141dbc06e562bd45daabd86b2f86bf3848R40 for example
We could change the interface of the user facing functions to make location optional and test for the error, but then we are expanding the API surface.
This pull request is dependent on #1093. After that PR and its predecessor are merged this one will just be a PR where we pull repeated functionality from two functions into a single utility function.