Closed photodude closed 7 years ago
When the HHVM php7 compatibility flag is on, the errors greatly increase. Currently tests die at about 12% Click here to see an example build...
This is the error that seems to be killing the HHVM_with_PHP7_compatibility flag unitTests
Methods with the same name as their class will not be constructors in a future version of PHP; Cache_Lite has a deprecated constructor: 1x
1x in JCacheStorageTest::casesGetInstance
That should only be a deprecation notice and not a failure. That or HHVM has a behavior change compared to PHP 7.
According to HHVM when the flag hhvm.php7.all = 1
is set to fully enable PHP 7 mode one of the things it does is "Disallow and warn when using old PHP 4 constructors"
Seems HHVM with php7 mode may be far more strict about some of the PHP7 changes.
There is a PR for Cache_Lite that will resolve the PHP7 deprecated constructor issue.
At some point a decision about what HHVM LTS versions we want to test against will be needed. There is a feature request and a PR for Travis CI that will allow Trusty available HHVM LTS versions If/when that PR gets merged we will have the following options 3.3, 3.6, 3.9, 3.12, and when it's released in July hhvm 3.15.
Currently, we only have the option to test against hhvm
latest (currently 3.13 which is partially broken and has no ability to do PostgreSQL), or hhvm-nightly
(currently 3.14 dev) and with/without PHP7 mode (php7 mode for hhvm is only available in 3.11 or newer).
[Allow HHVM LTS version specification #733]() has been implemented and documented.
we now have access to the following
note: hhvm is latest (not always an LTS version), and hhvm-nightly is current nightly dev of HHVM
I suggest not testing against LTS versions older than 3.12since that is the first LTS with an option for php7 mode
That would suggest a maximum HHVM build matrix option as follows
- php: hhvm-3.15
sudo: true
dist: trusty
group: edge # until the next update
addons:
apt:
packages:
- mysql-server-5.6
- mysql-client-core-5.6
- mysql-client-5.6
services:
- mysql
- postgresql
- memcache
- memcached
- redis-server
env: HHVMPHP7="no" INSTALL_APCU="no" INSTALL_APCU_BC_BETA="no" # Disabled items that currently do not work in travis-ci hhvm
- php: hhvm-3.15
sudo: true
dist: trusty
group: edge # until the next update
addons:
apt:
packages:
- mysql-server-5.6
- mysql-client-core-5.6
- mysql-client-5.6
services:
- mysql
- postgresql
- memcache
- memcached
- redis-server
env: HHVMPHP7="yes" INSTALL_APCU="no" INSTALL_APCU_BC_BETA="no" # Disabled items that currently do not work in travis-ci hhvm
- php: hhvm
sudo: true
dist: trusty
group: edge # until the next update
addons:
apt:
packages:
- mysql-server-5.6
- mysql-client-core-5.6
- mysql-client-5.6
services:
- mysql
- postgresql
- memcache
- memcached
- redis-server
env: HHVMPHP7="no" INSTALL_APCU="no" INSTALL_APCU_BC_BETA="no" # Disabled items that currently do not work in travis-ci hhvm
- php: hhvm
sudo: true
dist: trusty
group: edge # until the next update
addons:
apt:
packages:
- mysql-server-5.6
- mysql-client-core-5.6
- mysql-client-5.6
services:
- mysql
- postgresql
- memcache
- memcached
- redis-server
env: HHVMPHP7="yes" INSTALL_APCU="no" INSTALL_APCU_BC_BETA="no" # Disabled items that currently do not work in travis-ci hhvm
- php: hhvm-nightly
sudo: true
dist: trusty
group: edge # until the next update
addons:
apt:
packages:
- mysql-server-5.6
- mysql-client-core-5.6
- mysql-client-5.6
services:
- mysql
- postgresql
- memcache
- memcached
- redis-server
env: HHVMPHP7="no" INSTALL_APCU="no" INSTALL_APCU_BC_BETA="no" # Disabled items that currently do not work in travis-ci hhvm
- php: hhvm-nightly
sudo: true
dist: trusty
group: edge # until the next update
addons:
apt:
packages:
- mysql-server-5.6
- mysql-client-core-5.6
- mysql-client-5.6
services:
- mysql
- postgresql
- memcache
- memcached
- redis-server
env: HHVMPHP7="yes" INSTALL_APCU="no" INSTALL_APCU_BC_BETA="no" # Disabled items that currently do not work in travis-ci hhvm
allow_failures:
- php: hhvm-3.15
- php: hhvm
- php: hhvm-nightly
before_script:
- composer install
- if [[ $HHVMPHP7 == "yes" ]]; then echo hhvm.php7.all=1 >> /etc/hhvm/php.ini; fi
This travis build matrix will build 6 versions of HHVM
HHVM postgresql driver issue the pg_set_error_verbosity()
function is not implemented in HHVM for various reasons
This missing function causes Fatal error: Call to undefined function pg_set_error_verbosity()
in joomla-cms/libraries/joomla/database/driver/postgresql.php on line 140
See test build on hhvm-3.12 with hhvm pgsql.so
Fatal error: Call to unimplemented native function pg_set_client_encoding() in /joomla-cms/libraries/joomla/database/driver/postgresql.php on line 890
the pg_set_client_encoding()
function is not implemented in HHVM for various reasons
In reviewing the JInstallerAdapterTest
failures I've come to the conclusion that the JInstallerAdapterTest
failures are due to something with the SQLite implimentation in HHVM. I believe the HHVM installed modules are pdo_sqlite and SQLite3 I don't know if that makes any difference.
JDocumentHtmlTest::testEnsureMergeHeadDataReturnsThisObject Array to string conversion failure is due to an intentional difference in the HHVM array_unique()
implimentation
HHVM's
array_unique()
method works by converting all the items in the arrays to strings then comparing, so an array to string notice is thrown. Zend does the conversion to string each time it does a compare.
@mbabker In checking with HHVM on the JDocumentHtmlTest::testEnsureMergeHeadDataReturnsThisObject Array
failure with HHVM's array_unique() method; they indicated the this is related to PHPUnit converting notices to exceptions.
I tried looking into what might need to be done with our PHPUnit tests to deal with the PHP errors so that the test will pass, but I'm more lost now than before on how to solve this testing issue.
I also think that the PHPUnit converting notices to exceptions is also related to the php cache lite test failure on the notice Disallow and warn when using old PHP 4 constructors
issue with HHVM in PHP7 mode.
Do you have any thoughts on how to deal with this?
I believe https://github.com/joomla/joomla-cms/pull/11565 fixes the memcache issues in HHVM.
I'm not sure we are going to be able to get PHP unit and HHVM to work together.
The owner/maintainer of PHPunit doesn't care about HHVM anymore and is likely to drop HHVM from their travis builds
Maybe this is a dead road in the project now (=_=)
sorry to ear that :(
shall we remove the hhvm tests on travis them?
(Stupid phone, closing things because of small buttons and big fingers)
@andrepereiradasilva I'm trying to see if I can find out if PHP Unit is going to drop the possibility of HHVM for sure, or if they are just currently annoyed with the past state of the project. If PHP unit is definitely dropping support for HHVM then we need to figure out if we are going to find a different unit test option for HHVM, or if we are going to kill HHVM support development.
IMO That's more of a PLT call
yes, i agree
I got access to the "alpha way to initiate a debug job via an API call on travis" and tried to figure out what was going on with the memory issue. After a few tests I had to "hack" the /libraries/vendor/phpunit/php-timer/src/Timer.php
to work around the failure caused by memory_get_peak_usage(true)
at the end of the tests.
I have edited the list of tests that error or fail to reflect the existing issues that would need to be addressed for the unit tests to pass.
I got acknowledgment from HHVM of the bug that was causing our "Fatal error: request has exceeded memory limit" issue. They have implemented a patch that should fix the issue in the next release (I assume 3.15.1)
I'm still working with HHVM to get a fix to the HHVM-nightly packages so I can test the patch and verify that the memory issue caused by memory_get_peak_usage(true)
in the phpunit timer is actually solved.
So the bug in HHVM causing the "Fatal error: request has exceeded memory limit" issue has been fixed in HHVM 3.16.0-dev (hhvm-nightly) unfortunately the patch got missed in the 3.15.1 LTS release. hopefully it gets included in the 3.15.2 LTS release.
nice
HHVM testing note with the complete test run: Time: 2.92 minutes, Memory: 270.03MB Tests: 6119, Assertions: 10341, Errors: 80, Failures: 3, Skipped: 119, Incomplete: 40. User deprecation notices (60)
A number of the Errors are new and due to a newly introduced connection issue with Postgresql
"PDOException: [112]: could not translate host name "localhost port=5432 dbname=joomla_ut" to address: Name or service not known"
The memory issue is in part the size of the test suite, in part our own configurations and code structure. If you look at the last HHVM builds for staging and 4.0-dev (where about half the test suite has been dumped thanks to dropping code for the Framework stack and relying on its tests) the HHVM build runs in full on both but the memory runs out at the end of the cycle on staging but no such issue with 4.0.
Yes, that is correct @mbabker less memory use by the test suit would complete and report (less than ~255mb) there was a bug in HHVM related to failing to Save ini values so they could be restored with ini_restore() which was the root cause of memory_get_peak_usage(true) failing if the memory use was greater than 256mb and preventing the report. So solved thanks to less tests and with the old test suite in staging when hhvm 3.15.2 or 3.16 is released. Here is a build I've ran showing it's working in hhvm-nightly (3.16) https://travis-ci.org/photodude/joomla-cms/jobs/164806859 (trying to see what's going on with PDO pgsql)
The connection issue with Postgresql
"PDOException: [112]: could not translate host name "localhost port=5432 dbname=joomla_ut" to address: Name or service not known"
is due to how HHVM was parsing the DSN for pdo_pgsql.so it was expecting semicolons as delimiters not spaces. An issue is open with HHVM and we should have a fix hopefully in hhvm 3.15.2
We can also fix it on our end by modifying the getConnection()
to use semi-colons rather than spaces /tests/unit/core/case/database/postgresql.php but we'll still have to wait until the release of hhvm 3.15.2 for the fix to undefined function pg_set_error_verbosity()
example build
@photodude can you have a look here: https://github.com/joomla/joomla-cms/pull/12341 & https://github.com/joomla/joomla-cms/pull/12336
The biggest HHVM testing issue to solve at the moment is the JInstallerAdapterTest
failure(s).
My best guess on those failures are due to something with the SQLite in HHVM preventing the insertion of columns for the #__extensions table. so far I have not been able to find away to trace the steps leading up to the failure.
For the JDatabaseDriverPostgresqlTest::testReleaseTransactionSavepoint pg_result_error_field() expects parameter 1 to be resource, boolean given
error caused by this call to pg_result_error_field()
when $this->cursor
is false from a query failing.
Seems like we need to better handle this error situation
maybe something like (adjusted to show Exception)
protected function getErrorNumber()
{
if ($this->cursor === false)
{
$this->errorMsg = pg_last_error($this->connection);
throw new JDatabaseExceptionExecuting($this->sql, $this->errorMsg);
}
return (int) pg_result_error_field($this->cursor, PGSQL_DIAG_SQLSTATE) . ' ';
}
Is this a reasonable course to solve the issue?
Maybe not a full solution.... the underlying error for the failure of JDatabaseDriverPostgresqlTest::testReleaseTransactionSavepoint
is ERROR: no such savepoint
which does not have an error code that could be parsed easily...
At least we know a little more.
Maybe adjust it to throw new JDatabaseExceptionExecuting($this->sql, $this->errorMsg);
when $this->cursor
is false
from a query failing.
Here is the PR for the suggested change to the Postgresql driver https://github.com/joomla/joomla-cms/pull/12359
Hey @photodude thanks for all your work on this. I've looked into this, too, and you already saw one of my fixes. The array_unique() issue is unfortunate, to say the least. I think the behavior of HHVM here is wrong, but anyway. Regarding the JDatabaseDriverPostgresqlTest::testReleaseTransactionSavepoint error, this seems like a bigger issue, since we don't seem to be using the Postgres functions correctly...
I still think that keeping compatible to HHVM would be good.
1) JDocumentHtmlTest::testEnsureMergeHeadDataReturnsThisObject Array to string conversion /tests/unit/core/helper/helper.php:52 /libraries/joomla/document/html.php:222 /tests/unit/suites/libraries/joomla/document/html/JDocumentHTMLTest.php:134
https://github.com/joomla/joomla-cms/issues/10220#issuecomment-233831836
is fixed in PR https://github.com/joomla/joomla-cms/pull/12013
@csthomas That's awesome glad to see the fix, I hope it's merged soon.
The following error will not be fixed in HHVM as noted in this issue https://github.com/facebook/hhvm/issues/1071
3) JUserHelperTest::testGetCryptedPassword
Password is hashed to crypt-blowfish without salt
Failed asserting that false is true.
/tests/unit/suites/libraries/joomla/user/JUserHelperTest.php:443
This is returning *0
indicating a failure (guessing the DES in not valid or there is a bug in HHVM).
JUserHelperTest::testGetCryptedPassword
failure is fixed by correcting our internal method, HHVM is telling us we didn't form the salt correctly. See PR https://github.com/joomla/joomla-cms/pull/12428
I additionally suggest that the JUserHelperTest::testGetCryptedPassword
failure can be skipped in the HHVM testing since the GetCryptedPassword()
method was replaced in 3.2.1 and is only there for BC considerations. Since 3.2.0 will never be HHVM compatible and 3.2.1+ doesn't use the method, we can effectively ignore this issue with 3.6.3+ and 4.0 doesn't have this method (any 3rd party extensions still using the old method should have changed).
I believe this HHVM issue https://github.com/facebook/hhvm/issues/2060 is the cause of the failure in our JLoaderTest::testSetupWithoutPrefixes
test on HHVM
Looks like we have PR's or have discovered HHVM bugs for all of the test issues and errors with the singular exception of the JInstallerAdapterTest
items.
Anyone have suggestions on how to proceed with solving those?
I made a small change to one of the JInstallerAdapterTest tests from $this->getMockDatabase()
to an already stored call to this object (array) $mockDatabase
the change works in php but I got a new error in HHVM
Fatal error: Call to undefined method
PHPUnit_Framework_MockObject_InvocationMocker::getTableColumns() in
/libraries/joomla/table/table.php on line 241
@mbabker I don't know much about the _autoloader
in JLoader, can _autoloader
be set to public? The references I've looked at have just identified _autoloader
as function
without public/private static
options. or can we switch to use the spl_autoload_register()
function?
Again sorry for my lack of experience and knowledge on autoloaders.
I honestly have no clue why it was marked private, but ever since the auto loader has been there, people have been filing issues because it is incompatible with third-party code that uses public functions.
On Tuesday, October 18, 2016, Walt Sorensen notifications@github.com wrote:
@mbabker https://github.com/mbabker I don't know much about the _autoloader in JLoader https://github.com/joomla/joomla-cms/blob/staging/libraries/loader.php#L578-L647, can _autoloader and _load be set to public? The references I've looked at http://php.net/manual/en/function.autoload.php have just identified those as function without public/private static options. or can we switch to use the spl_autoload_register() function?
Again sorry for my lack of experience and knowledge on autoloaders.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/joomla/joomla-cms/issues/10220#issuecomment-254702001, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWfoVAsWWjdzW2oaoO6k5mypS7b5p2bks5q1Y_7gaJpZM4IW1pu .
Thanks @mbabker my only assumption is it got marked private by habit since in php 4 there was a preference to mark methods private or protected with leading underscores.
In this instance, it seems like marking this private is the completely wrong thing to do. (it's questionable why it even works in php https://github.com/facebook/hhvm/pull/959#issuecomment-25482909 )
We now have solutions in proposed PRs, Merged PRs, or HHVM bug fixes for all known issues except the JInstallerAdapterTest
items and the JLanguageTest::testParse
invalid configuration notice .
Once everything is merged, and the few errors remaining are fixed, it should be reasonable to get people attempting to live test on HHVM again (something I don't think people have done since J3.3 and J3.4 due to the errors we had to fix and the bugs in HHVM prior to HHVM 3.15.2)
Biggest block to live testing with HHVM is now just the failures with the JInstallerAdapterTest
The more I try to dig into the JInstallerAdapterTest failures the more this is looking like an issue with PHPUnit 4.x and HHVM.
My best guess is there is an issue with the mocking of JTableExtension
in the JInstallerAdapter
tests.
I'm at a bit of a loss on how to solve that issue. Best case scenario is this is only a phpunit/hhvm issue and the J3.7.0 nightly build for staging should be good to go for real world testing on HHVM 3.15.2 with mysql or hhvm 3.16.0-dev nightly with PostgreSQL.
The fix for the JInstallerAdapter failure on hhvm is included in PR #12990
There are some new PDOMySql failures in the HHVM tests but that is due to a Travis configuration issue. I'm still waiting for Travis to fix that error since it's due to some change they did in the Trusty testing images.
Once we merge PR #12990 and either PR #12428 for a salted crypt fix or #12668 for a PR that just skips this old crypt test since it's not used in core and has been deprecated. Then we can close this issue and IMO we can include HHVM as a testing option for the 3.7 alpha release
12668 for a PR that just skips this old crypt test since it's not used in core and has been deprecated
Not acceptable. Whether an API is used in core or not, deprecated or not, it should still function as expected until the day it's removed. Likewise, if the class has tests, those should continue to function to validate the behavior.
Ok I'll close #12668 as not a valid solution to the testing issue.
Ok so to close this issue we need to merge PR #12990 and PR #12428
@photodude if everything here is finally resolved with PR #12990 and PR #12428 then it can be closed here?
@brianteeman Just waiting for PR #12990 to reach RTC or merged. With that PR everything is finally resolved with the HHVM tests.
This is a list of the tests which are known to be failing or have errors in the HHVM phpunit testing (allowed failures)
Unfortunately, due to an out memory limit error in Travis-ci I can only retrieve the names of the tests that errored or failed (test listed below)I was able to get some of the test failure details by filtering and only running one test at a time.
you can see the results of my first test here https://travis-ci.org/photodude/joomla-cms/jobs/127702652
There were
1190 errors:~~1) JInstallerAdapterTest::testCheckExistingExtensionForExistingExtension UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:103 2) JInstallerAdapterTest::testCheckExistingExtensionForExtensionThatDoesNotExist UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:148 3) JInstallerAdapterTest::testDiscoverInstall UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:371 4) JInstallerAdapterTest::testDiscoverInstallWithNoDescription UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:439 5) JInstallerAdapterTest::testInstall UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:827 6) JInstallerAdapterTest::testInstallOnUpdateRoute UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:901 7) JInstallerAdapterTest::testInstallAbortsWhenSetupUpdatesThrowsException UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:980 8) JInstallerAdapterTest::testInstallWithNoDescription UnexpectedValueException: No columns found for #extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:1052 9) JInstallerAdapterTest::testParseQueriesWithUpdateRouteAndParsingReturningTrueCallsParseSchemaUpdatesCorrectly UnexpectedValueException: No columns found for #__extensions table /libraries/joomla/table/table.php:245 /libraries/joomla/table/table.php:162 /libraries/joomla/table/extension.php:30 /tests/unit/suites/libraries/cms/installer/JInstallerAdapterTest.php:1330~~ See proposed PR #12990
~~10) JDocumentHtmlTest::testEnsureMergeHeadDataReturnsThisObject Array to string conversion /tests/unit/core/helper/helper.php:52 /libraries/joomla/document/html.php:222 /tests/unit/suites/libraries/joomla/document/html/JDocumentHTMLTest.php:134~~ See merged PR #12013
~~11) JDatabaseDriverPostgresqlTest::testReleaseTransactionSavepoint pg_result_error_field() expects parameter 1 to be resource, boolean given /home/travis/build/photodude/joomla-cms/tests/unit/core/helper/helper.php:52 /home/travis/build/photodude/joomla-cms/libraries/joomla/database/driver/postgresql.php:1513 /home/travis/build/photodude/joomla-cms/libraries/joomla/database/driver/postgresql.php:742 /home/travis/build/photodude/joomla-cms/libraries/joomla/database/driver/postgresql.php:1384 /home/travis/build/photodude/joomla-cms/tests/unit/suites/database/driver/postgresql/JDatabaseDriverPostgresqlTest.php:1205~~ See merged PR https://github.com/joomla/joomla-cms/pull/12359
There were
32 failuresFatal error: Call to undefined function pg_set_error_verbosity() in /home/travis/build/photodude/joomla-cms/libraries/joomla/database/driver/postgresql.php on line 140fixed in the release of HHVM 3.15.2~~1) JInstallerAdapterTest::testParseQueriesWithUpdateRouteAndParsingReturningFalseReturnsException Expectation failed for method name is equal to string:parseSchemaUpdates when invoked 1 time(s). Method was expected to be called 1 times, actually called 0 times.~~ See proposed PR #12990
~~2) JLoaderTest::testSetupWithoutPrefixes Failed asserting that true is false. /tests/unit/suites/libraries/joomla/JLoaderTest.php:621~~ See facebook/hhvm#2060 for related issue to JLoaderTest See PR https://github.com/joomla/joomla-cms/pull/12478 for a merged internal solution
~~3) JUserHelperTest::testGetCryptedPassword Password is hashed to crypt-blowfish without salt Failed asserting that false is true. /tests/unit/suites/libraries/joomla/user/JUserHelperTest.php:443~~ See merged PR https://github.com/joomla/joomla-cms/pull/12428 for a salted fix
At the conclusion of testing memory failsFixed in hhvm 3.16.0 and 3.15.2Fatal error: request has exceeded memory limit in /libraries/vendor/phpunit/php-timer/src/Timer.php on line 97
Invalid configuration directive notice 'JLanguageTest::testParse' This gives a notice but no failure or error