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 vertx test using HttpTestBase.DEFAULT_HTTP_PORT #4991

Merged

Conversation

percyashu
Copy link
Contributor

Motivation: discussion

HttpTestBase.DEFAULT_HTTP_PORT could be loaded from a system property or be 8080 when there is no such system property. So it can be used instead of hard coding port 8080 in tests.

Conformance:

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

@percyashu
Copy link
Contributor Author

Hi @vietj and @tsegismont,
This is ready for review.

@percyashu percyashu force-pushed the add-configurable-test-server-port branch from a647d44 to fbea9f7 Compare December 4, 2023 08:45
@percyashu percyashu requested a review from vietj December 6, 2023 10:58
README.md Outdated Show resolved Hide resolved
@vietj
Copy link
Member

vietj commented Dec 6, 2023

we should also do the same for the HTTPS port DEFAULT_HTTPS_PORT in this PR.

@percyashu percyashu force-pushed the add-configurable-test-server-port branch from 8ebc68a to 546d65d Compare December 11, 2023 11:35
@percyashu percyashu force-pushed the add-configurable-test-server-port branch from 546d65d to be65e02 Compare December 11, 2023 12:10
@percyashu percyashu requested a review from vietj December 11, 2023 12:11
@percyashu
Copy link
Contributor Author

@vietj the changes requested have been applied and ready for review.

@vietj
Copy link
Member

vietj commented Dec 18, 2023

will have a look soon, can you prepare a backport PR for vert 4.x branch @percyashu ?

@vietj vietj modified the milestones: 4.5.2, 5.0.0 Dec 18, 2023
@percyashu
Copy link
Contributor Author

percyashu commented Dec 18, 2023

will have a look soon, can you prepare a backport PR for vert 4.x branch @percyashu ?

@vietj I don't quite understand how to do a backport PR but I am willing to prepare it with some explanation.

@vietj
Copy link
Member

vietj commented Dec 19, 2023

will have a look soon, can you prepare a backport PR for vert 4.x branch @percyashu ?

@vietj I don't quite understand how to do a backport PR but I am willing to prepare it with some explanation.

actually never mind, there is already a pull request there.

@vietj
Copy link
Member

vietj commented Dec 19, 2023

as said in the other PR, can you just revert the tests that actually tests objects (like HostAndPortTest) that do not use a real server ? that is preferable.

@vietj
Copy link
Member

vietj commented Dec 19, 2023

thanks, I will merge that tomorrow

@vietj vietj merged commit efa5b84 into eclipse-vertx:master Dec 20, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants