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

Find free hub port to support multiple local grids in common environments #275

Closed
wants to merge 1 commit into from

Conversation

squash-merge
Copy link
Contributor

Added a check to see whether the hub port in configuration is available.
Find a another free port when the hub port in the configuration is occupied.

I have not been able to test it for some reasons specific to my env.

int configHubPort = Integer.parseInt(Config.getConfigProperty(ConfigProperty.SELENIUM_PORT));
// Choose another free port when config port is not available.
int hubPort = PortProber.pollPort(configHubPort) ? configHubPort : PortProber.findFreePort();
instance.setPort(hubPort);
Copy link

Choose a reason for hiding this comment

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

Should we update the value of the property SELENIUM_PORT if we are not using 4444?

@squash-merge squash-merge force-pushed the develop branch 2 times, most recently from cf88513 to ada8a81 Compare April 25, 2016 20:06
instance.setPort(Integer.parseInt(Config.getConfigProperty(ConfigProperty.SELENIUM_PORT)));
int configHubPort = Integer.parseInt(Config.getConfigProperty(ConfigProperty.SELENIUM_PORT));
// Choose another free port when config port is not available.
int hubPort = LocalHub.pollPort(configHubPort) ? configHubPort : PortProber.findFreePort();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to fix my development environment and test this change. The PortProber.pollPort(int) returned true even when another grid was already running at port 4444. Hence add a private static method in LocalHub.

@squash-merge
Copy link
Contributor Author

Updated changes in ada8a81.

@mach6
Copy link
Contributor

mach6 commented Apr 25, 2016

@shelar I liked the previous version of this PR more. If we are starting a LocalHub, let's always assign a random freePort. We already do this for LocalNode, so let's be consistent. The only issue I took with the previous version is ConfigProperty.SELENIUM_PORT defaults to 4444 and the property value was not updated when a LocalHub was started.

@squash-merge
Copy link
Contributor Author

@mach6 I agree with the 'always random port' option too. Will update changes shortly.

@squash-merge squash-merge force-pushed the develop branch 2 times, most recently from 54808e4 to 93e2410 Compare April 25, 2016 22:03
@squash-merge
Copy link
Contributor Author

Updated changes to always use random port for hub - 93e2410.

@mach6
Copy link
Contributor

mach6 commented Apr 25, 2016

please increment date range of copyright header.

@mach6
Copy link
Contributor

mach6 commented Apr 25, 2016

Will you please update the javadocs for ConfigProperty#SELENIUN_PORT to include this text Used when {@link ConfigProperty#SELENIUM_RUN_LOCALLY} is <b>false</b> which is also called out for ConfigProperty#SELENIUM_HUB? If you can update the gh-pages branch for the same, I'd appreciate it.

@squash-merge
Copy link
Contributor Author

Updated changes per @mach6's feedback in 362419c

@mach6
Copy link
Contributor

mach6 commented Apr 26, 2016

Fix for #274

@squash-merge
Copy link
Contributor Author

Added Closes #274 to commit message. No other change made in e94ea05.

@mach6
Copy link
Contributor

mach6 commented Apr 26, 2016

LGTM too. merging

@mach6
Copy link
Contributor

mach6 commented Apr 26, 2016

Amended commit message and merged.

@mach6 mach6 closed this Apr 26, 2016
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.

None yet

3 participants