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

GeckoDriverService should wait for the geckodriver to be running #2255

Merged
merged 1 commit into from
Jun 10, 2016

Conversation

ngsankha
Copy link
Contributor

@ngsankha ngsankha commented Jun 10, 2016

@AutomatedTester
Copy link
Member

@lukeis could you review please? I have done a quick look and looks good to me.

while (System.currentTimeMillis() < end) {
try {
Socket socket = new Socket();
socket.connect(new InetSocketAddress("localhost", port), 1000);
Copy link
Member

Choose a reason for hiding this comment

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

is this the only difference between pollPort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukeis yes, and we are catching a few other Exceptions as a result of this.

Copy link
Member

Choose a reason for hiding this comment

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

so, why not modify the other pollPort? I'm not a fan of seemingly duplicate code that does the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukeis The pollPort function checks if a server can be created on that port, where as this checks if the server is running so that it can be connected to. Or are you referring to some other pollPort function that I missed?

Copy link
Member

Choose a reason for hiding this comment

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

i think you're confusing checkPortIsFree? pollPort, directly above this method does almost the same same logic.

Copy link
Member

Choose a reason for hiding this comment

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

ah, you're right, it's trying to bind instead... i thought we had other logic... let me check

Copy link
Member

Choose a reason for hiding this comment

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

looks like we don't... ok then :)

@lukeis lukeis merged commit 44dc930 into SeleniumHQ:master Jun 10, 2016
@ngsankha
Copy link
Contributor Author

@lukeis When will the next standalone jar be released? This would be really important for making it work reliably with Geckodriver. :)

@lukeis
Copy link
Member

lukeis commented Jun 10, 2016

¯_(ツ)_/¯ 3.0-beta will be the next release and will be done "when it's ready"

By Christmas, definitely Christmas....

https://botbot.me/freenode/selenium/2016-04-29/?msg=65149334&page=4

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.

None yet

3 participants