Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: change to generation script in manipulating googleapis/WORKSPACE. #3120

Merged
merged 5 commits into from
Aug 14, 2024
Merged

fix: change to generation script in manipulating googleapis/WORKSPACE. #3120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a906a99
fix: change to generation script in manipulating googleapis/WORKSPACE.
zhumin8 Aug 14, 2024
ab7fe87
fix: mocked method in test needs to change after java.time introduced…
zhumin8 Aug 14, 2024
f230fad
fix: address review comment, create a custom InstantiatingGrpcChannel…
zhumin8 Aug 14, 2024
eebd82b
chore: use mvn wrapper in shocase test ci.
zhumin8 Aug 14, 2024
ad412f6
address review comments.
zhumin8 Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/showcaseTests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,6 @@ jobs:
bash scripts/generate-showcase.sh
- name: Unit tests for showcase-spring-starter
# Runs showcase-spring-starter unit tests
working-directory: spring-cloud-generator
working-directory: spring-cloud-generator/showcase/showcase-spring-starter
run: |
cd showcase/showcase-spring-starter && mvn verify
../../../mvnw verify
2 changes: 1 addition & 1 deletion spring-cloud-generator/scripts/generate-steps.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ function modify_workspace_file(){
# delete existing maven_install rules
buildozer 'delete' ${path_to_workspace}:%maven_install
# add custom maven_install rules
perl -pi -e "s{(^_gapic_generator_java_version[^\n]*)}{\$1\n$(cat ${path_to_modification_string})}" ${path_to_workspace}
perl -pi -e "s{(^api_dependencies()[^\n]*)}{\$1\n$(cat ${path_to_modification_string})}" ${path_to_workspace}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq, is this adding the bazel rule to the lines after api_dependencies()?

}

# For individual library, set up bazel rules
Expand Down
9 changes: 6 additions & 3 deletions spring-cloud-generator/scripts/resources/googleapis_modification_string.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ _spring_cloud_generator_version = "5.5.1-SNAPSHOT" # {x-version-update:spring-cl

maven_install(
artifacts = [
"com.google.cloud:spring-cloud-generator:" + _spring_cloud_generator_version,
],
"com.google.cloud:spring-cloud-generator:" + _spring_cloud_generator_version,
] +
IO_GRPC_GRPC_JAVA_ARTIFACTS,
generate_compat_repositories = True,
override_targets = IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS,
#Update this False for local development
fail_on_missing_checksum = False,
repositories = [
"m2Local",
"https://repo.maven.apache.org/maven2/",
]
)
)
21 changes: 6 additions & 15 deletions ...-starter/src/test/java/com/google/showcase/v1beta1/spring/EchoAutoConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@
package com.google.showcase.v1beta1.spring;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;

import com.google.api.gax.core.CredentialsProvider;
import com.google.api.gax.core.InstantiatingExecutorProvider;
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.rpc.ApiCallContext;
import com.google.api.gax.rpc.TransportChannel;
Expand Down Expand Up @@ -189,27 +188,19 @@ void testExecutorThreadCountFromProperties() {

@Test
void testCustomTransportChannelProviderUsedWhenProvided() throws IOException {
when(mockTransportChannelProvider.getTransportName()).thenReturn("grpc");
when(mockTransportChannelProvider.getTransportChannel()).thenReturn(mockTransportChannel);
when(mockTransportChannel.getEmptyCallContext()).thenReturn(mockApiCallContext);
// Mock no-ops for ApiCallContext since this test only intends to verify override of
// TransportChannelProvider bean
when(mockApiCallContext.withCredentials(any())).thenReturn(mockApiCallContext);
when(mockApiCallContext.withTransportChannel(any())).thenReturn(mockApiCallContext);
when(mockApiCallContext.withStreamWaitTimeout(any())).thenReturn(mockApiCallContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there going to be errors if we don't mock the behaviors of ApiCallContext? I guess somewhere in the client initialization logic these lines are called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, removing will cause errors (the same ones we observe in the ci before this fix).
However, for the purpose of this test, instead of creating the mockTransportChannelProvider, I think it is also okay to create a real channelProvider and test it is actually used. The benefit is we free ourselves from implementation detail changes in the future.

E.g.

      InstantiatingGrpcChannelProvider channelProvider = InstantiatingGrpcChannelProvider
              .newBuilder()
              .setEndpoint("localhost:7469")
              .setMaxInboundMessageSize(Integer.MAX_VALUE)
              .build();

when(mockApiCallContext.withStreamIdleTimeout(any())).thenReturn(mockApiCallContext);
when(mockApiCallContext.withEndpointContext(any())).thenReturn(mockApiCallContext);

InstantiatingGrpcChannelProvider channelProvider =
InstantiatingGrpcChannelProvider.newBuilder()
.build();
contextRunner
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To @lqiu96 's concerns about resource closing from offline discussion:
Yes, createChannel() is actually called in this test. But Spring's ApplicationContextRunner (contextRunner in this test) is actually responsible of managing and closing all beans and data sources it manages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, gotcha. Thanks for checking!

.withBean(
TRANSPORT_CHANNEL_PROVIDER_QUALIFIER_NAME,
TransportChannelProvider.class,
() -> mockTransportChannelProvider)
() -> channelProvider)
.run(
ctx -> {
EchoClient client = ctx.getBean(EchoClient.class);
assertThat(client.getSettings().getTransportChannelProvider())
.isSameAs(mockTransportChannelProvider);
.isSameAs(channelProvider);
});
}

Expand Down
Loading