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

Update to PHPUnit 5.4+/6.x #365

Merged
merged 16 commits into from Aug 23, 2017

Conversation

Projects
None yet
4 participants
@Jean85
Contributor

Jean85 commented Aug 9, 2017

As discussed in #363, this PR aims to migrate to PHPUnit 5.4, so it will be also cross compatible with PHPUnit 6.

The hardest part was migrating the AbstractCacheConstraint and the WebServerListener classes, but @lyrixx pointed us toward a nice trick that was used in the symfony/phpunit-bridge to solve the same issue.

@lyrixx

This comment has been minimized.

lyrixx commented Aug 9, 2017

Why don't you add the phpunit brige ? It will help with deprecation & co ?

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 9, 2017

@lyrixx right now I'm focused into making the build green, I would like to avoid having issues due to an other additional change.

Later, if the maintainers are ok with that, I can add it!

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 9, 2017

Build is green under PHP 7.1 & 7.2, but not on 7.0 and lower... the strange thing is that between 7.0 and 7.1 the only different dependency seems to be doctrine/instantiator (1.0.5 vs 1.1.0)...

Any suggestions on how to fix this?

@dbu

This comment has been minimized.

Contributor

dbu commented Aug 9, 2017

great work so far, thanks! i will only have time to look at this when i am back from a long weekend next tuesday.

@Ocramius

This comment has been minimized.

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 10, 2017

Just tested it, Ocramius is right, doctrine/istantiator is not the culprit: with the same deps it fails under PHP 7.0 but not under 7.1/7.2... Also I'm unable to reproduce the error locally.

@dbu

This comment has been minimized.

Contributor

dbu commented Aug 18, 2017

now it looks like you are only left with a name clash for the class TestListener... can you try to rebase and see if you can avoid the name clash?

Jean85 added some commits Aug 9, 2017

@Jean85 Jean85 force-pushed the Jean85:update_phpunit branch from bd9efc7 to a68e164 Aug 18, 2017

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 18, 2017

Nope, the clash is still there 😞

@dbu dbu referenced this pull request Aug 19, 2017

Closed

Update to PHPUnit 5.4+/6.x #372

/**
* A PHPUnit test listener that starts and stops the PHP built-in web server.
*

This comment has been minimized.

@dbu

dbu Aug 19, 2017

Contributor

please add a line here to say what versions of php this legacy listener is for. and also change the first line of the class doc to mention phpunit 5 legacy support.

@dbu

i made some progress :-D in 94d5de0 i tried to alias the phpunit TestListener interface to something else. it looks to me like the class_alias functionality merges the use statements from both files or something like that.

then in a25974e i tried to find a solution for a phpunit exception that is not forward compatible.

https://travis-ci.org/FriendsOfSymfony/FOSHttpCache/builds/266238016 looks a lot better. lowest version fails. can you bump the lowest version to one that has the forward compatible interfaces? i guess that should be 5.6 or 5.7.

if (class_exists('PHPUnit_Runner_Version') && version_compare(\PHPUnit_Runner_Version::id(), '6.0.0', '<')) {
class_alias('FOS\HttpCache\Test\Legacy\PHPUnit\AbstractCacheConstraint', 'FOS\HttpCache\Test\PHPUnit\AbstractCacheConstraint');
// Using an early return instead of a else does not work when using the PHPUnit phar due to some weird PHP behavior (the class
// gets defined without executing the code before it and so the definition is not properly conditional)

This comment has been minimized.

@dbu

dbu Aug 19, 2017

Contributor

please indent this comment so that the ci is happy.

if (class_exists('PHPUnit_Runner_Version') && version_compare(\PHPUnit_Runner_Version::id(), '6.0.0', '<')) {
class_alias('FOS\HttpCache\Test\Legacy\WebServerListener', 'FOS\HttpCache\Test\WebServerListener');
// Using an early return instead of a else does not work when using the PHPUnit phar due to some weird PHP behavior (the class
// gets defined without executing the code before it and so the definition is not properly conditional)

This comment has been minimized.

@dbu

dbu Aug 19, 2017

Contributor

please indent to make the ci happy

* WEB_SERVER_PORT port to listen on (required)
* WEB_SERVER_DOCROOT path to the document root for the server (required)
*/
class WebServerListener implements TestListener

This comment has been minimized.

@dbu

dbu Aug 19, 2017

Contributor

if i understand my random experiments correctly, the class_alias thing merges the use statements from both files. to work around, i suggest you alias the TestListener in this file to LegacyTestListener.

namespace FOS\HttpCache\Test;
/**
* This fake trait is used to have the same code and behavior between WebServerListener and its legacy version.

This comment has been minimized.

@dbu

dbu Aug 19, 2017

Contributor

why fake?

This comment has been minimized.

@Jean85

Jean85 Aug 22, 2017

Contributor

Just because it's a class, not a real trait 😄

@dbu

yeah, progress!

/**
* A PHPUnit test listener that starts and stops the PHP built-in web server.
* This legacy version is for PHPUnit 5.x (min 5.4.4 required, due to FC layer)

This comment has been minimized.

@dbu

dbu Aug 22, 2017

Contributor

styleci complains about missing . at the end. please also add a blank line before this line.

@Jean85 Jean85 force-pushed the Jean85:update_phpunit branch from f36ae21 to bd3b5d7 Aug 22, 2017

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 22, 2017

CS fixed. CI seems to be stuck due to unreachable server for the NGINX package...

@dbu

This comment has been minimized.

Contributor

dbu commented Aug 22, 2017

i will restart the build later today to see. guess you could squash the commits and then we are hopefully good to merge!

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 22, 2017

If I squash the commits now I'll trigger an other not-useful build...
I'll do it later, but if I forgot it you can do it while merging, directly from the GitHub interface!

@dbu

This comment has been minimized.

Contributor

dbu commented Aug 22, 2017

@hpatoio we get build errors because frickle.com times out. they are the ones providing the purge extension for nginx... do you know them, would you know if they discontinued delivering the nginx extension or if its just a technical problem and they will fix their servers?

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 22, 2017

I don't know thoroughly your CI process, but shouldn't be easier to migrate it to Docker containers? A cache purge could be done with a container restart...

@dbu

This comment has been minimized.

Contributor

dbu commented Aug 23, 2017

docker

there is #344 which is about adding a docker file. i think that would be the way forward, also to be able to install the varnish xkey plugin in #332 so that we could test that... but it seems that #344 has stalled. if you want to pick it up, i'd be happy. i'd prefer having slower build over having to host a custom docker image somewhere that then needs to be rebuilt and re-uploaded manually when one of the vendors changes.

@dbu

This comment has been minimized.

Contributor

dbu commented Aug 23, 2017

ok, the frickle.com server is back online. now we get only an error for the lowest version build. can you bump the minimal phpunit version some more, @Jean85 ?

@Jean85

This comment has been minimized.

Contributor

Jean85 commented Aug 23, 2017

Done... Yay the build is finally green!
Bumping up to PHPUnit 5.7.0 was the only way, the FC layer for the TestListener was added only up there.

@dbu dbu merged commit 77165c3 into FriendsOfSymfony:master Aug 23, 2017

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dbu

This comment has been minimized.

Contributor

dbu commented Aug 23, 2017

thanks a lot for your patience and persistence!

@dbu dbu referenced this pull request Oct 9, 2017

Closed

phpunit 6 support #363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment