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

Configurable server port for the tests #2513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

percyashu
Copy link

Motivation: #465

Run test with port if it is specified e.g mvn test -Dport=8888.

Conformance:

Your commits should be signed and you should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

@vietj
Copy link
Contributor

vietj commented Nov 17, 2023

I think instead we should use the constant defined in vertx-core : HttpTestBase#DEFAULT_HTTP_PORT and make the vertx-core constant overridable with a system property

@percyashu
Copy link
Author

@vietj I don't quite understand. Can you elaborate further?

@tsegismont
Copy link
Contributor

@vietj let me give you some context. @percyashu is a former GSoC candidate, who is willing to contribute to Vert.x Web. We have talked off-list and he picked #465 which is marked wit the help wanted label.

As discussed with @pmlopes in the ticket, the idea is to use random ports for tests but, for debugging purposes, rely on a sysprop to determine if a fix port should be used instead. All in all, this is comparable to the behavior of sql client tests.

The motivation is to reduce the number of intermittent failures, by removing those due to servers being already bound to port 8080.

@vietj
Copy link
Contributor

vietj commented Nov 21, 2023

@percyashu in vertx-core tests we do have a constant DEFAULT_HTTP_PORT = 8080 (https://github.com/eclipse-vertx/vert.x/blob/d2fcda6283f327f505a79ed187afe7752ec75187/src/test/java/io/vertx/core/http/HttpTestBase.java#L37)

This constant could be loaded from a system property (e.g. vertx.defaultHttpPort) or be 8080 when there is no such system property, and Vertx-web could reuse this property. That would allow vertx-core which is already using this property to be configured with a different port for testing.

@percyashu
Copy link
Author

Ok @vietj,
From my understanding this will involve first making the change to HttpTestBase, then extending it in the baseTest classes like WebTestBase or SockJSTestBase instead of directly extending VertxTestBase right?

@vietj
Copy link
Contributor

vietj commented Nov 27, 2023 via email

@vietj
Copy link
Contributor

vietj commented Dec 20, 2023

thanks for the contribution @percyashu

@percyashu
Copy link
Author

@vietj and @tsegismont the next steps will be to use HttpTestBase.DEFAULT_HTTP_PORT in this PR right?

@vietj
Copy link
Contributor

vietj commented Dec 21, 2023 via email

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

Successfully merging this pull request may close these issues.

3 participants