search

spring-projects-experimental / spring-grpc

90 stars 19 forks source link

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

Closed andreilisa closed 1 week ago

andreilisa commented 3 weeks ago

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

dsyer commented 2 weeks ago

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 commented 2 weeks ago

@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.

dsyer commented 2 weeks ago

@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 commented 2 weeks ago

@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.

andreilisa commented 2 weeks ago

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 commented 2 weeks ago

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

andreilisa commented 2 weeks ago

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

I cannot see your comments

dsyer commented 1 week ago

Closing in favour of 2df6c37.