test(codegen): add unit tests for one generated starter module - #1355
Conversation
meltsufin
left a comment
meltsufin
left a comment
There was a problem hiding this comment.
I assume you intend these tests to be autogenerated. If that being the case, I'm curious what @blakeli0 and @burkedavison think about that because we've recently had discussions about pros and cons of auto-generating unit tests.
| this.contextRunner.run( | ||
| ctx -> { | ||
| LanguageServiceClient client = ctx.getBean(LanguageServiceClient.class); | ||
| assertThat(client.getSettings().getCredentialsProvider()).isNotNull(); |
There was a problem hiding this comment.
Wouldn't this be true even when properties were provided, like in the test above?
There was a problem hiding this comment.
Right - the intention I had for this test is to ensure that when no service-specific properties are provided, the configuration picks up the CredentialsProvider bean from spring-cloud-gcp-autoconfigure (as introduced in #1123). I’ve updated the test to better reflect this scenario.
| } | ||
|
|
||
| @Test | ||
| void testShouldUseDefaultGrpcTransport() { |
There was a problem hiding this comment.
Is there a test for the opposite, or we only allow grpc at this point?
There was a problem hiding this comment.
We are only generating for grpc client libraries in the initial list. But on a second thought, this test could have been set up in a better way. I've update it to check instead that the client library default TransportChannelProvider is used when no overrides are present.
@meltsufin Thank you for the review! I'll follow-up on the in-line comments individually but just wanted to quickly respond to this note - these are intended to be manually-written tests (for one example module) as an initial testing strategy we've decided for spring codegen, given the discussion on moving away from autogenerated unit tests as you've mentioned. (We’ve also considered the showcase module for unit testing, but decided to go with an example module for now given we are testing an integration layer and not for any client library functionalities that showcase provides power of coverage for.) |
I agree this is consistent with our general strategy. Handwritten unit tests should continue to be implemented whenever appropriate. Autogenerated unit tests should not. |
Task derived from GoogleCloudPlatform/spring-cloud-gcp#1355 (comment). Removing `spring.auto` from properties prefix.
| } | ||
|
|
||
| @Test | ||
| void testShouldUseDefaultTransportChannelProvider() { |
There was a problem hiding this comment.
For the sake of coverage, you might also want to add a testCustomTransportChannelProviderSetToRest?
when property "com.google.cloud.language.v1.language-service.useRest=true" present,
we should expect the transportChannelProvider set to
LanguageServiceSettings.defaultHttpJsonTransportProviderBuilder().build();
There was a problem hiding this comment.
Thank you for this suggestion and added! I think it also addresses what I missed earlier in #1355 (comment).
|
Kudos, SonarCloud Quality Gate passed!
|
emmileaf
changed the title
…ifier name. (#1418) This is a followup on #1355, the additional test verifies in case of multiple bean of type `TransportChannelProvider` the one with correct qualifier name is used. related fix in generator: googleapis/sdk-platform-java#1207
4 participants
This PR:
LanguageAutoConfigurationTests.java(and resource files), which contains handwritten unit tests (adapted from the POC) for the corresponding generatedgoogle-cloud-language-spring-startermodule.Notes for review:
google-cloud-language-spring-startermodule, which I could also temporarily commit if it makes reviewing easier?Current test setup depends on the following changes in generator code: