HHVM testing needs to be in PHP 7 mode #2629
Conversation
- Suspend HHVM testing until the following items are resolved
- HHVM PHP 7 mode issues.
- facebook/hhvm#7626 php 7 mode not recognized by composer as PHP 7
- facebook/hhvm#7198 type annotations on internal functions, `expects parameter 1 to be _______, null given`
- facebook/hhvm#7544 HHVM INI settings for PHP7 mode `php 7 is not the same as php 7.1`
- Trusty Container issues (currently beta)
- travis-ci/travis-ci#6842 - MySQL socket issue on Trusty beta Container (assume fixed sometime after 1Q17)
- Add line to Force hhvm PHP 7 mode for future testing
- HHVM fails on composer `This package requires php ^7.0 but your HHVM version does not satisfy that requirement`
- reference for change https://docs.hhvm.com/hhvm/configuration/INI-settings#php-7-settings
|
I'd rather say that we suspend releases until above blockers are fixed. Removing tests is only going to make it worse, unless @travis-ci has some time to polish HHVM support. |
|
We can merge without the waiting for @travis-ci to fix the Trusty Container issues (currently beta), that item will only improve testing and make the .travis.yml a bit smaller. The other 3 items are total blockers for running tests as composer fails on HHVM in PHP 7 mode due to those 3 issues. so we are waiting on @facebook to fix HHVM's php 7 mode. I know one issue is in internal testing, but is slow in progress as it is a lot more complex than they originally thought and they keep having to fix the implementation during testing. Consider that We are 7+ months waiting on these fixes from @facebook, we likely will keep waiting unless someone external to the HHVM project can fix the issues. If this was just a travis CI issue with HHVM I would agree on keeping the tests. but at the moment we are looking tests which just take up a lot of testing time with no benefit due to the blocking issues to even run the tests. |
Different opinion here on "improving testing" ;-) Nobody over here is really in a hurry, and the builds are still green since HHVM is in allowed failures ( https://travis-ci.org/doctrine/dbal/builds/197306835 ). Let's keep it where it is, and we simply remove it from allowed failures when it's ripe. |
|
Updated as requested, Letting HHVM run as an allowed failure even though it's currently broken due to the issues listed above |
| dist: trusty | ||
| addons: | ||
| postgresql: "9.5" | ||
| 8000 | services: | |
| - postgresql | ||
| env: DB=pgsql POSTGRESQL_VERSION=9.5 | ||
| - php: hhvm | ||
| sudo: false | ||
| sudo: true |
There was a problem hiding this comment.
This builds is still not able to install dependencies via composer: https://travis-ci.org/doctrine/dbal/jobs/198403520#L467
Is this expected?
There was a problem hiding this comment.
Yes, see the listed HHVM issues in the PR statement. It's due to the type annotations issue.
| @@ -256,6 +256,10 @@ matrix: | |||
| - php: 7.1 | |||
| env: DB=pgsql POSTGRESQL_VERSION=9.6 | |||
|
|
|||
| before_install: | |||
| # Force hhvm PHP 7 mode | |||
| - if [[ $TRAVIS_PHP_VERSION = hhv* ]]; then echo hhvm.php7.all=1 >> /etc/hhvm/php.ini; fi | |||
There was a problem hiding this comment.
Is there a reason why we do not match hhvm* here?
There was a problem hiding this comment.
As far as I know It works better to do hhv* if multiple hhvm LTS Versions are specified. I always choose to do the conditionals this way on Travis to ensure future flexibility and reduced change sets
There was a problem hiding this comment.
I don't get it. What would other hhvm version strings look like? Don't they all start with hhvm?
There was a problem hiding this comment.
Yes hhvm or like hhvm-3.12
I think I may have tried hhvm* when I first got Travis to support specifying different versions of hhvm but only hhv* worked.
There was a problem hiding this comment.
$ deeky:~/dev/doctrine/dbal$ if [[ "hhvm" = hhvm* ]]; then echo "match"; fi
match
$ deeky:~/dev/doctrine/dbal$ if [[ "hhvm-3.12" = hhvm* ]]; then echo "match"; fi
match
But ok I won't be too picky here.
There was a problem hiding this comment.
IMO That's so much more verbose to accomplish the same thing without actually improving readability.
There was a problem hiding this comment.
? This were just two examples to show off that the pattern hhvm* matches hhvm as well as hhvm-3.12. Nevermind, I approved the PR already.
|
At least now the suite runs! Thanks @photodude! |
|
You are welcome, I should have made these changes when I had made the pgsql 9.6 changes. Glad it's merged now. Hopefully HHVM will get their issues resolved so composer will work again on hhvm. |
SuspendHHVM testing is broken until the following items are resolvedexpects parameter 1 to be _______, null givenphp 7 is not the same as php 7.1This package requires php ^7.0 but your HHVM version does not satisfy that requirement