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

Allow request_timeout_in_ms and connection_timeout_in_ms for WebDriver #2065

Merged
merged 2 commits into from Jun 19, 2015

Conversation

@n8whnp
Copy link
Contributor

@n8whnp n8whnp commented Jun 18, 2015

This allows you to pass all possible arguments to php-webdriver's create()
from settings in the suite.yml file.

…bDriver

This allows you to pass all possible arguments to php-webdriver's create()
from settings in the suite.yml file.
@DavertMik
Copy link
Member

@DavertMik DavertMik commented Jun 18, 2015

Personally I don't like _in_ms suffix. I agree that this should be taken into account, as all other timeouts in configuration are seconds, but I'd rather write that in documentation instead of naming parameter this way

@n8whnp
Copy link
Contributor Author

@n8whnp n8whnp commented Jun 19, 2015

@DavertMik I am happy to change the parameter name to match conventions within codeception. I see two possible ways forward here:

  1. Remove the suffix and just mention the timeout is in miliseconds in the documentation.
  2. Remove the suffix and convert the time from seconds to miliseconds in the __initialize() function.

Which would you prefer going forward?

@zbateson
Copy link
Member

@zbateson zbateson commented Jun 19, 2015

Hi @n8whnp

Let's keep it in seconds and handle it in __initialize I think that is cleanest :) then in documentation it can say "default is 30 seconds".

Thanks

@n8whnp
Copy link
Contributor Author

@n8whnp n8whnp commented Jun 19, 2015

@zbateson thanks for the quick reply, updated to reflect agreed changes.

zbateson added a commit that referenced this pull request Jun 19, 2015
Allow request_timeout_in_ms and connection_timeout_in_ms for WebDriver
@zbateson zbateson merged commit e6f1410 into Codeception:2.0 Jun 19, 2015
3 checks passed
3 checks passed
Scrutinizer 2 new issues
Details
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zbateson
Copy link
Member

@zbateson zbateson commented Jun 19, 2015

Excellent, thank you :)

@n8whnp
Copy link
Contributor Author

@n8whnp n8whnp commented Jun 19, 2015

Given the branch name, will this be going out in a future 2.0 release or is it going out as part of 2.1?

@zbateson
Copy link
Member

@zbateson zbateson commented Jun 19, 2015

I will leave that for @DavertMik to respond to

@DavertMik
Copy link
Member

@DavertMik DavertMik commented Jun 25, 2015

This is going to be included in 2.0.15 which will be released today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.