Allow to specify mink driver in browser configuration #30

Vinai · 2015-01-17T14:24:34Z

Add option to specify the mink driver as part of the browser configuration.
For backward compatibility defaults to Selenium2Driver.
Add Goutte and SahiDriver support.
Allows to register additional mink drivers.

For example, to configure a test to run with the Goutte driver, use

    public static $browsers = [[
        'browserName' => 'Goutte',
        'minkDriverClass' => 'Behat\\Mink\\Driver\\GoutteDriver'
    ]];

Closes #12

Vinai · 2015-01-17T14:33:48Z

Is the failing build is not related to the PR?

PHP Fatal error:  Uncaught exception 'WebDriver\Exception\CurlExec' with message 'Curl error thrown for http PUT to https://saucelabs.com/rest/v1//jobs/session with params: {"passed":true}

At least that is the only test that fails (so far). Locally all tests are green. If you are interested in merging the PR, please let me know if there is anything I can do to have travis pass.

aik099 · 2015-01-17T17:39:02Z

library/aik099/PHPUnit/BrowserConfiguration/BrowserConfiguration.php

@@ -14,7 +14,6 @@
 use aik099\PHPUnit\BrowserTestCase;
 use aik099\PHPUnit\Event\TestEndedEvent;
 use aik099\PHPUnit\Event\TestEvent;
-use aik099\PHPUnit\IEventDispatcherAware;


The import was unused I presume.

Indeed, it must have been a leftover.

Vinai · 2015-01-17T18:19:48Z

Thank you very much for reviewing, I'll wait another hour or so if you comment more before updating the PR.

aik099 · 2015-01-17T18:37:18Z

library/aik099/PHPUnit/BrowserConfiguration/BrowserConfiguration.php

@@ -42,6 +41,7 @@ class BrowserConfiguration implements EventSubscriberInterface
 		'browserName' => 'firefox',
 		'desiredCapabilities' => array(),
 		'baseUrl' => '',
+		'minkDriverClass' => 'Behat\\Mink\\Driver\\Selenium2Driver',


I think you can remove mink word from key name and all associated methods because library itself is Mink-only compatible so the driver could only mean Mink's driver. I guess same approach is taken in Behat's .behat.yml with not writing mink word everywhere, but I'm not 100% sure.

Here is code link: https://github.com/Behat/MinkExtension/tree/master/src/Behat/MinkExtension/ServiceContainer/Driver

You can reuse aliases and way how driver parameters end up in actual driver class constructor arguments. This is how it's done for Guzzle: https://github.com/Behat/MinkExtension/blob/master/src/Behat/MinkExtension/ServiceContainer/Driver/GoutteFactory.php

Ah, I missed this note of yours. Do I understand correctly you would like me to copy the code from the linked class into the phpunit-mink driver factories?

Or write something similar. Always easier to modify existing class, then writing one from scratch.

aik099 · 2015-01-17T19:00:59Z

@Vinai , the PR itself is very in time, because several people already have requested such functionality in one way or another.

Today’s review completed. Better to make changes in phrases so nobody gets lost in review process.

Vinai · 2015-01-17T19:10:40Z

Did all the small changes, now only the big changes to the driver factory and options to the browser config is open.
Would you like me to update the PR already? I'll need to be afk a little while before I do those, so it will be a few hours.

aik099 · 2015-01-17T19:25:37Z

Also please comment on PR because GitHub doesn't send e-mail when new commits are pushed for review. But I think you're already aware of that.

Would you like me to update the PR already? I'll need to be afk a little while before I do those, so it will be a few hours.

No worries. You can make changes when you have time. I'll return back to review them tomorrow then (it's 21:25 already in my country).

aik099 · 2015-01-17T19:40:18Z

Also when all will be ok with code changes and public api would be stable please update ReadTheDocs documentation in the docs folder as well. You can find instructions on how to configure docs build environment locally here: https://github.com/minkphp/phpunit-mink/tree/master/docs

aik099 · 2015-01-17T20:07:15Z

Maybe we also can merge driver registry in session factory because the only think SessionFactory is doing is creating new Session using driver that is created using registry.

aik099 · 2015-01-18T10:25:01Z

@Vinai please do the following (don't forget to commit your changes first):

rebase on top of upstream's master to get passing Travis CI build and ability to run tests locally without SauceLabs account
run composer install to sync used dependency versions

Vinai · 2015-01-18T12:38:14Z

Quick note: appologies for the delay, life has kept me quite busy today. I have a few spare hours later though, hope to get the changes done then.

Vinai · 2015-01-18T21:44:43Z

Pushed update with all feedback from the code review implemented.
Here is the updated commit message for easy access:

Add option to specify the mink driver as part of the browser configuration.
For backward compatibility defaults to Selenium2Driver.
Add Goutte and SahiDriver support.
Allows to register additional mink drivers.

For example, to configure a test to run with the Goutte driver, use

    public static $browsers = [[
        'browserName' => 'Goutte',
        'driver' => 'goutte'
    ]];

There also is a new configuration option 'driverOptions' which can be used
by driver factories when instantiating the mink driver.
It defaults to null, places no restriction on the data structure and
is currently not used by any of the driver factories.

Also added a convenience feature that setting the browserName to 'goutte' in
the browser configuration will automatically also switch the driver
to goutte. This behavior is not consistent, but since goutte actually is a
headless browser and it probably will be the most used configuration besides
the default selenium2, I think it is a good idea.

Vinai · 2015-01-18T21:46:06Z

OK, just realized I'm no longer checking which mink drivers are installed. Will fix that.

Vinai · 2015-01-21T16:04:41Z

Thanks, thats what I meant.

Vinai · 2015-01-21T16:18:16Z

After investigating the possible driver options I decided not to implement that now due to time constraints.
As I wrote somewhere earlier, I personally don't need it and in order to create decent tests for the usage of the options I would need to understand what each option does. So not in this PR, sorry.
If that is a blocker then thats what its going to be.
I've also added a Zombie driver factory.

aik099 · 2015-01-21T16:21:38Z

It's ok. Better to create stub classes for other driver factories so they at least create driver as written in manual (on ReadTheDocs.org) and pass host/user/pass to it and that's it. Then we can finalize them in another PR.

aik099 · 2015-01-24T10:12:20Z

To prevent Mink driver not installed messages in tests we might want to alter .travis.yml so that it will install all Mink drivers to fully test factories.

aik099 · 2015-01-24T10:17:08Z

library/aik099/PHPUnit/BrowserConfiguration/BrowserConfiguration.php

@@ -356,6 +367,60 @@ public function getBaseUrl()
 	}

 	/**
+	 * Set the mink driver to use.


We should use Mink term instead of mink across the PR, because it's name of the product here, not an animal 😉

aik099 · 2015-01-24T16:29:39Z

@Vinai , let me know, when you'll complete 100% of comments so that I can review.

Vinai · 2015-01-26T15:39:02Z

All done.

aik099 · 2015-01-28T19:37:05Z

library/aik099/PHPUnit/BrowserConfiguration/BrowserConfiguration.php

+	 * @param string $driver The driver to use.
+	 *
+	 * @return self
+	 * @throws \InvalidArgumentException When Mink driver is not a valid driver alias.


Pay attention to text in DocBlocks. This method no longer validates driver alias. The only thing it does is checks that it's a string. Therefore please update it accordingly.

aik099 · 2015-02-16T07:44:15Z

@Vinai , have you've completed all requested changes?

* Add option to specify the mink driver as part of the browser configuration. * For backward compatibility defaults to Selenium2Driver. * Add Goutte and SahiDriver support. * Allows to register additional mink drivers. For example, to configure a test to run with the Goutte driver, use public static $browsers = [[ 'browserName' => 'Goutte', 'driver' => 'goutte' ]]; There also is a new configuration option 'driverOptions' which can be used by driver factories when instantiating the mink driver. It defaults to null, places no restriction on the data structure and is currently not used by any of the driver factories. This PR is realated to issue minkphp#12.

Vinai · 2015-02-17T11:50:52Z

Sorry for the delay, life is has been too busy (moving house etc). Pushed the latest update.

aik099 · 2015-03-21T08:15:58Z

Closing in favor of #49

Vinai · 2015-03-21T14:26:08Z

👍

aik099 added the type-enhancement label Jan 17, 2015

aik099 reviewed Jan 17, 2015
View reviewed changes

aik099 added the state-pending-changes label Jan 17, 2015

aik099 changed the title ~~Allow to specify mink driver in browser configuration.~~ Allow to specify mink driver in browser configuration Jan 18, 2015

aik099 modified the milestone: 2.1.0 Jan 18, 2015

Vinai force-pushed the more-mink-drivers branch from 21910b1 to e0f7acc Compare January 18, 2015 21:41

Vinai force-pushed the more-mink-drivers branch from 7de3e8b to 7137265 Compare January 21, 2015 16:14

aik099 reviewed Jan 24, 2015
View reviewed changes

Vinai force-pushed the more-mink-drivers branch from 7137265 to 658343d Compare January 26, 2015 15:38

aik099 added state-pending-review and removed state-pending-changes labels Jan 26, 2015

aik099 reviewed Jan 28, 2015
View reviewed changes

aik099 added state-pending-changes and removed state-pending-review labels Jan 28, 2015

Vinai force-pushed the more-mink-drivers branch from 658343d to 0f6721e Compare February 17, 2015 11:50

aik099 mentioned this pull request Mar 21, 2015

Mink driver selection ability in configuration #49

Merged

aik099 removed the state-pending-changes label Mar 21, 2015

aik099 closed this Mar 21, 2015

aik099 modified the milestone: 2.1.0 May 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to specify mink driver in browser configuration #30

Allow to specify mink driver in browser configuration #30

Vinai commented Jan 17, 2015

Vinai commented Jan 17, 2015

aik099 Jan 17, 2015

Vinai Jan 17, 2015

Vinai commented Jan 17, 2015

aik099 Jan 17, 2015

Vinai Jan 17, 2015

aik099 Jan 18, 2015

Vinai Jan 21, 2015

aik099 Jan 21, 2015

aik099 commented Jan 17, 2015

Vinai commented Jan 17, 2015

aik099 commented Jan 17, 2015

aik099 commented Jan 17, 2015

aik099 commented Jan 17, 2015

aik099 commented Jan 18, 2015

Vinai commented Jan 18, 2015

Vinai commented Jan 18, 2015

Vinai commented Jan 18, 2015

Vinai commented Jan 21, 2015

Vinai commented Jan 21, 2015

aik099 commented Jan 21, 2015

aik099 commented Jan 24, 2015

aik099 Jan 24, 2015

Vinai Jan 26, 2015

aik099 commented Jan 24, 2015

Vinai commented Jan 26, 2015

aik099 Jan 28, 2015

Vinai Feb 17, 2015

aik099 commented Feb 16, 2015

Vinai commented Feb 17, 2015

aik099 commented Mar 21, 2015

Vinai commented Mar 21, 2015

Allow to specify mink driver in browser configuration #30

Allow to specify mink driver in browser configuration #30

Conversation

Vinai commented Jan 17, 2015

Vinai commented Jan 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinai commented Jan 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Jan 17, 2015

Vinai commented Jan 17, 2015

aik099 commented Jan 17, 2015

aik099 commented Jan 17, 2015

aik099 commented Jan 17, 2015

aik099 commented Jan 18, 2015

Vinai commented Jan 18, 2015

Vinai commented Jan 18, 2015

Vinai commented Jan 18, 2015

Vinai commented Jan 21, 2015

Vinai commented Jan 21, 2015

aik099 commented Jan 21, 2015

aik099 commented Jan 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Jan 24, 2015

Vinai commented Jan 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Feb 16, 2015

Vinai commented Feb 17, 2015

aik099 commented Mar 21, 2015

Vinai commented Mar 21, 2015