#7: Support for in-process transport for testing #44

andreilisa · 2024-11-01T15:20:24Z

This pull request introduces support for in-process transport for gRPC, facilitating efficient testing of gRPC services within the same process.

Signed-off-by: andreilisa <[email protected]>

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

vuhp · 2024-11-07T18:36:48Z

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

+	public static void shutdown() {
+		if (grpcChannel != null)
+			grpcChannel.shutdownNow();
+		if (grpcServer != null)
+			grpcServer.shutdownNow();
+	}


I believe users should not have to call to startup or should down from their test classes.
Instead, we can leverage a post-processor to handle the shutdown process.

I think this TestcontainersLifecycleApplicationContextInitializer is a good reference

Yes I am agree with you about this point, and also I think that an ApplicationListener on ContextClosedEvent achieves the purpose of not requiring users to manually handle the lifecycle, specifically shutdown.

…rt-testing

Signed-off-by: andreilisa <[email protected]>

dsyer · 2024-11-09T10:50:56Z

Thanks for the fixes. I'm still waiting for spring.grpc.inprocess to be spring.grpc.inprocess.enabled. And if it's in the json metadata, it should really be an Environment property, not just a System property? Is that a problem (it might be that the Environment is not initialized in time)?

I think we should have a sample test as well. The existing samples should now be enabling this by default? So we could slip one in there.

andreilisa · 2024-11-09T13:14:12Z

@dsyer I changed the property to spring.grpc.inprocess.enabled and for the metadata, I wasn’t entirely sure it was the best fit for this case, so I created a custom class that reads the property value directly from application.properties. If it’s not set, it defaults to true. This approach avoids potential issues with system properties and ensures that the environment is properly initialized before accessing the value. The tests I've written validate that these changes work as expected.

vuhp · 2024-11-09T16:04:54Z

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

+
+	@Override
+	public void initialize(ConfigurableApplicationContext applicationContext) {
+		GrpcProperties grpcProperties = new GrpcProperties(applicationContext.getEnvironment());


Personally I don't think class GrpcProperties is necessary

vuhp · 2024-11-09T16:07:41Z

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

+	@Override
+	public void initialize(ConfigurableApplicationContext applicationContext) {
+		GrpcProperties grpcProperties = new GrpcProperties(applicationContext.getEnvironment());
+		if (grpcProperties.isInProcessEnabled() && isJarOnClasspath()) {


We could check the reversed conditions and return early

vuhp · 2024-11-09T16:39:11Z

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

+		}
+	}
+
+	private boolean isJarOnClasspath() {


Consider re-use existing method ClassUtils.isPresent

vuhp · 2024-11-09T16:44:25Z

...c/test/java/org/springframework/grpc/sample/InProcessApplicationContextInitializerTests.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+
+class InProcessApplicationContextInitializerTests {


should this test class be in spring-grpc-test module instead?

It depends if the test jar depends on Spring Boot or not. I'm happy for it to be here for now anyway -it could also be a nested class inside the existing GrpcServerIntegrationTests (which I see below you already have, so maybe we don't need this one at all?).

dsyer · 2024-11-09T19:07:56Z

@dsyer I changed the property to spring.grpc.inprocess.enabled and for the metadata, I wasn’t entirely sure it was the best fit for this case, so I created a custom class that reads the property value directly from application.properties.

I’m not sure I like the sound of that. There’s nothing like it in the whole Spring Boot ecosystem as far as I know. Did you try it in a sample and it wasn’t available in a test?

andreilisa · 2024-11-11T07:03:01Z

@dsyer I changed the property to spring.grpc.inprocess.enabled and for the metadata, I wasn’t entirely sure it was the best fit for this case, so I created a custom class that reads the property value directly from application.properties.

I’m not sure I like the sound of that. There’s nothing like it in the whole Spring Boot ecosystem as far as I know. Did you try it in a sample and it wasn’t available in a test?

I haven't tried it in a sample yet, let me try it and I will do the changes(using metadata file for this property) to match the ecosystem pattern as well.

…rt-testing

Signed-off-by: andreilisa <[email protected]>

andreilisa · 2024-11-11T16:02:27Z

Hello, @dsyer can you please check one more time the changes ? Now the PR should look better.
About this point: "The existing samples should now be enabling this by default. So we could slip one in there."
I'm not sure how the test class should be structured because I have the property configured individually for each test.

dsyer · 2024-11-11T16:22:18Z

I added a couple more comments. If you could squash and rebase that will help as well, please.

andreilisa · 2024-11-11T16:26:16Z

I added a couple more comments. If you could squash and rebase that will help as well, please.

I cannot see your comments

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

dsyer · 2024-11-04T08:56:36Z

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

+/*
+ * @author Andrei Lisa
+ */
+public class InProcessApplicationContextInitializer


Is there a special reason this is an ApplicationContextInitializer? What is the expected usage? I was sort of hoping that a user would be able to easily write an integration test that uses the in-process channel instead of the tcp one. Doesn't that mean we should make it look as much like the existing GrpcServerLifecycle pattern?

I believe ApplicationContextInitializer is a better fit here, as it allows us to set up a simple in-process channel builder directly at application startup and ensures that it’s automatically shut down when the process ends. This approach keeps things lightweight and aligns well with the purpose of the in-process channel, as we don’t need the full lifecycle management that GrpcServerLifecycle provides.

I disagree, and I'm not really sure I want to merge this yet. It works up to a point, but it doesn't stop the tcp server starting, which feels odd to me (I was hoping for a drop in replacement). It's also an unusual way to implement this kind of feature, and I'm not convinced it won't create problems down the road. Looking at the test containers support was a good suggestion, so I'd like to understand what we are missing there before we do anything else here.

dsyer · 2024-11-11T16:18:43Z

...c/test/java/org/springframework/grpc/sample/InProcessApplicationContextInitializerTests.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+
+class InProcessApplicationContextInitializerTests {


It depends if the test jar depends on Spring Boot or not. I'm happy for it to be here for now anyway -it could also be a nested class inside the existing GrpcServerIntegrationTests (which I see below you already have, so maybe we don't need this one at all?).

dsyer · 2024-11-11T16:21:28Z

...c/test/java/org/springframework/grpc/sample/InProcessApplicationContextInitializerTests.java

+
+	@Nested
+	@SpringBootTest(properties = { "spring.grpc.server.host=0.0.0.0", "spring.grpc.server.port=0",
+			"spring.grpc.inprocess.enabled=true" })


This is the same test as WithInProcessEnabled? Probably we can just have one (and not set the property - just use the default).

dsyer · 2024-11-12T08:22:55Z

...test/src/main/java/org/springframework/grpc/test/InProcessApplicationContextInitializer.java

+			try {
+				String serverName = InProcessServerBuilder.generateName();
+
+				Server grpcServer = InProcessServerBuilder.forName(serverName).directExecutor().build().start();


We are starting this server without registering any services. This is a bug, and it should have had a test. Feels like maybe a sign that we would be better sticking more closely to the GrpcServerFactory pattern. I'm still open to not doing that, but there has to be a test, and the services have to get registered, at a minimum. What about interceptors? They probably need to get registered as well (maybe with exceptions for testing).

Yes, you are right and also I’m aware of the missing service registration and encountered the same issue in testing. I agree that this indicates a need to align more closely with the GrpcServerFactory pattern, especially for consistency in managing service and interceptor registration. At a minimum, I’ll ensure that services are properly registered and that we have a test to validate this.

@dsyer Can you please check the changes again ? II implemented a factory for the in-process channel that now ensures the correct registration of services, which resolves the bugs we encountered earlier.

@dsyer Hello, what do think about this PR ?

…rt-testing

Signed-off-by: andreilisa <[email protected]>

dsyer · 2024-11-19T15:06:59Z

Closing in favour of 2df6c37.

andreilisa mentioned this pull request Nov 1, 2024

Support for in-process transport for testing #7

Closed

spring-projects#7: Support for in-process transport for testing

296653e

Signed-off-by: andreilisa <[email protected]>

andreilisa force-pushed the 7-in-proccess-transport-testing branch from 855d267 to 296653e Compare November 1, 2024 15:42

vuhp reviewed Nov 7, 2024

View reviewed changes

andreilisa force-pushed the 7-in-proccess-transport-testing branch from ced3c2d to 296653e Compare November 9, 2024 10:18

andreilisa added 2 commits November 9, 2024 12:19

Merge remote-tracking branch 'origin/main' into 7-in-proccess-transpo…

f7f7979

…rt-testing

spring-projects#7: fix comments

f645b98

Signed-off-by: andreilisa <[email protected]>

andreilisa requested a review from vuhp November 9, 2024 15:42

andreilisa force-pushed the 7-in-proccess-transport-testing branch from a107df3 to cfb847c Compare November 9, 2024 15:43

vuhp reviewed Nov 9, 2024

View reviewed changes

andreilisa added 2 commits November 11, 2024 17:33

Merge remote-tracking branch 'origin/main' into 7-in-proccess-transpo…

dded6bd

…rt-testing

spring-projects#7: resolve comments

4493635

Signed-off-by: andreilisa <[email protected]>

andreilisa force-pushed the 7-in-proccess-transport-testing branch from cfb847c to 4493635 Compare November 11, 2024 15:58

dsyer requested changes Nov 11, 2024

View reviewed changes

dsyer reviewed Nov 12, 2024

View reviewed changes

andreilisa added 2 commits November 13, 2024 17:17

Merge remote-tracking branch 'origin/main' into 7-in-proccess-transpo…

91debdb

…rt-testing

spring-projects#7: use factory instead of ApplicationContextInitializer

957c320

Signed-off-by: andreilisa <[email protected]>

andreilisa requested review from dsyer and vuhp November 13, 2024 16:38

dsyer closed this Nov 19, 2024

#7: Support for in-process transport for testing #44

#7: Support for in-process transport for testing #44

Uh oh!

Conversation

andreilisa commented Nov 1, 2024

Uh oh!

Uh oh!

Uh oh!

vuhp Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreilisa Nov 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyer commented Nov 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreilisa commented Nov 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyer commented Nov 9, 2024

Uh oh!

andreilisa commented Nov 11, 2024

Uh oh!

andreilisa commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyer commented Nov 11, 2024

Uh oh!

andreilisa commented Nov 11, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreilisa Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyer commented Nov 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vuhp Nov 7, 2024 •

edited

Loading

andreilisa Nov 9, 2024 •

edited

Loading

dsyer commented Nov 9, 2024 •

edited

Loading

andreilisa commented Nov 11, 2024 •

edited

Loading

andreilisa Nov 12, 2024 •

edited

Loading