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

Customizable grid-extras-port in nodes #385

Merged
merged 2 commits into from
Feb 9, 2018
Merged

Conversation

mariolameiras
Copy link

Currently there is an issue reported in #284 and some other issues, where if the Grid Extras is listening in the port 3000, the node is expected to be listening on the same port.

This is a bigger issue if you're running selenium-grid-extras inside docker on different machines communicating between them.

I propose to use the selenium node custom configuration map to insert the property grid-extras-port so that the hub knows on which port is the grid-extras from the node listening on.

Mario Lameiras added 2 commits January 12, 2018 18:31

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@smccarthy
Copy link
Collaborator

@mariolameiras Thank you ! I will look at this later this week and if all looks good I'll merge by this weekend and make a new release.

@mariolameiras
Copy link
Author

Did you get a chance to review it? @smccarthy

@smccarthy
Copy link
Collaborator

@mariolameiras I am sorry but I haven't had a chance. I will try to look at it this week .

@smccarthy
Copy link
Collaborator

@mariolameiras Can you tell me if you tested it, and it works well? with the custom port and without it?

@smccarthy smccarthy merged commit 3e86e55 into groupon:2.x Feb 9, 2018
@smccarthy
Copy link
Collaborator

It looks good after looking at it. I will try and do some testing of it tonight (just making sure nothing is broke), and will make a new release in the 2.x branch.

@smccarthy
Copy link
Collaborator

@mariolameiras I noticed it is failing this test testLogSessionWithSessionHistoryEnabled (it failed another one as well but it was a simple fix). Do you have any ideas why this test would start failing? I checked out master and it is passing there.

@mariolameiras
Copy link
Author

Sorry, I only checked your message now. Did you figure it out?

@smccarthy
Copy link
Collaborator

@mariolameiras I figured it out I think (and released it with your PR as well in 2.0.2). The issue with testLogSessionWithSessionHistoryEnabled was a test specific issue. I am not sure if it ever passed. I make the test log a mock session, and it passes now.

Thank you for your contribution !

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