refactor: Pull code to compute location into one function

[danieljbruce]

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.

[bcoe]

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.

[bcoe]

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.

[danieljbruce]

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:

1. Change the test as I have done. OR
2. Move this functionality into a private method of cluster instead so that the test does not need to be changed.

[danieljbruce]

It looks like we decided just to get rid of the mocks that weren't being used.

[bcoe]

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.

[danieljbruce]

Do you mean add a test that calls getClusterBaseConfigWithFullLocation and then compares the output against a predetermined value?

[bcoe]

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.

[danieljbruce]

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.