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

Fix all tests for PHP 7.x #189

Closed
wants to merge 32 commits into from
Closed

Fix all tests for PHP 7.x #189

wants to merge 32 commits into from

Conversation

beoboo
Copy link

@beoboo beoboo commented Mar 5, 2019

Tests should be run with phpunit and not simple-phpunit (because PHP 7-1 uses PHPUnit 5.7).

Copy link
Member

@aik099 aik099 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests should be run with phpunit and not simple-phpunit (because PHP 7-1 uses PHPUnit 5.7).

I'm not sure, why you thinking, that simple-phpunit is used.

composer.json Outdated Show resolved Hide resolved
src/NodeJS/Server.php Outdated Show resolved Hide resolved
src/NodeJS/Server.php Outdated Show resolved Hide resolved
@aik099
Copy link
Member

aik099 commented Mar 5, 2019

Fixing https://travis-ci.org/minkphp/MinkZombieDriver/jobs/502013256 would require locking to non-latest PHPUnit version for PHP 7.x version.

To fix https://travis-ci.org/minkphp/MinkZombieDriver/jobs/502013252 you need to do use latest PHPUnit version compatible with PHP 5.5.

tests/Custom/TestCase.php Outdated Show resolved Hide resolved
@beoboo beoboo changed the title Fixed all tests for PHP 7.x Fix all tests for PHP 7.x Mar 5, 2019
@aik099
Copy link
Member

aik099 commented Mar 5, 2019

The https://travis-ci.org/minkphp/MinkZombieDriver/jobs/502148600 build of PHP 5.3 has failed due incorrect NODEJS path.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@aik099
Copy link
Member

aik099 commented Mar 6, 2019

@beoboo , good job indeed. Just finish latest comments I've mentioned (about composer.json changes).

I have no idea why PHP 5.3 build fails that bad. I've tried to to run test locally using PHP 5.3.29 and Zombie 6.1.4.

@beoboo
Copy link
Author

beoboo commented Mar 6, 2019

I think PHP 5.3 might be failing because the script is not able to run

vendor/bin/mink-test-server > /dev/null 2>&1 &`

(or better it exists without showing any error). I'll try removing the output redirects to debug it.

@beoboo
Copy link
Author

beoboo commented Mar 6, 2019

I created a wrong #190, #189 is the last correct one that fixed all tests.

@aik099
Copy link
Member

aik099 commented Mar 6, 2019

You do create lots of commits (I guess no other way to test Travis CI) and I do get an e-mail for every of them.

When you're ready for review just post a comment.

@beoboo
Copy link
Author

beoboo commented Mar 6, 2019

Ready for review 😄.

@aik099
Copy link
Member

aik099 commented Mar 6, 2019

Looks good to me. @stof , what you think?

@aik099 aik099 requested a review from stof March 6, 2019 11:38
@rpkamp
Copy link

rpkamp commented Mar 26, 2019

This looks awesome, and would bring mink one step closer to be compatible with Symfony 4 (see minkphp/Mink#757).

Anything I can do to help this move forward?

@Einenlum
Copy link

Any news?

@rpkamp
Copy link

rpkamp commented Jun 18, 2019

@aik099 can we move forward on this please? Moving forward to allow using Symfony 4 is an important next step for Mink I think, since all developers that use Symfony flex that install Mink now will get an error.

@aik099
Copy link
Member

aik099 commented Jun 19, 2019

@rpkamp , still waiting for @stof to review.

@stmllr
Copy link

stmllr commented Aug 14, 2019

I applied this PR and raised composer requirement "symfony/process": "~2.1|~3.0|~4.0". My tests are running fine with Symfony process 4.3.3.

@Sidlegionair
Copy link

@stof Any updates on this?

@stof
Copy link
Member

stof commented Oct 11, 2021

Closing in favor of #196

@stof stof closed this Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants