Refactor unit tests for Remote Config #884
Conversation
lahirumaramba
force-pushed
the
lm-rc-rollback
branch
from
1ec5d13
to
2fb95eb
Compare
| @@ -240,6 +191,14 @@ describe('RemoteConfigApiClient', () => { | |||
| .should.eventually.be.rejectedWith(noProjectId); | |||
| }); | |||
|
|
|||
| // tests for input template validations | |||
| testInvalidInputTemplates((t: RemoteConfigTemplate) => { apiClient.validateTemplate(t); }); | |||
There was a problem hiding this comment.
Type annotation not needed here. Let the compiler infer the type whenever possible.
There was a problem hiding this comment.
The compiler complains if I don't annotate the type here. I could remove the return types though.
remote-config-api-client.spec.ts (193,32): Parameter 't' implicitly has an 'any' type. (7006)
| testInvalidInputTemplates((t: RemoteConfigTemplate) => { apiClient.validateTemplate(t); }); | ||
|
|
||
| // tests for api response validations | ||
| runApiResponseValidationTests((): Promise<RemoteConfigTemplate> => { |
There was a problem hiding this comment.
Here too. Just () => apiClient.validateTemplate(...)
| }); | ||
| }); | ||
|
|
||
| function runApiResponseValidationTests(rcOperation: () => Promise<RemoteConfigTemplate>): void { |
There was a problem hiding this comment.
Most of this is checking for various error responses. So may be runErrorResponseTests() is more appropriate? Your call.
| }); | ||
| } | ||
|
|
||
| function runTemplateVersionNumberTests(rcOperation: Function): void { |
There was a problem hiding this comment.
Don't see why this needs to be a function. If you're planning for future reuse, I'd rather refactor this into a function at that point.
There was a problem hiding this comment.
Yeah, I am planing to use in the next PR. I will introduce the function at that point then :)
| 'message', `Remote Config conditions must be an array`); | ||
| const invalidEtags = [...INVALID_STRINGS]; | ||
| let sourceTemplate = deepCopy(REMOTE_CONFIG_RESPONSE); | ||
| invalidEtags.forEach((invalidEtag) => { |
There was a problem hiding this comment.
INVALID_STRINGS.forEach(...
| .resolves(response); | ||
| stubs.push(stub); | ||
| return remoteConfig.publishTemplate(REMOTE_CONFIG_TEMPLATE) | ||
| return rcOperation() | ||
| .should.eventually.be.rejected.and.have.property( | ||
| 'message', `Remote Config conditions must be an array`); | ||
| }); | ||
|
|
||
| it('should resolve with parameters:{} when no parameters present in the response', () => { |
There was a problem hiding this comment.
This and the subsequent resolves tests should not be in the runApiResponseValidationTests IMO. May be put them in a separate function?
There was a problem hiding this comment.
Good point. Moved to a new function runValidResponseTests. Renamed runApiResponseValidationTests to runInvalidResponseTests.
* Get remote config template by version number (#874) * Get remote config template by version number * Refactor unit tests for Remote Config (#884) * Refactor unit tests * Add Remote Config Rollback operation (#885) * Add Remote Config Rollback operation * Update docs and move etag validation to a helper function * Update docs * Introduce a util to create a template from API response * PR fixes * Remote Config Add list versions operation (#896) * Remote Config: Add list versions operation * Add version Impl and other PR fixes * PR fixes * Imrpoved unit tests and some code clean up * Fix code formatting * Add a separate function to get a Template with version (#902) * Add version meta data to RC templates (#906) * Add version meta data to RC templates * PR fixes * Use assertion to unwrap template.version * Update Remote Config Docstrings in index.d.ts
Our unit tests for RC are quite repetitive. As we are adding more features to the RC API, it is time to clean up the code a bit.
remote-config.spec.tstoremote-config-api-client.spec.tsas that is where the validation really takes place.