? ?
avatar mbabker
mbabker
9 Aug 2016

PHPUnit 4.8 isn't compatible with PHP 7.x and on 7.1 segfaults and doesn't even run. Our CI jobs and test suite need to be updated to be able to run PHPUnit 5.x on PHP 5.6+ (the PHP versions it supports) while being able to run PHPUnit 4.8 on PHP 5.5 and older.

avatar mbabker mbabker - open - 9 Aug 2016
avatar brianteeman brianteeman - change - 9 Aug 2016
Category Unit Tests
avatar brianteeman brianteeman - change - 9 Aug 2016
Labels Added: ?
avatar rdeutz
rdeutz - comment - 16 Aug 2016

This comes bundled with a lot of pain. Made an attempt, first problem is the use off getMock, function is available in 4.8 but not in 5.x, I made a wrapper but then it failed at some test later. I stopped working on this for now, might be something the automated test team should be involved into. (My Branch: https://github.com/rdeutz/joomla-cms/tree/test_phpunit_5x)

avatar yvesh
yvesh - comment - 16 Aug 2016

Agree with Robert this comes with a lot of pain.. Maybe we separate it - prior PHP 7 we run the current unit tests.

And with PHP 7 and above we run the ones that are still working and the ones we port / write within time.. We can have a second setup script that installs PHPUnit 5.x with PHP 5.6+..

avatar rdeutz
rdeutz - comment - 16 Aug 2016

most of the time I come across a unit test I am wondering what we are testing, often it looks for me we are testing if reflection and mocking works but not really the class

avatar mbabker
mbabker - comment - 16 Aug 2016

Made an attempt, first problem is the use off getMock, function is available in 4.8 but not in 5.x

It's deprecated as of PHPUnit 5.4. In a lot of the Framework packages we basically locked the PHPUnit dependencies to ~4.8 | >=5.0 <5.4 until the heavy uses of getMock() can be refactored to getMockBuilder() (as long as we're supporting < 5.6 this is the only cross-branch method without a wrapper like you've done).

Maybe we separate it - prior PHP 7 we run the current unit tests.

It's not that we need a different test suite for PHP 5.6+ (PHPUnit 5). The main hurdle is that we have tests using PHPUnit 4 functions that were removed in PHPUnit 5 (assertTag being the main culprit). Even if we can just get ourselves to PHPUnit 5.3, we've crossed the major hurdle keeping us from trying to run the tests against PHP 7.1. Personally, I'd aim for that version and focus on refactoring off the methods that were deprecated and removed from PHPUnit 5 before tackling the mock API changes in PHPUnit 5.4.

most of the time I come across a unit test I am wondering what we are testing, often it looks for me we are testing if reflection and mocking works but not really the class

Sadly, a heavy majority of the tests seem to validate PHP behaviors more than our own API. We've even had tests at one point that validated the engine raising fatal errors when an object not matching a method's typehinted signature is passed.

avatar brianteeman
brianteeman - comment - 16 Aug 2016

Tests are only as good as the people writing them

avatar yvesh
yvesh - comment - 17 Aug 2016

@mbabker any hints on that: https://travis-ci.org/joomla/joomla-cms/builds/153007525 - have seen that now two times happening this erratic with PHP 7. On a second try with the same code it works fine..

avatar mbabker
mbabker - comment - 17 Aug 2016

It's a Travis thing. Nothing we can do.

On Wednesday, August 17, 2016, Yves Hoppe notifications@github.com wrote:

@mbabker https://github.com/mbabker any hints on that:
https://travis-ci.org/joomla/joomla-cms/builds/153007525 - have seen that
now two times happening this erratic with PHP 7. On a second try with the
same code it works fine..


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11523 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoX76ammriAR-L40qugYHaZkafRkIks5qgzDtgaJpZM4JfnwX
.

avatar mbabker
mbabker - comment - 22 Aug 2016

Apparently PHPUnit 4.8 can run on PHP 7.1 - https://twitter.com/calevans/status/767761955964719104

So apparently there's something "funny" with our tests and the PHP 7.1 changes causing this.

avatar mbabker
mbabker - comment - 22 Aug 2016

Also this reference - https://twitter.com/calevans/status/767767034696531970

Seems it might be a Travis thing causing the segfault.

avatar brianteeman brianteeman - change - 22 Aug 2016
Labels Added: ?
avatar mbabker
mbabker - comment - 23 Aug 2016

If you disable XDebug you should be able to get PHPUnit 4.8 to run on PHP 7.1.

avatar mbabker
mbabker - comment - 23 Aug 2016

https://gist.github.com/mbabker/568d45f5c5b627184652d90c4305a1c2

This gist has the current test failures after running on my machine. I've got PHP 7.1 beta 3 installed via Homebrew and have the XDebug extension disabled. It's the same configuration I have for my PHP 7.0 install except I usually have XDebug enabled.

avatar yvesh
yvesh - comment - 23 Aug 2016

7 failures sounds doable, when i have some time this week and you are not faster i am going to look into it.. :)

avatar mbabker
mbabker - comment - 23 Aug 2016

6 of the failures relate to my machine configuration (5 were because I have the Redis extension installed but wasn't running a Redis server instance and the sixth has always been an issue for me for some reason but it passes on every other system I've ran our test suite against). So it's really just one failure.

As for the 17 errors in the JHttp API, I haven't a clue where to start on those. The Framework tests all pass OK (however the Framework package isn't using a Registry object for the options, rather simple arrays or ArrayAccess objects) and neither the HTTP or Registry package tests fail on PHP 7.1 (actually all of the Framework packages passed with zero changes minus the one needed for the Google test because it was injecting a bad data type anyway, so more of a bad test than a code issue).

avatar photodude
photodude - comment - 28 Aug 2016

I agree the segfault is likely Travis or our code, PHPUnit 4.8 will test itself with no issues and just a few tests skipped. On Travis xdebug is not enabled for PHP 7.1 so that's not causing the segfault on Travis.

As for the "17 errors in the JHttp API" they all seem to have a common thread with Invalid argument supplied for foreach() and tests/unit/core/helper/helper.php:52

avatar mbabker
mbabker - comment - 28 Aug 2016

That helper.php line will be where all errors get raised from because of
the listener we have for the deprecation notices, you've gotta go back a
couple steps in the stack trace for the real error point.

On Sunday, August 28, 2016, Walt Sorensen notifications@github.com wrote:

I agree the segfault is likely Travis or our code, PHPUnit 4.8 will test
itself with no issues and just a few tests skipped
https://travis-ci.org/photodude/phpunit/jobs/155683192. On Travis
xdebug is not enabled for PHP 7.1
travis-ci/travis-ci#6321 so that's not
causing the segfault on Travis.

As for the "17 errors in the JHttp API" they all seem to have a common
thread with Invalid argument supplied for foreach() and
tests/unit/core/helper/helper.php:52


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11523 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfob3nVfmBoADy0_A8u5dn6JIi61Ybks5qkhfmgaJpZM4JfnwX
.

avatar photodude
photodude - comment - 29 Aug 2016

Looking back a step, foreach is supplied with this $this->options->get('transport.stream', array()), $this->options->get('transport.curl', array()), $this->options->get('transport.socket', array()),

seems like the get() might be at fault (someone would need to look at the values at the point of execution to see what's going on (maybe it's passing an empty array with these arguments)

or it could be something in the test setup

avatar mbabker
mbabker - comment - 29 Aug 2016

Except unless there's a behavior change in PHP 7.1 foreach allows empty
arrays, so that shouldn't be an issue.

On Sun, Aug 28, 2016 at 8:17 PM, Walt Sorensen notifications@github.com
wrote:

Looking back a step, foreach is supplied with this $this->options->get('transport.stream',
array()) or $this->options->get('transport.curl', array())

seems like the get() might be at fault (someone would need to look at the
values at the point of execution to see what's going on (maybe it's passing
an empty array with these arguments)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11523 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfofOo4rRN9PpvGUae2PF2K-O8bd4Wks5qkjMlgaJpZM4JfnwX
.

avatar photodude
photodude - comment - 29 Aug 2016

An Empty array was just a guess of something that might be an "invalid argument", my other guess would be that get() isn't returning an array at all. Someone would need to look at the values at the point of execution/failure in the tests to see what's going on.

ref: http://stackoverflow.com/questions/2630013/invalid-argument-supplied-for-foreach

avatar mbabker
mbabker - comment - 7 Oct 2016

So interestingly enough sometime recently the test suite started running on PHP 7.1 (at least the current staging build ran and passed; I only glanced because I noticed one of my 4.0 PRs was running).

avatar andrepereiradasilva
andrepereiradasilva - comment - 7 Oct 2016

Is always good when problems solve themselfes 😉

So can this be closed?

avatar andrepereiradasilva
andrepereiradasilva - comment - 7 Oct 2016

Btw it seems all branches passed the php 7.1 test

avatar mbabker
mbabker - comment - 7 Oct 2016

Leave it be for now. I have no idea if it's pure luck or some change (presumably in PHP; ironically enough the Framework packages have all been running OK on 7.1 though using PHPUnit 5) that fixed it, and I don't know if it's something that will regress.

avatar dgt41
dgt41 - comment - 7 Oct 2016

@mbabker can we remove the node tests from travis?

avatar mbabker
mbabker - comment - 7 Oct 2016

Ask someone involved with that. I just keep the unit test suite running while other people envision dropping the whole thing and rewriting from scratch.

avatar dgt41
dgt41 - comment - 7 Oct 2016

I mean the drone.io is up and running, so what's the point of using also travis for that?

avatar mbabker
mbabker - comment - 7 Oct 2016

Again, ask someone involved with that.

avatar rdeutz
rdeutz - comment - 7 Oct 2016

We will remove the drone.io test at some point, they are running now because we don't see too much problems with them and a long time test makes sense. It is not our final setup we will use drone0.5 instead of 0.4 we are using now but 0.5 is unstable and not ready to use.

avatar brianteeman
brianteeman - comment - 29 Oct 2016

Is this issue resolved now?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11523.

avatar rdeutz
rdeutz - comment - 30 Oct 2016

no

avatar mbabker
mbabker - comment - 30 Oct 2016

For the PHPUnit side of things, we're stuck between a rock and a hard place.

PHPUnit 4.8 doesn't support PHP 7 officially, so the mere fact it is working is good for us. PHPUnit 5 requires PHP 5.6 minimum, neither the 3.x or 4.0 branches meet this so PHPUnit 4.8 compatibility has to be retained even if we can restructure our CI to allow PHPUnit 5 to be used. We're using functionality that was removed from PHPUnit 5.0 and more stuff deprecated in newer PHPUnit 5 releases (mostly around class mocks). Considering the number of tests removed in the 4.0 branch, we might be able to work through addressing some of these issues there but the 3.x test suite is going to be too large to work through unless you go through with the idea proposed by some to just abandon it and start from scratch.

So long and short, while things are at least running against PHP 7.1, we're in a less than optimal state.

avatar photodude
photodude - comment - 25 Nov 2016

There are two issues for running our tests in a way that's possibly compatible with PHPUnit 4.8 and PHPUnit 5.x : Direct usage of getMock() and use of the assertTag()

PR #12990 addresses the Direct usage of getMock() by implementing getMockBuilder()

The assertTag() is a bit more problematic to resolve as it will likely take rewriting some 59 tests to use DOMXPath::evaluate() with assertTrue(). I believe this is the form the code will take after rewriting $this->assertTrue($xpath->evaluate('xpath expression returning boolean'), "message");

avatar brianteeman
brianteeman - comment - 9 Dec 2016

As @photodude has been doing work on getmockbuilder am I correct that this issue can be closed now?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11523.

avatar brianteeman brianteeman - change - 9 Dec 2016
The description was changed
Status New Information Required
Labels
avatar brianteeman brianteeman - edited - 9 Dec 2016
avatar mbabker
mbabker - comment - 9 Dec 2016

Depends what our goal of compatibility will be. Remember that PHPUnit 4.x does not officially support PHP 7 so we're really flirting with disaster if the PHP engine ever changes in a way that it completely stops working. And we still use deprecated PHPUnit functionality that was dropped in PHPUnit 5 so we can't reasonably come up with a solution that allows us to use that version of the program for supported environments (PHP 5.6+) until we've addressed that.

avatar photodude
photodude - comment - 9 Dec 2016

The assertTag() is the remaining major problem that still needs to be resolved in some 59 tests for things to be compatibility with PHPUnit 4.8 and PHPUnit 5.x (assuming that there is not other unknown issues)

avatar mbabker
mbabker - comment - 9 Dec 2016

There shouldn't be AFAIK. The Framework's test suite was easily converted to supporting PHPUnit 4.8 and 5.x, the CMS doesn't do anything different than the Framework packages.

avatar ymages
ymages - comment - 2 May 2017

Is Joomla 3.7 working 100% with php 7.1 now ?

avatar franz-wohlkoenig franz-wohlkoenig - change - 27 May 2017
Status Information Required Discussion
avatar mbabker mbabker - change - 29 May 2017
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2017-05-29 11:45:01
Closed_By mbabker
avatar mbabker mbabker - close - 29 May 2017
avatar mbabker
mbabker - comment - 29 May 2017

Closing. Things are running but not in an optimal state. We're stuck running PHPUnit 4.8 as long as we support PHP 5.5 and earlier. We could attempt a build plan that runs PHPUnit 5 or 6 depending on the PHP version, but that may also be a bit too complex to run efficiently with our current setup.

avatar photodude
photodude - comment - 29 May 2017

hopefully, we can fix The assertTag() issues for J4 since we are not constrained there.

avatar mbabker
mbabker - comment - 29 May 2017

We haven't revised the earlier projection of supporting PHP 5.5 yet, so right now while working on that is still welcome, it's not urgent.

Add a Comment

Login with GitHub to post a comment