Issue 57 bis - Closes #64 #67
Conversation
|
| @@ -27,7 +27,8 @@ | |||
| "ext-curl": "*" | |||
| }, | |||
| "require-dev": { | |||
| "satooshi/php-coveralls": "dev-master" | |||
| "satooshi/php-coveralls": "dev-master", | |||
| "phpunit/phpunit": ">=4.0.0" | |||
There was a problem hiding this comment.
How this is helpful knowing, that:
- Travis CI has PHPUnit installed already
- Most of developers have PHPUnit installed anyway (e.g. via Composer globally or PHAR)
?
There was a problem hiding this comment.
well, I personally do not have phpunit installed globally, but tend to install it inside every project I develop. This because there have been in the past incompatible updates, so different projects were stuck with different phpunit versions.
But I can move this to a separate PR if you think
There was a problem hiding this comment.
well, I personally do not have phpunit installed globally, but tend to install it inside every project I develop.
I'm personally against having tools, that are better to be used at latest version on all projects as project dependency at all. PHPUnit tends to include more and more different dependencies to work and all that really takes up disk space.
There was a problem hiding this comment.
ok then, will revert
|
ps: will squash after all the changes from suggestions |
Since GitHub code review process isn't very feature reach, then please reply to inline comments once you'll have corresponding changes committed/pushed. |
|
All points for which there was a clear indication have been tackled. |
|
While this PR is being reviewed the 0c20707 commit was made to revert code line added, that caused exception. |
| namespace WebDriver\Exception; | ||
|
|
||
|
|
||
| final class CurlDetails extends \Exception { |
There was a problem hiding this comment.
Move to lib/WebDriver/Service/CurlServiceException.php and rename class to CurlServiceException
Fix: require phpunit when installing via composer in dev mode Allow configuring testsuite via env var (for root url) Refactor error handling for curl requests Clean up leftover code Remove one more leftover var_dump comment Add one more test for Selenium different response types Better detection of non-running selenium (tested on php 5.5.17 windows) Proper fix for last commit (skip tests when selenium down, not otherwise) Merge branch 'master' of github.com:instaclick/php-webdriver into issue_57_bis Conflicts: lib/WebDriver/SauceLabs/SauceRest.php Fix the travis build which runs selenium fix travis/selenium/firefox: 2nd try Remove one more leftover Revert: declare dependency on phpunit Merge branch 'master' of github.com:instaclick/php-webdriver into issue_57_bis Implement changes according to PR review One more PR fix: even more detailed error message for bad responses from Selenium Fix previous commit: typo Implement changes recommended by @robocoder
|
Implemented all changes suggested by @robocoder and squashed. |
|
@robocoder Is there anything blocking the merge of this? We're using php-webdriver with Behat (which requires |
|
@chillu Sorry. The last commit message was "WIP: ...", and it simply fell by the wayside. If everyone is satisfied with this PR's functionality, I'll review and merge. |
|
@robocoder I think you can take that silence as agreement? ;) |
|
I am also ok with this being merged :-) |
|
@robocoder Given your message from October, are you happy to press the big green button? ;) |
|
@robocoder this merge caused breaking regressions. Can you please look at #79? |
A fix for issue #57 that avoids introducing issue #64.
Intended to replace #58