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 #465

Open
brunoais opened this issue Oct 7, 2016 · 18 comments
Open

Configurable server port for the tests #465

brunoais opened this issue Oct 7, 2016 · 18 comments

Comments

@brunoais
Copy link
Contributor

brunoais commented Oct 7, 2016

Currently

On my PC, port 8080 is permanently in use for a personal server.
Currently, every time I want to test I have either to change the port on multiple places or turn off my server so that vert.x can bind to a port.

Request

May one change vert.x's testing framework so it may read (maybe in a properties file?) the port vert.x should listen to?
If so, how do you suggest?

@pmlopes
Copy link
Member

pmlopes commented Oct 7, 2016

Hummm that will be a hard one to solve, you need to review all tests... Or run tests inside docker...

@brunoais
Copy link
Contributor Author

brunoais commented Oct 7, 2016

Why would that be?

@pmlopes
Copy link
Member

pmlopes commented Oct 8, 2016

because the port is not configurable (at the moment) so you would need to look into all super classes that the tests extend and make it configurable there. Anyway it should be some work but i guess it won't be a paramount.

@cescoffier
Copy link
Member

Maybe I don't understand, but why are you not using the port 0 or the method explained in http://vertx.io/blog/vert-x-application-configuration/

@brunoais
Copy link
Contributor Author

brunoais commented Oct 8, 2016

I don't know.

@brunoais
Copy link
Contributor Author

brunoais commented Oct 8, 2016

@pmlopes We could use automatic ports given by ServerSocket. Looks correct to me...

@pmlopes
Copy link
Member

pmlopes commented Oct 8, 2016

we're not using port 0 because that feature got introduced with 3.2 if i'm not mistaken and vertx-web tests predate that. Maybe we should refactor the tests to do so and probably use vertx-unit instead of the blocking api we currently have. But that my friends is a huge refactor, not complicated but time consuming :)

@cescoffier
Copy link
Member

Yes, you are right, it was introduced in vert.x 3.2

@vietj
Copy link
Contributor

vietj commented Oct 9, 2016

keep in mind that using a specific port can be quite useful when you want to use wireshark to see what's happening on the wire

@brunoais
Copy link
Contributor Author

brunoais commented Nov 9, 2016

I've been dwelling on this from time to time and this is what I came up with:

  • Add a System property that is read when the tests are started.
  • Use the port number specified in the System property, otherwise, use automatic ports.
  • If a port is in use (for example, tests that require multiple ports), then:
    • Use the port with the following number (increased by 1) as specified in the System property.
    • Otherwise, use a different automatic port for the new connection.

What do you think about that ideology?

@AnthonyMBonafide
Copy link

Would it suffice to update the base class, WebTestBase, with the described logic, and update its subclasses to use this newly added functionality from the super class rather then using hard-coding values like 8080?

@biolearning
Copy link

@pmlopes, I apply to pick up this issue and update that with the logic as describered by @brunoais.
If approved, how can I proceed this 'quick win'?

@pmlopes
Copy link
Member

pmlopes commented Mar 8, 2018

@biolearning sorry for the late reply, sure you can proceed

@brunoais
Copy link
Contributor Author

brunoais commented Mar 1, 2020

@biolearning Still not done?

@dratler
Copy link

dratler commented Apr 5, 2020

Hi @brunoais ,
has someone took this one ?

@brunoais
Copy link
Contributor Author

brunoais commented Apr 5, 2020

@dratler I'm not working on it. I'm not a member of this so I'm not keeping track if someone is working on this.
It does appear, from this issue, @biolearning was working on it but it's been 1 year since then

@dratler
Copy link

dratler commented Apr 9, 2020

Hi @pmlopes ,
is this issue still relevant?

@pmlopes
Copy link
Member

pmlopes commented Apr 9, 2020

Hi @dratler, it's a low priority bug. If you would like to fix it, please open a pull request and we will assist with any help you need.

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

No branches or pull requests

7 participants