Support zombie.js options #154

robballou · 2016-02-03T22:51:15Z

Here is the start of this functionality. I opted to add this as a last parameter rather than allowing the params or an array just for clarity of the interface. Also I added some "validation" of the options per what is in Zombie's BROWSER_OPTIONS settings.

aik099 · 2016-02-04T08:11:56Z

src/NodeJS/Server.php

+     *
+     * @return array
+     */
+    public function getOptions()


Looking at order of existing setters/getters shows that all getters are grouped into one group, and all setters are grouped into another group. Could you please move your setter/getter to comply with that?

aik099 · 2016-02-04T08:14:17Z

Also if you can create some tests for changed code, this would be great. You can manually create ZombieServer and talk to it without using driver class as well.

aik099 · 2016-02-04T08:23:54Z

src/NodeJS/Server.php

+    /**
+     * Set options array.
+     */
+    public function setOptions($options)


Please update example in README.md to show that it's now possible to set Zombie options.

While we're at it you can send PR's to MinkExtension and PHPUnit-Mink to update Zombie integration to allow specifying Zombie options in there as well.

That would be a great addition.

Rather than updating the readme, it would be better to update the docs repo though

aik099 · 2016-02-15T08:48:20Z

@robballou , any updates?

robballou · 2016-02-16T15:22:21Z

I might have some leeway this week to work on it. Thanks

aik099 · 2016-03-04T11:29:26Z

src/NodeJS/Server.php

+    /**
+     * Set a single option.
+     */
+    public function setOption($option, $value)


Please inline this method (after fixing other snuff), because it will complicate things down the road.

Not sure I follow what this means. I just posted some other changes, so let me know and I'll get that in there.

Inline means:

put body the method (setOption in this case) inside the method, that calls it (setOptions in this case)

remove original method (setOption in this case)

…hich was missing

aik099 · 2016-03-04T16:02:48Z

src/NodeJS/Server.php

+     *
+     * @return mixed The option value or default value if it is not set.
+     */
+    public function getOption($option, $default_value = NULL)


Please remove this method.

I don't see any potential use of this method, because the only consumer of this options is Zombie itself. Nobody wants to introspect what options current server has.

aik099 · 2016-03-04T16:03:42Z

src/NodeJS/Server.php

+        foreach ($options as $key => $value) {
+            $this->options[$key] = $value;
+        }
+        $this->serverPath = $this->createTemporaryServer();


Please add empty line above. If you have any automated code style checks in place for PSR2 standard then this should have been reported as error by them. Also the Scrutinizer CI should have picked that as well.

aik099 · 2016-03-04T16:06:58Z

Turns out there is a test case covering changed class. @robballou , please also add relevant tests.

stof · 2016-03-05T10:05:40Z

This should be updated to rely on an environment variable containing a json string

robballou · 2016-04-04T13:46:05Z

I might have some time this week to work on this. Taking a look at tests right now.

robballou · 2016-04-04T18:23:34Z

I'm having trouble getting tests to run (just trying in master). If I could get some help with that I can try to write some tests to cover the changes.

Thanks,

Rob

aik099 · 2016-04-04T19:05:07Z

I'm having trouble getting tests to run

What is the specific error you're having when running phpunit?

robballou · 2016-04-04T19:27:33Z

Looks like there is an issue with the JS side of things, attaching the phpunit output:

phpunit.txt

I'm using node 5.9.1.

aik099 · 2016-04-05T07:09:35Z

To summarize the error you're getting from most (if not all) tests is:

Behat\Mink\Exception\DriverException: Error "Unexpected identifier" while executing code: ...

What is Zombie version you're using? I'm on 4.x something. Not sure how to determine which exactly. Try installing 4.0.0 version of Zombie and see if tests pass then.

aik099 · 2016-04-05T07:10:20Z

And if you're on Windows then you might also get that error.

aik099 · 2016-06-16T18:54:40Z

@robballou , any update?

robballou · 2016-06-30T14:11:23Z

Last time I tried, I still had issues running tests but I haven't been able to work on it since April :(

stof · 2016-11-02T16:45:49Z

For reference, the list of available options is defined at https://github.com/assaf/zombie/blob/v4.3.0/src/index.js#L30

christopher-francisco · 2016-12-07T18:25:40Z

Will this options include timeout? 5 seconds is just too little

aik099 · 2016-12-07T19:11:31Z

All options will be passed to Zombie as-as (we won't be storing local copies of allowed options in the driver). So if in Zombie docs there is such options it will be passed.

christopher-francisco · 2016-12-07T19:44:38Z

What is needed in order to merge this? I can help if you need me to do something (though I'm really new to Zombie in general, been using it for maximum 1 month)

aik099 · 2016-12-07T20:44:38Z

You need to:

have Zombie installed locally
make sure you can run test suite locally
create new PR as rebased version of this PR, that will also use env variable (according to Support zombie.js options #154 (comment))
add some tests (according to Support zombie.js options #154 (comment))
try to use changes from PR to see if they really set options for Zombie

aik099 · 2016-12-07T20:49:28Z

Since this PR was created the JS server code was moved to https://github.com/minkphp/MinkZombieDriver/blob/master/bin/mink-zombie-server.js file, that reads server settings from enviroment variables, that are set in

MinkZombieDriver/src/NodeJS/Server.php

Lines 319 to 324 in 614cfc8

    
           $processBuilder->setEnv('HOST', $this->host) 
        
               ->setEnv('PORT', $this->port); 
        
           if (!empty($this->nodeModulesPath)) { 
        
               $processBuilder->setEnv('NODE_PATH', $this->nodeModulesPath); 
        
           }

when Zombie process is created.

aik099 · 2016-12-13T14:17:22Z

@chris-fa , you plan on sending PR any time soon?

christopher-francisco · 2016-12-13T14:19:41Z

@aik099 I'm trying to set up some free time, maybe in the holiday; but I'm not sure whether I might be able to do it quickly, I haven't contributed before to this repo, so I'm not really familiarized with the source code; I'm willing to provide any help I can, but can't guarantee a fast time, sorry :(

aik099 · 2016-12-13T14:24:41Z

Any help any time is appreciated. Thanks upfront.

christopher-francisco · 2017-03-24T15:17:38Z

Is anybody maintaining this PR?

aik099 · 2017-03-24T17:56:43Z

@chris-fa , what do you mean by maintaining? The PR author abandoned this PR more than half a year ago. You told couple of month ago that you might continuing development of this PR and disappeared from the radar as well.

So I guess nobody is writing code for this PR at the moment.

christopher-francisco · 2017-03-24T18:23:36Z

@aik099 I just realized I wrote on this PR my bad 😂

berliner · 2017-06-09T17:34:33Z

I would work on this as I need to be able to specify some options for zombiejs.

I'm a little bit confused about where to do that though. Finally I need to have this feature in https://github.com/Behat/MinkZombieDriver which is behind https://github.com/minkphp/MinkZombieDriver by 64 commits at the time of this comment.

Also, there are needed changes in the ZombieFactory of the Behat/MinkExtension, so I suppose that I need to issue a separate PR there?

What I got so far is this:
master...berliner:berliner/zombie-options
Behat/MinkExtension@master...berliner:berliner/zombie-options

In order to proceed I'd need the following information:

how to run the included tests in both MinkExtension and MinkZombieDriver
ideally also how to run this as part of a behat setup, installed using composer require behat/mink-zombie-driver
~~how to go from minkphp/MinkZombieDriver to behat/MinkZombieDriver~~

Sorry if any of this is too obvious to you, but I rather ask. I already spent a lot of time figuring out why my tests just don't succeed, digging through pretty cryptic error messages and trying to get my head around on how to pass on zombiejs options when going through mink. The documentation is looking very good, but is actually rather thin for anything that is slightly special.

stof · 2017-06-09T17:56:39Z

@berliner you don't need this in the https://github.com/Behat/MinkZombieDriver repo, as this is a legacy repo (from the time where download links were not redirected by github and so we forked back the repo after moving it). the source of the behat/mink-zombie-driver composer package is the https://github.com/minkphp/MinkZombieDriver

berliner · 2017-06-09T18:05:33Z

@stof Thanks for clarifying. It would be good to put a note to that effect on the repo page. It really confused me.

aik099 · 2017-06-10T06:30:51Z

how to run the included tests in both MinkExtension and MinkZombieDriver

Since Zombie is headless driver you only need to do npm -g install zombie to install Zombie once and then you can run tests as many times as needed, because Zombie process is launched/destroyed on demand.

ideally also how to run this as part of a behat setup, installed using composer require behat/mink-zombie-driver

You'll need to create another PR to MinkExtension so that zombie options can be defined in .behat.yml and then passed along to driver.

berliner · 2017-06-12T08:40:02Z

@aik099 Thanks for your answers, I need a clarification though:
When asking how to run the tests, I actually meant the tests that will be run here on github too, so that I can check these before issuing a PR.

Also, I created a PR for MinkExtension: Behat/MinkExtension#284
I am a little lost with the failed test, it seems the test only failed for PHP 5.3 on something not related to my changes. Can you tell me if this is something I should try to fix or am I right assuming that this is unrelated?

aik099 · 2017-06-12T13:10:44Z

When asking how to run the tests, I actually meant the tests that will be run here on github too, so that I can check these before issuing a PR.

What you see on GitHub is result of running phpunit command on Travis CI servers. You can run same phpunit command inside your GitHub repo clone considering you have PHPUnit installed.

Also, I created a PR for MinkExtension: Behat/MinkExtension#284
I am a little lost with the failed test, it seems the test only failed for PHP 5.3 on something not related to my changes. Can you tell me if this is something I should try to fix or am I right assuming that this is unrelated?

Nothing needs to be fixed. I've responded in that PR as well with more details.

berliner · 2017-06-12T13:21:54Z

@aik099 Thanks, that's been sufficiently clear. I am able to run the tests now using phpspec.

aik099 · 2017-06-12T13:23:32Z

PhpSpec? I thought mink tests are written in PHPUnit, but Behat tests (and possible MinkExtension tests) are written in PhpSpec.

berliner · 2017-06-12T13:28:21Z

PhpSpec? I thought mink tests are written in PHPUnit, but Behat tests (and possible MinkExtension tests) are written in PhpSpec.

Sorry to confuse you. I was looking at MinkExtension at the moment, which uses phpspec. Your previous answer made it clear for me that I simply needed to look at the travis logs to find out how to run the tests for the project, which is what I did for MinkExtension as I have an PR open there. I'll be looking at MinkZombieDriver next, which seems to be using phpunit as you said.

aik099 · 2017-07-18T13:36:20Z

Replaced by #183.

robballou added 2 commits February 2, 2016 08:59

Started on allowing Zombie.js options

8a17347

Merge branch 'master' into zombie-options

f57a58e

aik099 reviewed Feb 4, 2016
View reviewed changes

aik099 mentioned this pull request Feb 15, 2016

zombie proxy setting minkphp/Mink#690

Closed

aik099 added the response-needed label Feb 15, 2016

aik099 reviewed Mar 4, 2016
View reviewed changes

robballou added 3 commits March 4, 2016 08:41

Removed option validation, moved options methods, added getOption() w…

860aaca

…hich was missing

Added createTemporaryServer() call in setOptions()

ab2a51e

Removed setOption(), updated setOptions()

80999d8

aik099 reviewed Mar 4, 2016
View reviewed changes

Removed getOption()

95fa786

aik099 reviewed Mar 4, 2016
View reviewed changes

Updated setOptions() code styles

847ecea

stof mentioned this pull request Mar 5, 2016

Add strictSSL support #152

Closed

aik099 mentioned this pull request Jul 3, 2016

Mink + Zombie Driver: Error while processing click and clickLink #145

Closed

aik099 mentioned this pull request Nov 2, 2016

How to change timeout? #175

Closed

berliner mentioned this pull request Jun 10, 2017

Add support for zombiejs options Behat/MinkExtension#283

Open

berliner mentioned this pull request Jun 23, 2017

Add support for zombie options #183

Merged

aik099 removed the response-needed label Jul 18, 2017

aik099 closed this Jul 18, 2017

Support zombie.js options #154

Support zombie.js options #154

Conversation

robballou commented Feb 3, 2016

Choose a reason for hiding this comment

aik099 commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Feb 15, 2016

robballou commented Feb 16, 2016

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 Mar 4, 2016

stof commented Mar 5, 2016

robballou commented Apr 4, 2016

robballou commented Apr 4, 2016

aik099 commented Apr 4, 2016

robballou commented Apr 4, 2016

aik099 commented Apr 5, 2016

aik099 commented Apr 5, 2016

aik099 commented Jun 16, 2016

robballou commented Jun 30, 2016

stof commented Nov 2, 2016

christopher-francisco commented Dec 7, 2016

aik099 commented Dec 7, 2016

christopher-francisco commented Dec 7, 2016

aik099 commented Dec 7, 2016

aik099 commented Dec 7, 2016

aik099 commented Dec 13, 2016

christopher-francisco commented Dec 13, 2016 • edited Loading

aik099 commented Dec 13, 2016

christopher-francisco commented Mar 24, 2017

aik099 commented Mar 24, 2017 • edited Loading

christopher-francisco commented Mar 24, 2017

berliner commented Jun 9, 2017 • edited Loading

stof commented Jun 9, 2017

berliner commented Jun 9, 2017

aik099 commented Jun 10, 2017

berliner commented Jun 12, 2017

aik099 commented Jun 12, 2017 • edited Loading

berliner commented Jun 12, 2017

aik099 commented Jun 12, 2017 • edited Loading

berliner commented Jun 12, 2017

aik099 commented Jul 18, 2017

christopher-francisco commented Dec 13, 2016 •

edited

Loading

aik099 commented Mar 24, 2017 •

edited

Loading

berliner commented Jun 9, 2017 •

edited

Loading

aik099 commented Jun 12, 2017 •

edited

Loading

aik099 commented Jun 12, 2017 •

edited

Loading