-
Notifications
You must be signed in to change notification settings - Fork 74
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
Enable selenium grid support#40 #42
Conversation
Do we need a test for this? |
This is just use of capabilities in my opinion even simpler than for sauce. Testing this can be quite tricky because it requires working selenium grid. |
👍 Looks good to me. @hellmanj , @frishberg ? |
@@ -154,7 +155,13 @@ def __init__(self): | |||
|
|||
self._attempt_sauce = self._validate_sauce_options() | |||
|
|||
# There's only a session ID when using a remote webdriver (Sauce, for example) | |||
self._Capabilities = getattr(webdriver.DesiredCapabilities, self.browser.upper()) | |||
for grid_cap in self._Capabilities: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we're checking all options in DesiredCapabilities
here not just grid-related ones. I think that's fine, but we shouldn't use "grid" in these variable names.
Also, it would be great if you could you update the readme with a brief section on using grid. |
Ok, I will add short description how to use capabilities. |
Please also briefly mention in the README why choose sauce over grid and vice versa. We are pretty unfamiliar with grid. |
Basically Sauce Labs is a fancy selenium grid. Grid is simpler and you can run it in your env. |
Looks good to me. In the read me we may want to specify these are options related to grid and not necessary for running in sauce. Otherwise looks good. Jared? Daniel? |
@hellmanj , @frishberg Are we ok to merge this? Do the option names make sense, or should they be pre-fixed with grid_? |
It's probably fine for now, we can change later if it's apparent there is any confusion. |
OK I am going to reorder the options in the README and merge. |
I closed this because I pushed this forked branch to origin. I will open another PR there. |
No description provided.