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.

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

Improve testing gRPC services with random ports #34135

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

nahguam
Copy link
Contributor

@nahguam nahguam commented Jun 19, 2023

Currently, testing gRPC is not as seamless as HTTP when it comes to using random ports. If one injects a @GrpcClient when quarkus.grpc.server.test-port=0, it will attempt to connect to port 0, or if one is using the Vert.x server, it will attempt to connect to the http test port, 8081.

This change aims to improve this by making it possible to inject @GrpcClients into tests that have discovered the actual port that the service is running on. It shares the approach that is used for HTTP.

@nahguam nahguam force-pushed the grpc-random-port branch from 4f416b3 to f48d842 Compare June 19, 2023 14:35
@geoand geoand requested review from alesj and cescoffier June 19, 2023 14:45
@jbonofre
Copy link

@nahguam Thanks ! I'm doing a pass/review.

@quarkus-bot

This comment has been minimized.

@nahguam nahguam force-pushed the grpc-random-port branch from f48d842 to e39b864 Compare June 20, 2023 08:35
@alesj
Copy link
Contributor

alesj commented Jun 20, 2023

Where in the docs does it state if you pass 0 port to (Vert.x) gRPC server, that it randomly selects one?
(not that I'm doubting this is true ... just curious)

Plus, it would also be good/nice to fix to actual port where ever we print it out.
e.g.

            msg += String.format("gRPC server on %s:%d [%s]",
                    configuration.host, port, "TLS enabled: " + !configuration.plainText);

Otherwise it looks good + nice test coverage!

@nahguam
Copy link
Contributor Author

nahguam commented Jun 20, 2023

Where in the docs does it state if you pass 0 port to (Vert.x) gRPC server, that it randomly selects one? (not that I'm doubting this is true ... just curious)

To my limited knowledge, the only reference is https://github.com/quarkusio/quarkus/blob/main/docs/src/main/asciidoc/http-reference.adoc#7-listening-on-a-random-port

Should I add similar detail to the grpc doc?

Plus, it would also be good/nice to fix to actual port where ever we print it out. e.g.

            msg += String.format("gRPC server on %s:%d [%s]",
                    configuration.host, port, "TLS enabled: " + !configuration.plainText);

Indeed, I did have a look at this but it looked non-trivial. I'll take another look.

@nahguam nahguam force-pushed the grpc-random-port branch from e39b864 to a060e76 Compare June 20, 2023 13:54
@nahguam
Copy link
Contributor Author

nahguam commented Jun 20, 2023

Plus, it would also be good/nice to fix to actual port where ever we print it out. e.g.

            msg += String.format("gRPC server on %s:%d [%s]",
                    configuration.host, port, "TLS enabled: " + !configuration.plainText);

Indeed, I did have a look at this but it looked non-trivial. I'll take another look.

I've updated this output.

@alesj
Copy link
Contributor

alesj commented Jun 20, 2023

Should I add similar detail to the grpc doc?

Are we sure this works for the gRPC as well? :-)
Since I guess that is then a (pure) gRPC thing (not http / Vert.x)

@@ -226,7 +228,7 @@ private void prodStart(GrpcContainer grpcContainer, Vertx vertx, GrpcServerConfi

private void postStartup(GrpcServerConfiguration configuration, GrpcBuilderProvider<?> provider, boolean test) {
initHealthStorage();
int port = test ? configuration.testPort : configuration.port;
int port = test ? testPort(configuration) : configuration.port;
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this was easy. :-)
Are we sure we captured all such log outputs -- with this "new" port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an output in buildServer but that's before the server is started - so before the port is chosen.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM!

You can retrieve the actual port from the httpServer (once bound to the socket): `server.getactualPort()).

Copy link

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@nahguam nahguam force-pushed the grpc-random-port branch from a060e76 to ea149e1 Compare June 20, 2023 16:06
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 20, 2023

Failing Jobs - Building ea149e1

Status Name Step Failures Logs Raw logs
Native Tests - Misc3 Build ⚠️ Check → Logs Raw logs
Native Tests - Windows - RESTEasy Jackson Setup GraalVM ⚠️ Check → Logs Raw logs

@nahguam
Copy link
Contributor Author

nahguam commented Jun 21, 2023

Should I add similar detail to the grpc doc?

Are we sure this works for the gRPC as well? :-) Since I guess that is then a (pure) gRPC thing (not http / Vert.x)

Do you mean when use-separate-server=true? If so, yes, the tests cover that.

@nahguam
Copy link
Contributor Author

nahguam commented Jun 21, 2023

Do I need to keep rebasing on main? (it moves so quickly! :D)

Looks like the failure is unrelated. What needs to happen now?

@jbonofre
Copy link

@nahguam Clément and I approved this PR, so I think we are good to go. No need to keep rebasing, we can alway rebase when merging. @cescoffier thoughts ?

@cescoffier cescoffier merged commit be00963 into quarkusio:main Jun 21, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jun 21, 2023
@cescoffier
Copy link
Member

I was just waiting for 3.2 to be branched.

@jbonofre
Copy link

@cescoffier great, thank you 😄

@nahguam
Copy link
Contributor Author

nahguam commented Jun 21, 2023

Great Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants